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

Issue 29995555: Issue 7253 - Pre-render icons for badge on Chromium (Closed)

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

Description

Chrome seems to render the icon each time browserAction.setIcon is called. If we render the icon in advance onto a canvas, we save 5-10% in CPU usage. This patch pre-renders the icons into ImageData objects and passes the relevant ImageData object to browserAction.setIcon. Memory footprint: ~12 KB Time for initial render: <0.1% of startup time

Patch Set 1 #

Total comments: 1

Patch Set 2 : Restrict to Chromium #

Total comments: 8

Patch Set 3 : Use only 16x16 and 32x32 icons #

Patch Set 4 : Update comment #

Patch Set 5 : Update for loop #

Total comments: 1

Patch Set 6 : Use iconPath and iconImageData separately #

Patch Set 7 : Do not use 19x19 and 38x38 icons on any platform #

Patch Set 8 : Remove 19x19 and 38x38 icons entirely #

Patch Set 9 : Remove icons from metadata.chrome #

Total comments: 8

Patch Set 10 : Rebase on Rietveld patch 29997602 #

Patch Set 11 : Split into two methods #

Patch Set 12 : Rewrite renderIcons #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+57 lines, -6 lines) Patch
M ext/background.js View 1 2 3 4 5 6 7 8 9 10 2 chunks +13 lines, -1 line 0 comments Download
M lib/icon.js View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +44 lines, -5 lines 3 comments Download

Messages

