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

Issue 29793590: Issue 6490 - Wrap browser.browserAction.set* with Promise

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 3 weeks ago by Jon Sonesen
Modified:
3 months, 1 week ago
Reviewers:
Sebastian Noack
CC:
kzar
Visibility:
Public.

Description

Issue 6490 - Wrap browser.browserAction.set* with Promise

Patch Set 1 #

Patch Set 2 : Address PS1 comment #

Total comments: 9

Patch Set 3 : Address PS2 comments #

Total comments: 11

Patch Set 4 : Address PS3 comments #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+39 lines, -52 lines) Patch
M ext/background.js View 1 2 3 1 chunk +36 lines, -52 lines 3 comments Download
M polyfill.js View 1 1 chunk +3 lines, -0 lines 1 comment Download

Messages

Total messages: 12
Jon Sonesen
4 months, 3 weeks ago (2018-05-29 23:07:18 UTC) #1
kzar
I've read through your discussion on the issue and I don't think this was the ...
4 months, 3 weeks ago (2018-05-30 11:31:23 UTC) #2
Jon Sonesen
On 2018/05/30 11:31:23, kzar wrote: > I've read through your discussion on the issue and ...
4 months, 3 weeks ago (2018-05-30 15:32:19 UTC) #3
Jon Sonesen
On 2018/05/30 15:32:19, Jon Sonesen wrote: > On 2018/05/30 11:31:23, kzar wrote: > > I've ...
4 months, 3 weeks ago (2018-05-30 15:34:32 UTC) #4
Jon Sonesen
https://codereview.adblockplus.org/29793590/diff/29805577/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29793590/diff/29805577/ext/background.js#newcode338 ext/background.js:338: try Now that this is in polyfill.js I suspect ...
4 months, 1 week ago (2018-06-12 21:18:02 UTC) #5
Sebastian Noack
https://codereview.adblockplus.org/29793590/diff/29805577/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29793590/diff/29805577/ext/background.js#newcode320 ext/background.js:320: return Promise.all(Object.keys(this._changes).map((change) => Nit: The parenthesis around the argument ...
4 months, 1 week ago (2018-06-13 01:35:34 UTC) #6
Jon Sonesen
https://codereview.adblockplus.org/29793590/diff/29805577/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29793590/diff/29805577/ext/background.js#newcode320 ext/background.js:320: return Promise.all(Object.keys(this._changes).map((change) => On 2018/06/13 01:35:33, Sebastian Noack wrote: ...
4 months, 1 week ago (2018-06-13 23:05:07 UTC) #7
Jon Sonesen
https://codereview.adblockplus.org/29793590/diff/29805577/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29793590/diff/29805577/ext/background.js#newcode389 ext/background.js:389: this._applyChanges().catch(() => On 2018/06/13 01:35:33, Sebastian Noack wrote: > ...
4 months, 1 week ago (2018-06-13 23:06:40 UTC) #8
Sebastian Noack
https://codereview.adblockplus.org/29793590/diff/29808578/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29793590/diff/29808578/ext/background.js#newcode322 ext/background.js:322: if (change == "iconPath") Combine the check here as ...
4 months ago (2018-06-16 02:02:53 UTC) #9
Jon Sonesen
https://codereview.adblockplus.org/29793590/diff/29808578/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29793590/diff/29808578/ext/background.js#newcode322 ext/background.js:322: if (change == "iconPath") On 2018/06/16 02:02:52, Sebastian Noack ...
3 months, 1 week ago (2018-07-09 21:15:25 UTC) #10
Jon Sonesen
Also, as discussed in IRC these changes are nt working for me in Ubuntu 18 ...
3 months, 1 week ago (2018-07-09 22:46:00 UTC) #11
Jon Sonesen
3 months, 1 week ago (2018-07-10 23:01:08 UTC) #12
https://codereview.adblockplus.org/29793590/diff/29826598/ext/background.js
File ext/background.js (right):

https://codereview.adblockplus.org/29793590/diff/29826598/ext/background.js#n...
ext/background.js:353: let onReplaced = (addedTabId, removedTabId) =>
Since I had trouble removing the check below this definition would still need to
be here.

https://codereview.adblockplus.org/29793590/diff/29826598/ext/background.js#n...
ext/background.js:368: if (!browser.tabs.onReplaced.hasListener(onReplaced))
Replacing this check with the applyChanges boolean does not seem to be a
reliable indication of whether a listener has been added or not. Removing the
hasListener check causes the badge color to stay blue.

https://codereview.adblockplus.org/29793590/diff/29826598/ext/background.js#n...
ext/background.js:373: }).catch(() =>
I still think this is more readable than the callback-ish way. But am happy to
change.

https://codereview.adblockplus.org/29793590/diff/29826598/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29793590/diff/29826598/polyfill.js#newcode24
polyfill.js:24: "browserAction.setBadgeBackgroundColor",
chromium-browser==66.0.3359.181-0ubuntu0.18.04.1 complains that the invocation
of all the set methods doesn't meet the parameter signature, reporting that it
is defined as `set*(object details)` not `set*(object details, function)`.
However, Chrome does not complain and neither does Chromium 67. So I am not sure
what is going on there or if it is of importance.
Sign in to reply to this message.

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