Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1890)

Issue 29417597: Issue 5161 - Use maps and sets where appropriate (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 months ago by Manish Jethani
Modified:
5 months ago
Reviewers:
kzar, Sebastian Noack
CC:
Wladimir Palant
Base URL:
https://hg.adblockplus.org/adblockpluschrome/
Visibility:
Public.

Description

Use maps and sets where appropriate The following changes have been made: - A number of instances of Object.create have been changed to use Map or Set objects instead - Some related optimizations, esp. in ext.PageMap In some cases the semantics have changed. For example, iteration is now guaranteed to be in insertion order. I have assumed that this won't break anything.

Patch Set 1 #

Total comments: 14

Patch Set 2 : Address comments to Patch Set 1 #

Total comments: 13

Patch Set 3 : Remove blank line #

Patch Set 4 : Replace nonEmptyPageMaps with allPageMaps #

Patch Set 5 : Revert previous change and remove unnecessary logic #

Patch Set 6 : Rebased #

Total comments: 2

Patch Set 7 : Rebased #

Patch Set 8 : Minor unrelated changes #

Unified diffs Side-by-side diffs Delta from patch set Stats (+71 lines, -85 lines) Patch
M ext/background.js View 1 2 3 4 5 6 7 8 chunks +46 lines, -44 lines 0 comments Download
M lib/devtools.js View 1 2 3 4 5 4 chunks +10 lines, -17 lines 0 comments Download
M lib/popupBlocker.js View 1 2 3 4 5 6 7 2 chunks +11 lines, -14 lines 0 comments Download
M lib/whitelisting.js View 2 chunks +3 lines, -3 lines 0 comments Download
M options.js View 1 2 1 chunk +1 line, -7 lines 0 comments Download

Messages

Total messages: 24
Manish Jethani
6 months ago (2017-04-19 16:33:29 UTC) #1
Manish Jethani
Patch Set 1
6 months ago (2017-04-19 16:46:07 UTC) #2
kzar
On 2017/04/19 16:46:07, Manish Jethani wrote: > Patch Set 1 Please could you file an ...
6 months ago (2017-04-20 06:51:48 UTC) #3
Manish Jethani
On 2017/04/20 06:51:48, kzar wrote: > Please could you file an issue for this? Filed ...
6 months ago (2017-04-20 15:41:27 UTC) #4
Manish Jethani
By the way, the mapIterable function could be implemented as a generator function like so: ...
6 months ago (2017-04-21 12:51:19 UTC) #5
Sebastian Noack
On 2017/04/21 12:51:19, Manish Jethani wrote: > I wasn't sure if we had a policy ...
5 months, 3 weeks ago (2017-04-29 22:46:33 UTC) #6
Manish Jethani
Patch Set 2: Address comments to Patch Set 1 https://codereview.adblockplus.org/29417597/diff/29417598/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29417597/diff/29417598/ext/background.js#newcode62 ext/background.js:62: ...
5 months, 2 weeks ago (2017-05-04 14:47:34 UTC) #7
Sebastian Noack
https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js#newcode48 ext/background.js:48: if (prevSize == 0) Why not just calling nonEmptyPageMaps.add() ...
5 months ago (2017-05-17 06:46:49 UTC) #8
Manish Jethani
Patch Set 2: Remove blank line Comments inline. https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js#newcode48 ext/background.js:48: if ...
5 months ago (2017-05-17 21:53:24 UTC) #9
Manish Jethani
On 2017/05/17 21:53:24, Manish Jethani wrote: > Patch Set 2: Remove blank line Sorry, messed ...
5 months ago (2017-05-17 21:55:26 UTC) #10
Sebastian Noack
https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js#newcode48 ext/background.js:48: if (prevSize == 0) On 2017/05/17 21:53:24, Manish Jethani ...
5 months ago (2017-05-18 12:23:00 UTC) #11
Manish Jethani
Comments inline. https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js#newcode48 ext/background.js:48: if (prevSize == 0) On 2017/05/18 12:23:00, ...
5 months ago (2017-05-18 21:16:37 UTC) #12
Sebastian Noack
https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js#newcode48 ext/background.js:48: if (prevSize == 0) On 2017/05/18 21:16:37, Manish Jethani ...
5 months ago (2017-05-18 21:42:12 UTC) #13
Manish Jethani
Patch Set 4: Replace nonEmptyPageMaps with allPageMaps https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js#newcode48 ext/background.js:48: if (prevSize ...
5 months ago (2017-05-18 22:39:09 UTC) #14
Sebastian Noack
https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js#newcode48 ext/background.js:48: if (prevSize == 0) On 2017/05/18 22:39:09, Manish Jethani ...
5 months ago (2017-05-19 12:01:14 UTC) #15
Manish Jethani
Patch Set 5: Revert previous change and remove unnecessary logic https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js#newcode48 ...
5 months ago (2017-05-19 16:20:34 UTC) #16
Sebastian Noack
LGTM https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js#newcode48 ext/background.js:48: if (prevSize == 0) On 2017/05/19 16:20:34, Manish ...
5 months ago (2017-05-19 17:33:14 UTC) #17
kzar
On 2017/05/19 17:33:14, Sebastian Noack wrote: > LGTM (I'm not planning to review this one, ...
5 months ago (2017-05-19 18:15:44 UTC) #18
Manish Jethani
On 2017/05/19 18:15:44, kzar wrote: > On 2017/05/19 17:33:14, Sebastian Noack wrote: > > LGTM ...
5 months ago (2017-05-20 00:22:52 UTC) #19
Manish Jethani
Patch Set 6: Rebased
5 months ago (2017-05-20 17:49:24 UTC) #20
Sebastian Noack
https://codereview.adblockplus.org/29417597/diff/29443555/lib/popupBlocker.js File lib/popupBlocker.js (left): https://codereview.adblockplus.org/29417597/diff/29443555/lib/popupBlocker.js#oldcode112 lib/popupBlocker.js:112: let {tabId} = details; Perhaps get rid of this ...
5 months ago (2017-05-21 20:40:38 UTC) #21
Manish Jethani
Patch Set 7: Rebased Patch Set 8: Minor unrelated changes https://codereview.adblockplus.org/29417597/diff/29443555/lib/popupBlocker.js File lib/popupBlocker.js (left): https://codereview.adblockplus.org/29417597/diff/29443555/lib/popupBlocker.js#oldcode112 ...
5 months ago (2017-05-21 22:17:07 UTC) #22
Sebastian Noack
Still LGTN
5 months ago (2017-05-21 22:26:46 UTC) #23
Sebastian Noack
5 months ago (2017-05-21 22:27:03 UTC) #24
On 2017/05/21 22:26:46, Sebastian Noack wrote:
> Still LGTN

*LGTM
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5