Total messages: 26
Manish Jethani
5 months, 2 weeks ago (2019-02-01 14:28:42 UTC) #1
Manish Jethani
Patch Set 1 See description above. I'll file an issue. https://codereview.adblockplus.org/29995555/diff/29995556/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29995555/diff/29995556/lib/icon.js#newcode34 ...
5 months, 2 weeks ago (2019-02-01 14:42:43 UTC) #2
Manish Jethani
If you want to see what difference this patch makes, follow these steps before and ...
5 months, 2 weeks ago (2019-02-02 08:58:37 UTC) #3
Manish Jethani
Patch Set 2: Restrict to Chromium
5 months, 2 weeks ago (2019-02-02 11:55:09 UTC) #4
Manish Jethani
Also filed an issue now: https://issues.adblockplus.org/ticket/7253
5 months, 2 weeks ago (2019-02-02 11:56:05 UTC) #5
Sebastian Noack
https://codereview.adblockplus.org/29995555/diff/29996576/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29995555/diff/29996576/ext/background.js#newcode338 ext/background.js:338: 40: this._changes.icon.path.replace("$size", "40") This should go after trying to ...
5 months, 2 weeks ago (2019-02-03 06:43:18 UTC) #6
Manish Jethani
Patch Set 3-4 https://codereview.adblockplus.org/29995555/diff/29996576/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29995555/diff/29996576/ext/background.js#newcode338 ext/background.js:338: 40: this._changes.icon.path.replace("$size", "40") On 2019/02/03 06:43:18, ...
5 months, 2 weeks ago (2019-02-03 07:26:49 UTC) #7
Manish Jethani
Patch Set 5: Update for loop
5 months, 2 weeks ago (2019-02-03 07:31:52 UTC) #8
Sebastian Noack
https://codereview.adblockplus.org/29995555/diff/29996576/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29995555/diff/29996576/lib/icon.js#newcode68 lib/icon.js:68: loadImage("icons/abp-40-whitelisted.png"), On 2019/02/03 07:26:49, Manish Jethani wrote: > On ...
5 months, 2 weeks ago (2019-02-03 07:45:16 UTC) #9
Manish Jethani
https://codereview.adblockplus.org/29995555/diff/29996576/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29995555/diff/29996576/lib/icon.js#newcode68 lib/icon.js:68: loadImage("icons/abp-40-whitelisted.png"), On 2019/02/03 07:45:15, Sebastian Noack wrote: > On ...
5 months, 2 weeks ago (2019-02-03 08:00:02 UTC) #10
Sebastian Noack
On 2019/02/03 08:00:02, Manish Jethani wrote: > > Also if these sizes are in fact ...
5 months, 2 weeks ago (2019-02-03 08:09:01 UTC) #11
Manish Jethani
On 2019/02/03 08:09:01, Sebastian Noack wrote: > On 2019/02/03 08:00:02, Manish Jethani wrote: > > ...
5 months, 2 weeks ago (2019-02-03 08:15:30 UTC) #12
Manish Jethani
Patch Set 6: Use iconPath and iconImageData separately
5 months, 2 weeks ago (2019-02-03 08:15:53 UTC) #13
Manish Jethani
Patch Set 7: Do not use 19x19 and 38x38 icons on any platform I'm assuming ...
5 months, 2 weeks ago (2019-02-03 08:21:13 UTC) #14
Manish Jethani
On 2019/02/03 08:21:13, Manish Jethani wrote: > Patch Set 7: Do not use 19x19 and ...
5 months, 2 weeks ago (2019-02-03 08:23:26 UTC) #15
Sebastian Noack
How about removing the redundant icons first and the base this patch on top. My ...
5 months, 2 weeks ago (2019-02-03 08:29:01 UTC) #16
Manish Jethani
Patch Set 8: Remove 19x19 and 38x38 icons entirely Patch Set 9: Remove icons from ...
5 months, 2 weeks ago (2019-02-03 08:33:02 UTC) #17
Sebastian Noack
https://codereview.adblockplus.org/29995555/diff/29997590/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29995555/diff/29997590/ext/background.js#newcode411 ext/background.js:411: if (imageData) Maybe it would be better, to just ...
5 months, 2 weeks ago (2019-02-03 08:47:16 UTC) #18
Manish Jethani
On 2019/02/03 08:33:02, Manish Jethani wrote: > Patch Set 8: Remove 19x19 and 38x38 icons ...
5 months, 2 weeks ago (2019-02-03 08:47:20 UTC) #19
Manish Jethani
https://codereview.adblockplus.org/29995555/diff/29997590/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29995555/diff/29997590/lib/icon.js#newcode75 lib/icon.js:75: let sizes = [16, 32]; On 2019/02/03 08:47:16, Sebastian ...
5 months, 2 weeks ago (2019-02-03 09:09:36 UTC) #20
Manish Jethani
Patch Set 10: Rebase on Rietveld patch 29997602 Patch Set 11: Split into two methods ...
5 months, 2 weeks ago (2019-02-03 09:10:22 UTC) #21
Manish Jethani
Patch Set 12: Rewrite renderIcons This a better, there's no need to use Promise.all and ...
5 months, 2 weeks ago (2019-02-03 09:47:55 UTC) #22
Manish Jethani
https://codereview.adblockplus.org/29995555/diff/29997590/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29995555/diff/29997590/lib/icon.js#newcode75 lib/icon.js:75: let sizes = [16, 32]; On 2019/02/03 09:09:35, Manish ...
5 months, 2 weeks ago (2019-02-03 09:50:13 UTC) #23
Sebastian Noack
https://codereview.adblockplus.org/29995555/diff/29997590/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29995555/diff/29997590/lib/icon.js#newcode75 lib/icon.js:75: let sizes = [16, 32]; On 2019/02/03 09:09:35, Manish ...
5 months, 2 weeks ago (2019-02-03 10:07:54 UTC) #24
Manish Jethani
https://codereview.adblockplus.org/29995555/diff/29997590/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29995555/diff/29997590/lib/icon.js#newcode75 lib/icon.js:75: let sizes = [16, 32]; On 2019/02/03 10:07:53, Sebastian ...
5 months, 2 weeks ago (2019-02-03 10:37:55 UTC) #25
Sebastian Noack
5 months, 2 weeks ago (2019-02-03 10:41:14 UTC) #26
LGTM
Sign in to reply to this message.

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