Opened 8 months ago

Closed 7 months ago

Last modified 5 months ago

#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):

https://codereview.adblockplus.org/29417597/

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.

Change History (8)

comment:1 Changed 8 months ago 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 8 months ago by mjethani

  • Description modified (diff)

comment:3 Changed 8 months ago by mjethani

  • Cc greiner added
  • Description modified (diff)

comment:4 Changed 8 months ago 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 7 months ago by abpbot

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

comment:6 Changed 7 months ago by mjethani

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:7 Changed 7 months ago by kzar

  • Milestone set to Adblock-Plus-for-Chrome-Opera-next

comment:8 Changed 5 months ago 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

Note: See TracTickets for help on using tickets.