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)


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

  • Cc sebastian added
  • Description modified (diff)
  • Platform changed from Chrome to Unknown / Cross platform
  • Ready set
  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:2 Changed on 04/20/2017 at 03:50:48 PM by mjethani

  • Description modified (diff)

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

A commit referencing this issue has landed:
Issue 5161 - Use maps and sets where appropriate

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.

Chrome 49 / 59 / Windows 7
Opera 36 / 45 / Windows 7

Add Comment

Modify Ticket

Change Properties
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from mjethani.
Note: See TracTickets for help on using tickets.