Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 5741004535627776: Fix toolbar icon customization in Australis (Closed)

Created:
Nov. 22, 2013, 5:06 p.m. by Wladimir Palant
Modified:
Sept. 15, 2016, 12:18 p.m.
Reviewers:
Felix Dahlke, tschuster
Visibility:
Public.

Description

Multiple changes here: * Default icon placement is nav-bar again, no longer addon-bar * Removed all add-on bar specific code, including checks for collapsed toolbars * Our icon isn`t necessarily a direct child of the toolbar, account for that * Use CustomizeUI API when available, fake it if it isn`t * Make sure our menu is still initialized even if the icon is placed inside the Australis panel * Silence a parameter redeclaration warning in _showNextNotification * While at it, simplify a bunch of code passages by using for..of.

Patch Set 1 #

Patch Set 2 : Fixed unintentional code duplication #

Patch Set 3 : Got rid of repeated require() calls #

Patch Set 4 : Fixed icon`s menu initialization when the icon is inside the Australis panel #

Patch Set 5 : Improved approach (custom button type), marked our custom API extensions #

Total comments: 2

Patch Set 6 : Added 32x32 icon and fixed my own review comments #

Total comments: 11
Unified diffs Side-by-side diffs Delta from patch set Stats (+423 lines, -257 lines) Patch
M chrome/content/ui/overlay.xul View 1 chunk +0 lines, -1 line 0 comments Download
M chrome/content/ui/settings.xul View 1 chunk +0 lines, -1 line 4 comments Download
M chrome/locale/en-US/overlay.dtd View 1 chunk +0 lines, -1 line 0 comments Download
A chrome/skin/abp-status-32.png View 1 2 3 4 5 Binary file 0 comments Download
M chrome/skin/overlay.css View 1 2 3 4 5 1 chunk +15 lines, -0 lines 0 comments Download
M lib/appSupport.js View 2 chunks +3 lines, -6 lines 2 comments Download
A lib/customizableUI.js View 1 2 3 4 5 1 chunk +324 lines, -0 lines 0 comments Download
M lib/ui.js View 1 2 3 4 15 chunks +76 lines, -243 lines 5 comments Download
M metadata.gecko View 1 chunk +5 lines, -5 lines 0 comments Download

Messages

Total messages: 14
Wladimir Palant
Nov. 22, 2013, 5:06 p.m. (2013-11-22 17:06:32 UTC) #1
Wladimir Palant
Nov. 22, 2013, 5:12 p.m. (2013-11-22 17:12:00 UTC) #2
Wladimir Palant
Nov. 22, 2013, 5:15 p.m. (2013-11-22 17:15:45 UTC) #3
Wladimir Palant
Nov. 22, 2013, 5:28 p.m. (2013-11-22 17:28:39 UTC) #4
Wladimir Palant
Improved approach (custom button type), marked our custom API extensions
Nov. 25, 2013, 9:31 a.m. (2013-11-25 09:31:49 UTC) #5
Wladimir Palant
http://codereview.adblockplus.org/5741004535627776/diff/5135032834326528/lib/customizableUI.js File lib/customizableUI.js (right): http://codereview.adblockplus.org/5741004535627776/diff/5135032834326528/lib/customizableUI.js#newcode253 lib/customizableUI.js:253: return {area: toolbar.id, placement: index}; Given that we don't ...
Nov. 27, 2013, 4:04 p.m. (2013-11-27 16:04:01 UTC) #6
Wladimir Palant
These changes have been pushed so that nightly users can already test them: https://hg.adblockplus.org/adblockplus/rev/2da8a4cb876a https://hg.adblockplus.org/adblockplus/rev/adb40c833539 ...
Dec. 2, 2013, 7:40 a.m. (2013-12-02 07:40:38 UTC) #7
Wladimir Palant
I've added a 32x32 status icon now so that our 24x24 icon isn't being stretched ...
Dec. 2, 2013, 10:58 a.m. (2013-12-02 10:58:00 UTC) #8
Wladimir Palant
http://codereview.adblockplus.org/5741004535627776/diff/5651442522128384/lib/ui.js File lib/ui.js (right): http://codereview.adblockplus.org/5741004535627776/diff/5651442522128384/lib/ui.js#newcode435 lib/ui.js:435: onCommand: this.onIconCommand These two lines don't actually do anything, ...
Dec. 2, 2013, 3:14 p.m. (2013-12-02 15:14:17 UTC) #9
tschuster
I haven't really reviewed your shim for CustomizeUI yet, because I want to look a ...
May 15, 2014, 8:12 p.m. (2014-05-15 20:12:29 UTC) #10
tschuster
lgtm The new API usage looks good, but I can't really verify your shim is ...
May 19, 2014, 11:21 a.m. (2014-05-19 11:21:29 UTC) #11
Felix Dahlke
On 2014/05/19 11:21:29, tschuster wrote: > lgtm > > The new API usage looks good, ...
July 8, 2014, 5:33 p.m. (2014-07-08 17:33:45 UTC) #12
Wladimir Palant
Sorry, looks like I forgot replying to Tom's comments. I don't think any of these ...
July 11, 2014, 7:15 a.m. (2014-07-11 07:15:54 UTC) #13
Felix Dahlke
Sept. 15, 2016, 9:09 a.m. (2016-09-15 09:09:10 UTC) #14
Sorry for letting this sit here so long. It has landed pre-review a long time
ago, so there's little point in a thorough review from my side now. Instead I
did a very high level review to see if anything bothers me. Nothing does, so
LGTM :)

Powered by Google App Engine
This is Rietveld