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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 weeks, 5 days ago by Jon Sonesen
Modified:
2 days, 7 hours 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: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+59 lines, -69 lines) Patch
M ext/background.js View 1 2 1 chunk +56 lines, -69 lines 5 comments Download
M polyfill.js View 1 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 9
Jon Sonesen
2 weeks, 5 days 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 ...
2 weeks, 4 days 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 ...
2 weeks, 4 days 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 ...
2 weeks, 4 days 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 ...
5 days, 12 hours 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 ...
5 days, 7 hours 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 days, 10 hours 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 days, 10 hours ago (2018-06-13 23:06:40 UTC) #8
Sebastian Noack
2 days, 7 hours ago (2018-06-16 02:02:53 UTC) #9
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#n...
ext/background.js:322: if (change == "iconPath")
Combine the check here as well?

  if (change == "iconPath" && "setIcon" in browser.browserAction)

https://codereview.adblockplus.org/29793590/diff/29808578/ext/background.js#n...
ext/background.js:363: "setBadgeBackgroundColor" in browser.browserAction)
Nit: Please indent this line so that the condition on this line is aligned with
the condition on the line above:

  if (change == "badgeColor" &&
      "setBadgeBackgroundColor" in browser.browserAction)

https://codereview.adblockplus.org/29793590/diff/29808578/ext/background.js#n...
ext/background.js:372: let onReplaced = (addedTabId, removedTabId) =>
For better code locality, I'd define this listener right before registering it,
like you did before.

https://codereview.adblockplus.org/29793590/diff/29808578/ext/background.js#n...
ext/background.js:387: if (!browser.tabs.onReplaced.hasListener(onReplaced))
Is this check necessary? It might be better to just rely on this._changes:

  let applyChanges = false;

  if (!this._changes)
  {
    this._changes = {};
    applyChanges = true;
  }

  this._changes[name] = value;

  if (applyChanges)
  {
    this._applyChanges().then(...);
  }

https://codereview.adblockplus.org/29793590/diff/29808578/ext/background.js#n...
ext/background.js:392: }).catch(() =>
Instead of .then(onSuccess).catch(onFailure),
you can call .then(onSuccess, onFailure).
Sign in to reply to this message.

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