Issue 29793590: Issue 6490 - Wrap browser.browserAction.set* with Promise (Closed)
|
Created:
May 29, 2018, 11:07 p.m. by Jon Sonesen Modified:
Nov. 8, 2018, 9:02 p.m. Reviewers:
Sebastian Noack CC:
kzar Visibility:
Public. |
DescriptionIssue 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
MessagesTotal messages: 12
I've read through your discussion on the issue and I don't think this was the approach Sebastian was suggesting. I think he (correct me if I'm wrong) was suggesting to attempt the browser.set immediately instead of calling browser.tabs.get first, but to then handle the pre-rendered tab case on failure. That way we'd avoid deferring in the common case that the tab isn't a pre-rendered one.
On 2018/05/30 11:31:23, kzar wrote: > I've read through your discussion on the issue and I don't think this was the > approach Sebastian was suggesting. I think he (correct me if I'm wrong) was > suggesting to attempt the browser.set immediately instead of calling > browser.tabs.get first, but to then handle the pre-rendered tab case on failure. > That way we'd avoid deferring in the common case that the tab isn't a > pre-rendered one. Ah, shoot yeah you are right. I guess my thinking was a bit flawed here, because I thought that the case where the block element dialogue box is missing causing this error message usually only occurs once for me that it's batter to handle at the setIcon since prerendered tabs happen quite alot. But alas, the callback occurs every time no matter what so his approach is better. hehe thanks!
On 2018/05/30 15:32:19, Jon Sonesen wrote: > On 2018/05/30 11:31:23, kzar wrote: > > I've read through your discussion on the issue and I don't think this was the > > approach Sebastian was suggesting. I think he (correct me if I'm wrong) was > > suggesting to attempt the browser.set immediately instead of calling > > browser.tabs.get first, but to then handle the pre-rendered tab case on > failure. > > That way we'd avoid deferring in the common case that the tab isn't a > > pre-rendered one. > > Ah, shoot yeah you are right. I guess my thinking was a bit flawed here, because > I thought that the case where the block element dialogue box is missing causing > this error message usually only occurs once for me that it's batter to handle at > the setIcon since prerendered tabs happen quite alot. But alas, the callback > occurs every time no matter what so his approach is better. hehe thanks! (I set Dave as the reviewer since Sebastian is on vacation btw)
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#n... ext/background.js:338: try Now that this is in polyfill.js I suspect this not longer works for edge, but do not have a test VM at this exact moment.
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#n... ext/background.js:320: return Promise.all(Object.keys(this._changes).map((change) => Nit: The parenthesis around the argument list is optional in arrow functions with a single argument. Every where else we omit the parenthesis in this case. https://codereview.adblockplus.org/29793590/diff/29805577/ext/background.js#n... ext/background.js:322: if (change == "iconPath") Unrelated, but since you are changing this code anyway, we can simplify the Firefox for Android checks by combining them with the check here: if (change == "iconPath && "setIcon" in browser.browserAction) ... Same for the other methods below. https://codereview.adblockplus.org/29793590/diff/29805577/ext/background.js#n... ext/background.js:378: this._changes = null; It seems in case of failure (when we register the onReplaced handler to handle prerendered tabs) the changes (still to be performed) will no longer be known. https://codereview.adblockplus.org/29793590/diff/29805577/ext/background.js#n... ext/background.js:389: this._applyChanges().catch(() => If this._changes is not null (and an onReplaced handler has already been registered), we should not call _applyChanges() again but wait for the onReplaced handler to take care of them.
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#n... ext/background.js:320: return Promise.all(Object.keys(this._changes).map((change) => On 2018/06/13 01:35:33, Sebastian Noack wrote: > Nit: The parenthesis around the argument list is optional in arrow functions > with a single argument. Every where else we omit the parenthesis in this case. Oh yeah forgot about that, thanks https://codereview.adblockplus.org/29793590/diff/29805577/ext/background.js#n... ext/background.js:322: if (change == "iconPath") On 2018/06/13 01:35:33, Sebastian Noack wrote: > Unrelated, but since you are changing this code anyway, we can simplify the > Firefox for Android checks by combining them with the check here: > > if (change == "iconPath && "setIcon" in browser.browserAction) > ... > > Same for the other methods below. Acknowledged. https://codereview.adblockplus.org/29793590/diff/29805577/ext/background.js#n... ext/background.js:378: this._changes = null; On 2018/06/13 01:35:33, Sebastian Noack wrote: > It seems in case of failure (when we register the onReplaced handler to handle > prerendered tabs) the changes (still to be performed) will no longer be known. Acknowledged.
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#n... ext/background.js:389: this._applyChanges().catch(() => On 2018/06/13 01:35:33, Sebastian Noack wrote: > If this._changes is not null (and an onReplaced handler has already been > registered), we should not call _applyChanges() again but wait for the > onReplaced handler to take care of them. Acknowledged.
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).
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") On 2018/06/16 02:02:52, Sebastian Noack wrote: > Combine the check here as well? > > if (change == "iconPath" && "setIcon" in browser.browserAction) Acknowledged. https://codereview.adblockplus.org/29793590/diff/29808578/ext/background.js#n... ext/background.js:363: "setBadgeBackgroundColor" in browser.browserAction) On 2018/06/16 02:02:52, Sebastian Noack wrote: > 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) Acknowledged. https://codereview.adblockplus.org/29793590/diff/29808578/ext/background.js#n... ext/background.js:372: let onReplaced = (addedTabId, removedTabId) => On 2018/06/16 02:02:53, Sebastian Noack wrote: > For better code locality, I'd define this listener right before registering it, > like you did before. Acknowledged. https://codereview.adblockplus.org/29793590/diff/29808578/ext/background.js#n... ext/background.js:387: if (!browser.tabs.onReplaced.hasListener(onReplaced)) On 2018/06/16 02:02:53, Sebastian Noack wrote: > 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(...); > } Acknowledged. https://codereview.adblockplus.org/29793590/diff/29808578/ext/background.js#n... ext/background.js:392: }).catch(() => On 2018/06/16 02:02:53, Sebastian Noack wrote: > Instead of .then(onSuccess).catch(onFailure), > you can call .then(onSuccess, onFailure). Yeah, I am aware of that. I thought it was a bit more readable but your suggestion fits our callbackish coding style better.
Also, as discussed in IRC these changes are nt working for me in Ubuntu 18 on chromium 66. It seems to dislike the polyfill changes. 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:387: if (!browser.tabs.onReplaced.hasListener(onReplaced)) On 2018/06/16 02:02:53, Sebastian Noack wrote: > Is this check necessary? So perhaps i did this wrong but removing the check and the onReplaced definition and using the applyChanges boolean doesn't seem to work right. It appears to not be a reliable indicator of whether the listener is registered.
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. 
|
