Opened on 04/20/2017 at 01:59:39 PM
Closed on 05/22/2017 at 09:15:09 AM
Last modified on 07/10/2017 at 11:27:21 AM
#5161 closed change (fixed)
Use maps and sets where appropriate
| Reported by: | mjethani | Assignee: | mjethani |
|---|---|---|---|
| Priority: | Unknown | Milestone: | Adblock-Plus-1.13.3-for-Chrome-Opera |
| Module: | Platform | Keywords: | |
| Cc: | kzar, sebastian, greiner | Blocked By: | |
| Blocking: | Platform: | Unknown / Cross platform | |
| Ready: | yes | Confidential: | no |
| Tester: | Ross | Verified working: | yes |
| Review URL(s): | |||
Description (last modified by mjethani)
Background
This continues from #4795 and #4864.
In a number of places in adblockpluschrome we're still using plain objects instead of Map or Set instances. Ideally when we have a dynamic collection, say a list of event listeners, we should use one of these specialized objects.
What to change
In general, go through the code looking for instances where plain objects could be replaced with maps or sets as appropriate.
In particular the following changes could be made:
- In ext/background.js:
- nonEmptyPageMaps could be a set
- this._map in ext.PageMap could be a map
- framesOfTabs could be a map
- Values of framesOfTabs could also be maps
- If nonEmptyPageMaps is a set, avoid unnecessarily adding to or deleting from the set
- If this._map in ext.PageMap is a map, avoid creating an intermediate array inside ext.PageMap.prototype.keys, instead return the iterable
- In lib/devtools.js:
- panels could be a map
- In lib/popupBlocker.js:
- loadingPopups could be a map
- In lib/whitelisting.js:
- Values of sitekeys could be maps
Hints for testers
Since we're changing some of the core data structures in the background page, I would recommend testing ad blocking in general, along with Chrome DevTools, pop-up blocking, and sitekeys, as well as the options page.
I would also test with older versions of Chrome to make sure everything works properly, since we're using some of the newer JavaScript features here.
Attachments (0)
Change History (8)
comment:1 Changed on 04/20/2017 at 03:38:46 PM by mjethani
comment:3 Changed on 04/21/2017 at 12:42:51 PM by mjethani
- Cc greiner added
- Description modified (diff)
comment:4 Changed on 05/04/2017 at 02:52:17 PM by mjethani
- Description modified (diff)
- Summary changed from Use maps, sets, and null-prototype objects where appropriate to Use maps and sets where appropriate
comment:5 Changed on 05/22/2017 at 08:17:22 AM by abpbot
comment:6 Changed on 05/22/2017 at 09:15:09 AM by mjethani
- Resolution set to fixed
- Status changed from reviewing to closed
comment:7 Changed on 05/22/2017 at 09:26:25 AM by kzar
- Milestone set to Adblock-Plus-for-Chrome-Opera-next
comment:8 Changed on 07/10/2017 at 11:27:21 AM by Ross
- Tester changed from Unknown to Ross
- Verified working set
Done. Features like sitekeys, popup blocking, devtools work as expected.
ABP 1.13.2.1785
Chrome 49 / 59 / Windows 7
Opera 36 / 45 / Windows 7
A commit referencing this issue has landed:
Issue 5161 - Use maps and sets where appropriate