Opened 7 months ago

Closed 7 months ago

Last modified 6 months ago

#7253 closed change (fixed)

Pre-render icons for badge on Chromium

Reported by: mjethani Assignee: mjethani
Priority: P2 Milestone: Adblock-Plus-3.5-for-Chrome-Opera-Firefox
Module: Platform Keywords:
Cc: sebastian, kzar Blocked By:
Blocking: #7000 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29995555/

Description (last modified by mjethani)

Background

Chrome's implementation of chrome.browserAction.setIcon loads the image from the given path and renders it onto a 2D canvas; it then uses the resulting ImageData object for the icon.

In lib/icon.js we render the icons for notification animations. If we simply use the same code to render the regular icons for passing to chrome.browserAction.setIcon (which supports an imageData option), it would save some unnecessary re-rendering in Chrome's extensions framework.

Based on my experimental patch:

  • Rendering the icons into ImageData objects once takes less than 0.5% of the extension's total startup time
  • The ImageData objects cached in memory take up ~12 KB
  • Overall savings in CPU usage for typical browsing can be ~5-10% at least

PS: I got this idea from uBlock Origin.

What to change

In lib/icon.js, render the non-whitelisted and whitelisted versions of the icons of the 16x16 and 32x32 sizes into ImageData objects, and pass these objects to BrowserAction.prototype.setIcon as an additional parameter.

In ext/background.js, if the imageData parameter is set, pass the value to chrome.browserAction.setIcon; otherwise pass path as usual.

Hints for testers

Try this:

  1. Open two windows, load a site like nytimes.com in one and a site like msn.com in the other (but really any site where some ads are being blocked will do)
  2. In one of the windows, disable ad blocking for the domain
  3. Make sure the icon is gray after disabling ad blocking; also make sure it looks alright (quality is OK compared with previous version of Adblock Plus)
  4. Switch between the two windows and see that the icon changes between red (enabled) and gray (disabled); also make sure that the quality of the red icon is alright

Try this on the oldest supported version of Chrome (49+) as well as the latest version of Chrome.

Also see #7256.

Change History (12)

comment:1 Changed 7 months ago by mjethani

  • Owner set to mjethani
  • Review URL(s) modified (diff)

comment:2 Changed 7 months ago by mjethani

  • Description modified (diff)

comment:3 Changed 7 months ago by mjethani

  • Summary changed from Pre-render icons for badge to Pre-render icons for badge on Chromium

comment:4 Changed 7 months ago by mjethani

  • Description modified (diff)

comment:5 Changed 7 months ago by abpbot

A commit referencing this issue has landed:
Issue 7253 - Pre-render icons for badge on Chromium

comment:6 Changed 7 months ago by mjethani

  • Description modified (diff)

comment:7 Changed 7 months ago by mjethani

  • Description modified (diff)

comment:8 Changed 7 months ago by mjethani

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

comment:9 Changed 7 months ago by mjethani

  • Cc sebastian kzar added

comment:10 Changed 7 months ago by mjethani

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

comment:11 Changed 7 months ago by sebastian

  • Priority changed from Unknown to P2
  • Ready set

comment:12 Changed 6 months ago by ukacar

  • Verified working set
Note: See TracTickets for help on using tickets.