Issue 29532767: Issue 5593 - Use messaging in popup for prefs, whitelisting, and stats (Closed)
|
Created:
Aug. 31, 2017, 5:07 p.m. by Manish Jethani Modified:
Sept. 27, 2017, 10:13 p.m. Reviewers:
Sebastian Noack CC:
Thomas Greiner, Wladimir Palant, kzar Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
Patch Set 1 : Use messaging for prefs I/O #
Total comments: 4
Patch Set 2 : Use messaging for prefs in stats.js #
Total comments: 2
Patch Set 3 : Use messaging to communicate with filterComposer #
Total comments: 1
Patch Set 4 : Use chrome APIs instead of ext.pages #
Total comments: 7
Patch Set 5 : Move functions into popup.js #
Total comments: 3
Patch Set 6 : Check for page ready again after adding listener #
Total comments: 2
Patch Set 7 : Use promises for page ready check #Patch Set 8 : Remove callback parameter #
Total comments: 5
Patch Set 9 : Remove odd use of promise rejection #
Total comments: 1
Patch Set 10 : Check for page ready state only once #
Total comments: 1
Patch Set 11 : Remove listener on promise fulfillment #
Total comments: 3
Patch Set 12 : Refactor whenPageReady #
Total comments: 16
Patch Set 13 : Simplify whenPageReady #
Total comments: 4
Patch Set 14 : Add comment explaining createPageObject hack #
Total comments: 1
Patch Set 15 : Change whitelisting calls to use messaging #
Total comments: 15
Patch Set 16 : Rebase on next #Patch Set 17 : Fix lint error and simplify code #
Total comments: 1
Patch Set 18 : Revert to using ext.pages.open #
Total comments: 1
Patch Set 19 : Move require to notification.js #Patch Set 20 : Revert one more instance of ext.pages.open #
Total comments: 1
MessagesTotal messages: 43
Patch Set 1 Since the Prefs object is not going to be available in the popup once we make that change, we can read and write prefs by sending messages to the background page instead. Note: This is the alternative to 29532763 If you like this approach, I'll do it for stats.js as well, which also uses the Prefs object.
Comments inline. https://codereview.adblockplus.org/29532767/diff/29532768/popup.html File popup.html (right): https://codereview.adblockplus.org/29532767/diff/29532768/popup.html#newcode74 popup.html:74: <li id="stats-container" class="collapsed"> Collapsed by default. https://codereview.adblockplus.org/29532767/diff/29532768/popup.html#newcode81 popup.html:81: <ul id="stats" class="collapsible"> This has been renamed as per Thomas's suggestion, both here and in the JS and CSS files.
What is about the other APIs imported from the background page? I suppose we have to replace them with messaging as well, or do I miss something? Do you plan to do that with separate reviews. It might be easier to review all changes together.
https://codereview.adblockplus.org/29532767/diff/29532768/popup.html File popup.html (right): https://codereview.adblockplus.org/29532767/diff/29532768/popup.html#newcode25 popup.html:25: <script src="ext/content.js"></script> Do we need this? The messaging uses Chrome APIs directly, without going through ext (not that it would need anything defined in ext/content.js anyway).
On 2017/08/31 18:19:19, Sebastian Noack wrote: > It might be easier to review all changes > together. Seriously? I doubt that, small self-contained reviews are generally better.
On 2017/08/31 18:20:30, Wladimir Palant wrote: > On 2017/08/31 18:19:19, Sebastian Noack wrote: > > It might be easier to review all changes > > together. > > Seriously? I doubt that, small self-contained reviews are generally better. Well, this is not self-contained, but fragmented which is the opposite. Though I see how it has some advantages to look at changes with a limited scope, but the downside is that you miss the larger context and it is hard to judge whether one particular approach makes still sense later when you put all changes together. Also if committed individually it creates an inconsistent state in the repository.
On 2017/08/31 18:20:30, Wladimir Palant wrote: > On 2017/08/31 18:19:19, Sebastian Noack wrote: > > It might be easier to review all changes > > together. > > Seriously? I doubt that, small self-contained reviews are generally better. Yeah I was thinking of doing this incrementally over multiple commits. For this one it makes sense to make the same changes for stats.js as well.
On 2017/08/31 18:28:57, Sebastian Noack wrote: > On 2017/08/31 18:20:30, Wladimir Palant wrote: > > On 2017/08/31 18:19:19, Sebastian Noack wrote: > > > It might be easier to review all changes > > > together. > > > > Seriously? I doubt that, small self-contained reviews are generally better. > > Well, this is not self-contained, but fragmented which is the opposite. Though I > see how it has some advantages to look at changes with a limited scope, but the > downside is that you miss the larger context and it is hard to judge whether one > particular approach makes still sense later when you put all changes together. > Also if committed individually it creates an inconsistent state in the > repository. So I think both approaches have their pros and cons and what you're saying makes sense as well. I could still do multiple reviews as long as they're independent and then hold off the commits until all reviews are done. It's your call though.
On 2017/08/31 18:37:22, Manish Jethani wrote: > On 2017/08/31 18:28:57, Sebastian Noack wrote: > > On 2017/08/31 18:20:30, Wladimir Palant wrote: > > > On 2017/08/31 18:19:19, Sebastian Noack wrote: > > > > It might be easier to review all changes > > > > together. > > > > > > Seriously? I doubt that, small self-contained reviews are generally better. > > > > Well, this is not self-contained, but fragmented which is the opposite. Though > I > > see how it has some advantages to look at changes with a limited scope, but > the > > downside is that you miss the larger context and it is hard to judge whether > one > > particular approach makes still sense later when you put all changes together. > > Also if committed individually it creates an inconsistent state in the > > repository. > > So I think both approaches have their pros and cons and what you're saying makes > sense as well. I could still do multiple reviews as long as they're independent > and then hold off the commits until all reviews are done. > > It's your call though. How about multiple patch sets in the same review? So reviewers can look at individual sub-changes separately, as well as looking at all changes together.
On 2017/08/31 19:22:05, Sebastian Noack wrote: > How about multiple patch sets in the same review? So reviewers can look at > individual sub-changes separately, as well as looking at all changes together. How do you handle modifications then?
On 2017/08/31 19:23:42, Wladimir Palant wrote: > On 2017/08/31 19:22:05, Sebastian Noack wrote: > > How about multiple patch sets in the same review? So reviewers can look at > > individual sub-changes separately, as well as looking at all changes together. > > How do you handle modifications then? We could do this incrementally within the same review. Each patch set would build on the previous one, but with more modifications. Then it would be a work in progress but reviewers would have the chance to give feedback along the way.
Patch Set 2: Use messaging for prefs in stats.js With this update the popup code no longer depends on the Prefs object. https://codereview.adblockplus.org/29532767/diff/29532768/popup.html File popup.html (right): https://codereview.adblockplus.org/29532767/diff/29532768/popup.html#newcode25 popup.html:25: <script src="ext/content.js"></script> On 2017/08/31 18:19:32, Wladimir Palant wrote: > Do we need this? The messaging uses Chrome APIs directly, without going through > ext (not that it would need anything defined in ext/content.js anyway). Right, it's not required. I've removed it now. https://codereview.adblockplus.org/29532767/diff/29533650/ext/popup.js File ext/popup.js (right): https://codereview.adblockplus.org/29532767/diff/29533650/ext/popup.js#newcode36 ext/popup.js:36: toggle(key, callback) The callback will be needed here if 29533613 lands. https://codereview.adblockplus.org/29532767/diff/29533650/stats.js File stats.js (right): https://codereview.adblockplus.org/29532767/diff/29533650/stats.js#newcode138 stats.js:138: ext.prefs.get("show_statsinicon", showStatsInIcon => Calling prefs.get here to get the new value should not be necessary. I'd like to make prefs.toggle return the new value, see 29533613. If we leave it this way there's a noticeable delay in the UI update.
Patch Set 3: Use messaging to communicate with filterComposer https://codereview.adblockplus.org/29532767/diff/29533656/popup.html File popup.html (right): https://codereview.adblockplus.org/29532767/diff/29533656/popup.html#newcode25 popup.html:25: <script src="ext/content.js"></script> Now we really need this.
Patch Set 4: Use chrome APIs instead of ext.pages
https://codereview.adblockplus.org/29532767/diff/29533671/ext/popup.js File ext/popup.js (right): https://codereview.adblockplus.org/29532767/diff/29533671/ext/popup.js#newcode29 ext/popup.js:29: }; While moving this function around anyway, how about just removing it, and calling window.close() directly, as discussed before on another review? https://codereview.adblockplus.org/29532767/diff/29533671/ext/popup.js#newcode40 ext/popup.js:40: }; IMO, this doesn't belong into ext. Prefs are part of the application logic, not the abstraction layer. https://codereview.adblockplus.org/29532767/diff/29533671/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29533671/popup.js#newcode52 popup.js:52: page = ext.createPageObject(tabs[0]); Why do you need to wrap this object here?
Patch Set 5: Move functions into popup.js https://codereview.adblockplus.org/29532767/diff/29533671/ext/popup.js File ext/popup.js (right): https://codereview.adblockplus.org/29532767/diff/29533671/ext/popup.js#newcode29 ext/popup.js:29: }; On 2017/09/13 02:49:39, Sebastian Noack wrote: > While moving this function around anyway, how about just removing it, and > calling window.close() directly, as discussed before on another review? OK, this should happen naturally now after changeset b615bfdf4897 is merged into master. https://codereview.adblockplus.org/29532767/diff/29533671/ext/popup.js#newcode40 ext/popup.js:40: }; On 2017/09/13 02:49:39, Sebastian Noack wrote: > IMO, this doesn't belong into ext. Prefs are part of the application logic, not > the abstraction layer. I've moved it into popup.js now. https://codereview.adblockplus.org/29532767/diff/29533671/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29533671/popup.js#newcode52 popup.js:52: page = ext.createPageObject(tabs[0]); On 2017/09/13 02:49:39, Sebastian Noack wrote: > Why do you need to wrap this object here? The checkWhilelisted call further down requires a Page object, for one. We would also need to create a URL object, page.url here. I've moved the createPageObject function into popup.js though. https://codereview.adblockplus.org/29532767/diff/29533671/stats.js File stats.js (right): https://codereview.adblockplus.org/29532767/diff/29533671/stats.js#newcode140 stats.js:140: ext.prefs.get("show_statsinicon", showStatsInIcon => There's no need to call prefs.get now since prefs.toggle already returns the new value. https://codereview.adblockplus.org/29532767/diff/29547558/dependencies File dependencies (right): https://codereview.adblockplus.org/29532767/diff/29547558/dependencies#newcode6 dependencies:6: adblockplusui = adblockplusui hg:8ad6a38e6870 git:9460adb Update this dependency here so prefs.toggle returns a value.
(Please at least Cc me for platform reviews, I've added myself now.) The way that popup.js shares state (and the require function) with the background page causes us problems with the switch to Webpack. I think we'd be better to replace all uses of require with messaging here.
I still think that this change needs to be finished and first, removing other requires can be done in separate commits and reviews. Otherwise this will get very messy and take unnecessarily long. https://codereview.adblockplus.org/29532767/diff/29547558/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29547558/popup.js#newcode92 popup.js:92: waitForPageReady(page.id, () => That's a potential race condition - what if "composer.ready" message is received before we call waitForPageReady()? Probably requires message reordering and won't happen, but this logic is still awkward. I think that waitForPageReady should start listening for "composer.ready" message and send "composer.isPageReady" message at the same time. Both "composer.ready" and a positive response to "composer.isPageReady" should trigger the callback then.
On 2017/09/18 12:01:28, Wladimir Palant wrote: > I still think that this change needs to be finished and first, removing other > requires can be done in separate commits and reviews. Otherwise this will get > very messy and take unnecessarily long. Fair enough, I'll open a new issue for that.
Patch Set 6: Check for page ready again after adding listener https://codereview.adblockplus.org/29532767/diff/29547558/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29547558/popup.js#newcode92 popup.js:92: waitForPageReady(page.id, () => On 2017/09/18 12:01:27, Wladimir Palant wrote: > That's a potential race condition - what if "composer.ready" message is received > before we call waitForPageReady()? Probably requires message reordering and > won't happen, but this logic is still awkward. I think that waitForPageReady > should start listening for "composer.ready" message and send > "composer.isPageReady" message at the same time. Both "composer.ready" and a > positive response to "composer.isPageReady" should trigger the callback then. I've made the change, though I couldn't tell why this would be the case, since if composer.isPageReady returns false we immediately add the handler for composer.ready. https://codereview.adblockplus.org/29532767/diff/29549568/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29549568/popup.js#newcode63 popup.js:63: handlePageReady = null; Set this to null so it's never called again. https://codereview.adblockplus.org/29532767/diff/29549568/popup.js#newcode102 popup.js:102: checkPageReady(page.id, ready => We still do this check since it's going to return true in most cases, then we can avoid adding and then removing the "nohtml" CSS class unnecessarily. For a page that is not ready though this means an extra round trip.
Patch Set 6: Use promises for page ready check Same, but use promises instead.
On 2017/09/19 09:24:44, Manish Jethani wrote: > Patch Set 6: Use promises for page ready check ^^^ 7, not 6
Patch Set 8: Remove callback parameter
https://codereview.adblockplus.org/29532767/diff/29549595/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29549595/popup.js#newcode91 popup.js:91: new Promise(resolve => assertPageReady(pageId).then(resolve)) You cannot just ignore the rejection reason of a promise, the JS engine will warn about that. You need .catch(...) here as well. https://codereview.adblockplus.org/29532767/diff/29549595/popup.js#newcode112 popup.js:112: whenPageReady(page.id).then(() => I think that "nohtml" class should just be there initially. You can drop assertPageReady() call and call whenPageReady() immediately. This should also let you simplify the helper functions above.
Patch Set 9: Remove odd use of promise rejection https://codereview.adblockplus.org/29532767/diff/29549595/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29549595/popup.js#newcode91 popup.js:91: new Promise(resolve => assertPageReady(pageId).then(resolve)) On 2017/09/19 10:17:14, Wladimir Palant wrote: > You cannot just ignore the rejection reason of a promise, the JS engine will > warn about that. You need .catch(...) here as well. I've removed this odd use of promise rejection now. https://codereview.adblockplus.org/29532767/diff/29549595/popup.js#newcode112 popup.js:112: whenPageReady(page.id).then(() => On 2017/09/19 10:17:14, Wladimir Palant wrote: > I think that "nohtml" class should just be there initially. You can drop > assertPageReady() call and call whenPageReady() immediately. This should also > let you simplify the helper functions above. Acknowledged. I'll look into this next.
Patch Set 10: Check for page ready state only once https://codereview.adblockplus.org/29532767/diff/29549595/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29549595/popup.js#newcode112 popup.js:112: whenPageReady(page.id).then(() => On 2017/09/19 10:22:50, Manish Jethani wrote: > On 2017/09/19 10:17:14, Wladimir Palant wrote: > > I think that "nohtml" class should just be there initially. You can drop > > assertPageReady() call and call whenPageReady() immediately. This should also > > let you simplify the helper functions above. > > Acknowledged. > > I'll look into this next. Done. https://codereview.adblockplus.org/29532767/diff/29549716/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29549716/popup.js#newcode87 popup.js:87: // Check if the page is ready after adding the listener just in case we've This comment seems unnecessary now as the code is more self-explanatory. https://codereview.adblockplus.org/29532767/diff/29549757/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29549757/popup.js#newcode99 popup.js:99: document.body.classList.remove("nohtml"); Remove nohtml even if it's local.
Patch Set 11: Remove listener on promise fulfillment https://codereview.adblockplus.org/29532767/diff/29549777/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29549777/popup.js#newcode60 popup.js:60: ext.onMessage.addListener(listener = (message, sender) => Assign listener here when it's added. https://codereview.adblockplus.org/29532767/diff/29549777/popup.js#newcode62 popup.js:62: let {id: senderPageId} = sender.page || {}; sender.page can be undefined. https://codereview.adblockplus.org/29532767/diff/29549777/popup.js#newcode79 popup.js:79: ext.onMessage.removeListener(listener); When either of the promises is fulfilled, remove the listener. We need to do this, otherwise ext.onMessage will hold on to the listener (and the surrounding closure in turn) and keep sending messages.
Patch Set 12: Refactor whenPageReady https://codereview.adblockplus.org/29532767/diff/29549882/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29549882/popup.js#newcode98 popup.js:98: promise.then(cleanUp, error => We need to clean up on both fulfillment and rejection. This is the equivalent of Promise.prototype.finally [1]. Also we have to stick the clean-up handler on the original promise so any errors thrown from there don't reject the returned promise. [1]: https://github.com/tc39/proposal-promise-finally
https://codereview.adblockplus.org/29532767/diff/29549882/ext/popup.js File ext/popup.js (left): https://codereview.adblockplus.org/29532767/diff/29549882/ext/popup.js#oldcode13 ext/popup.js:13: }; It seems you need to rebase. There is no closePopup() in here anymore, as of the current "next" bookmark. https://codereview.adblockplus.org/29532767/diff/29549882/notification.js File notification.js (right): https://codereview.adblockplus.org/29532767/diff/29549882/notification.js#new... notification.js:85: chrome.tabs.create({url: link.href}); I'm all for replacing usage of ext.* wit chrome.* (eventually getting rid completely of the ext namepsace). But this change seems to be unrelated here. Can you please submit a separate patch, that is replacing ext.pages.open with chrome.tabs.create, not only here, but everywhere? https://codereview.adblockplus.org/29532767/diff/29549882/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29549882/popup.js#newcode29 popup.js:29: function createPageObject(tab) I suppose this hack is matter to be removed in a later iteration, where we move the checkWhitelisted() call to the background page as well? If so, can you please add a comment here, explaining why creating a fake Page object is necessary and that this is just a temporary workaround? https://codereview.adblockplus.org/29532767/diff/29549882/popup.js#newcode104 popup.js:104: return promise; It seems the logic here would be much simpler and easier to follow, if you use less promise-ifcation: return new Promise(resolve => { function onMessage(message, sender) { if (message.type == "composer.ready" && sender.page && sender.page.id == pageId) { ext.onMessage.removeListener(onMessage); resolve(); } } chrome.runtime.sendMessage( {type: "composer.isPageReady", pageId}, response => { if (response) { ext.onMessage.removeListener(onMessage); resolve(); } } ); ext.onMessage.addListener(onMessage); });
Patch Set 13: Simplify whenPageReady I'll address the other comments in the next update. https://codereview.adblockplus.org/29532767/diff/29549882/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29549882/popup.js#newcode104 popup.js:104: return promise; On 2017/09/20 18:57:02, Sebastian Noack wrote: > It seems the logic here would be much simpler and easier to follow, if you use > less promise-ifcation: > [...] Yeah, that was a bit overkill. Done. https://codereview.adblockplus.org/29532767/diff/29550811/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29550811/popup.js#newcode36 popup.js:36: let pageObject = {id: tab.id}; We need to rename this to get rid of a lint error, since there's already a variable called page in the outer scope. https://codereview.adblockplus.org/29532767/diff/29550811/popup.js#newcode64 popup.js:64: let onMessage = (message, sender) => I meant to ask, is it preferred to use "function foo() { ... }" or "let foo = () => { ... }" in such instances? I find it better to use the latter, especially since the function doesn't need to have its own "this". It often also leads to shorter code, sometimes one-liners.
Patch Set 14: Add comment explaining createPageObject hack https://codereview.adblockplus.org/29532767/diff/29549882/ext/popup.js File ext/popup.js (left): https://codereview.adblockplus.org/29532767/diff/29549882/ext/popup.js#oldcode13 ext/popup.js:13: }; On 2017/09/20 18:57:02, Sebastian Noack wrote: > It seems you need to rebase. There is no closePopup() in here anymore, as of the > current "next" bookmark. OK, I'll rebase on master instead once next has been merged, which I'm guessing is soon. https://codereview.adblockplus.org/29532767/diff/29549882/notification.js File notification.js (right): https://codereview.adblockplus.org/29532767/diff/29549882/notification.js#new... notification.js:85: chrome.tabs.create({url: link.href}); On 2017/09/20 18:57:02, Sebastian Noack wrote: > I'm all for replacing usage of ext.* wit chrome.* (eventually getting rid > completely of the ext namepsace). But this change seems to be unrelated here. Actually the goal of this set of changes was to get rid of any dependence on the background page, therefore we had to get rid of uses of ext.pages or ext.Page as part of these changes. It is definitely not unrelated. > Can you please submit a separate patch, that is replacing ext.pages.open with > chrome.tabs.create, not only here, but everywhere? I can file an issue for the general case of getting rid of ext.pages and others throughout the codebase. https://codereview.adblockplus.org/29532767/diff/29549882/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29549882/popup.js#newcode29 popup.js:29: function createPageObject(tab) On 2017/09/20 18:57:02, Sebastian Noack wrote: > I suppose this hack is matter to be removed in a later iteration, where we move > the checkWhitelisted() call to the background page as well? If so, can you > please add a comment here, explaining why creating a fake Page object is > necessary and that this is just a temporary workaround? Done. By the way, there is a "filters.isPageWhitelisted" in lib/whitelisting.js, it is not used anywhere right now (?), and it takes a page object as well. I think this should be changed so it takes a page-like object with only an id and a url, then it can construct a full ext.Page object locally before passing it to checkWhitelisted. Same for getBlockedPerPage.
Patch Set 15: Change whitelisting calls to use messaging Most uses of require are gone, and entirely in popup.js. https://codereview.adblockplus.org/29532767/diff/29551555/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29551555/popup.js#newcode23 popup.js:23: const {FilterStorage} = require("filterStorage"); Most uses of require are gone, and entirely from popup.js. https://codereview.adblockplus.org/29532767/diff/29551564/lib/whitelisting.js File lib/whitelisting.js (right): https://codereview.adblockplus.org/29532767/diff/29551564/lib/whitelisting.js... lib/whitelisting.js:91: port.on("filters.isWhitelisted", message => Changed this to isWhitelisted since "Page" is kinda implied, also corresponds to checkWhitelisted. https://codereview.adblockplus.org/29532767/diff/29551564/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29551564/popup.js#newcode22 popup.js:22: let tab = null; So now it's just a lightweight tab object and createPageObject is gone. https://codereview.adblockplus.org/29532767/diff/29551564/popup.js#newcode34 popup.js:34: function isPageWhitelisted(callback) I wonder if we should always use promises here. https://codereview.adblockplus.org/29532767/diff/29551564/popup.js#newcode74 popup.js:74: tab = tabs[0] && {id: tabs[0].id, url: tabs[0].url}; I don't like to hold on to an object passed in by the browser, as it may cause memory leaks, especially when it represents a browser tab, so I'm creating a new "tab" object with just the id and the url (both primitive types). https://codereview.adblockplus.org/29532767/diff/29551564/popup.js#newcode157 popup.js:157: chrome.tabs.sendMessage(tab.id, { I forgot to account for this earlier. It needs to use tabs.sendMessage directly.
https://codereview.adblockplus.org/29532767/diff/29551564/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29551564/popup.js#newcode24 popup.js:24: function getPref(key, callback) By the way, since we moved getPref and togglePref into popup.js, now we're getting a linter error in stats.js for these functions as they are not declared global. Any thoughts on how to deal with this?
https://codereview.adblockplus.org/29532767/diff/29549882/ext/popup.js File ext/popup.js (left): https://codereview.adblockplus.org/29532767/diff/29549882/ext/popup.js#oldcode13 ext/popup.js:13: }; On 2017/09/21 06:11:16, Manish Jethani wrote: > On 2017/09/20 18:57:02, Sebastian Noack wrote: > > It seems you need to rebase. There is no closePopup() in here anymore, as of > the > > current "next" bookmark. > > OK, I'll rebase on master instead once next has been merged, which I'm guessing > is soon. Why not rebasing against "next" now? If the patch applies on "next", it will also apply on "master", once "next" has been merged. But it makes it unnecessarily difficult to review changes based on outdated code, and if a merge conflict needs to be resolved later, I'd have to review the changes again. https://codereview.adblockplus.org/29532767/diff/29549882/notification.js File notification.js (right): https://codereview.adblockplus.org/29532767/diff/29549882/notification.js#new... notification.js:85: chrome.tabs.create({url: link.href}); On 2017/09/21 06:11:16, Manish Jethani wrote: > On 2017/09/20 18:57:02, Sebastian Noack wrote: > > I'm all for replacing usage of ext.* wit chrome.* (eventually getting rid > > completely of the ext namepsace). But this change seems to be unrelated here. > > Actually the goal of this set of changes was to get rid of any dependence on the > background page, therefore we had to get rid of uses of ext.pages or ext.Page as > part of these changes. It is definitely not unrelated. > > > Can you please submit a separate patch, that is replacing ext.pages.open with > > chrome.tabs.create, not only here, but everywhere? > > I can file an issue for the general case of getting rid of ext.pages and others > throughout the codebase. Sure, but there seems to be much more to do than done in this patch in order to get rid of that dependency. So changing this call here seemed unrelated to the other changes here. It's not too important, but if you split it out we could get rid of other usage of ext.tabs.open() in the same go. https://codereview.adblockplus.org/29532767/diff/29549882/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29549882/popup.js#newcode29 popup.js:29: function createPageObject(tab) On 2017/09/21 06:11:16, Manish Jethani wrote: > By the way, there is a "filters.isPageWhitelisted" in lib/whitelisting.js, it is > not used anywhere right now (?) Well spotted. This is relic from Safari support. I will remove it. https://codereview.adblockplus.org/29532767/diff/29550811/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29550811/popup.js#newcode64 popup.js:64: let onMessage = (message, sender) => On 2017/09/21 04:54:00, Manish Jethani wrote: > I meant to ask, is it preferred to use "function foo() { ... }" or "let foo = () > => { ... }" in such instances? I find it better to use the latter, especially > since the function doesn't need to have its own "this". It often also leads to > shorter code, sometimes one-liners. I prefer named functions over arrow functions that are assigned to a variable. I think it is cleaner (and easier to read), since this is what named function syntax is for, while explicitly defining a local variable just to assign an anonymous function right during initialization seems unnecessary. Also the two equal signs makes it harder to read such lines (for me at least). https://codereview.adblockplus.org/29532767/diff/29551564/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29551564/popup.js#newcode24 popup.js:24: function getPref(key, callback) On 2017/09/21 08:06:25, Manish Jethani wrote: > By the way, since we moved getPref and togglePref into popup.js, now we're > getting a linter error in stats.js for these functions as they are not declared > global. > > Any thoughts on how to deal with this? Where accessing globals from other scripts cannot be avoided, it is possible to annotate these at the top of the script like this: /* global getPref, toggelPref */ However, we should avoid cases like this as much as possible. And FWIW, I don't see why the code for the popup UI is spread across 3 scripts in the first place. So please consider merging these files, but this should probably happen in a separate review. https://codereview.adblockplus.org/29532767/diff/29551564/popup.js#newcode41 popup.js:41: let pageId = tab.id; Why do we have to chache tab.id into a local variable here? https://codereview.adblockplus.org/29532767/diff/29551564/popup.js#newcode45 popup.js:45: let handlePageReady = () => I'm not sure if it's worth to move these two trivial lines into a function. I feel that the code is slightly easier to follow with this code inlined. https://codereview.adblockplus.org/29532767/diff/29551564/popup.js#newcode74 popup.js:74: tab = tabs[0] && {id: tabs[0].id, url: tabs[0].url}; On 2017/09/21 08:02:37, Manish Jethani wrote: > I don't like to hold on to an object passed in by the browser, as it may cause > memory leaks, especially when it represents a browser tab, so I'm creating a new > "tab" object with just the id and the url (both primitive types). Alternatively, you could use two global variables, `tabUrl` and `tabId`. As a side effect, this would also simplify checks like below, instead for `tab && tab.url`, you could just check for `tabUrl` there then.
Patch Set 16: Rebase on next https://codereview.adblockplus.org/29532767/diff/29549882/ext/popup.js File ext/popup.js (left): https://codereview.adblockplus.org/29532767/diff/29549882/ext/popup.js#oldcode13 ext/popup.js:13: }; On 2017/09/21 22:57:16, Sebastian Noack wrote: > On 2017/09/21 06:11:16, Manish Jethani wrote: > > On 2017/09/20 18:57:02, Sebastian Noack wrote: > > > It seems you need to rebase. There is no closePopup() in here anymore, as of > > the > > > current "next" bookmark. > > > > OK, I'll rebase on master instead once next has been merged, which I'm > guessing > > is soon. > > Why not rebasing against "next" now? If the patch applies on "next", it will > also apply on "master", once "next" has been merged. But it makes it > unnecessarily difficult to review changes based on outdated code, and if a merge > conflict needs to be resolved later, I'd have to review the changes again. Done.
Patch Set 17: Fix lint error and simplify code https://codereview.adblockplus.org/29532767/diff/29549882/notification.js File notification.js (right): https://codereview.adblockplus.org/29532767/diff/29549882/notification.js#new... notification.js:85: chrome.tabs.create({url: link.href}); On 2017/09/21 22:57:16, Sebastian Noack wrote: > On 2017/09/21 06:11:16, Manish Jethani wrote: > > On 2017/09/20 18:57:02, Sebastian Noack wrote: > > > I'm all for replacing usage of ext.* wit chrome.* (eventually getting rid > > > completely of the ext namepsace). But this change seems to be unrelated > here. > > > > Actually the goal of this set of changes was to get rid of any dependence on > the > > background page, therefore we had to get rid of uses of ext.pages or ext.Page > as > > part of these changes. It is definitely not unrelated. > > > > > Can you please submit a separate patch, that is replacing ext.pages.open > with > > > chrome.tabs.create, not only here, but everywhere? > > > > I can file an issue for the general case of getting rid of ext.pages and > others > > throughout the codebase. > > Sure, but there seems to be much more to do than done in this patch in order to > get rid of that dependency. So changing this call here seemed unrelated to the > other changes here. It's not too important, but if you split it out we could get > rid of other usage of ext.tabs.open() in the same go. OK, I thought we were going to make all the changes to remove the dependence on the background page in one commit, and one code review, but do it incrementally. Is this still true? If we've decided to split it into multiple commits and code reviews then it makes sense, in that case I'll move these changes over to another code review. https://codereview.adblockplus.org/29532767/diff/29550811/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29550811/popup.js#newcode64 popup.js:64: let onMessage = (message, sender) => On 2017/09/21 22:57:17, Sebastian Noack wrote: > On 2017/09/21 04:54:00, Manish Jethani wrote: > > I meant to ask, is it preferred to use "function foo() { ... }" or "let foo = > () > > => { ... }" in such instances? I find it better to use the latter, especially > > since the function doesn't need to have its own "this". It often also leads to > > shorter code, sometimes one-liners. > > I prefer named functions over arrow functions that are assigned to a variable. I > think it is cleaner (and easier to read), since this is what named function > syntax is for, while explicitly defining a local variable just to assign an > anonymous function right during initialization seems unnecessary. Also the two > equal signs makes it harder to read such lines (for me at least). Changed now. https://codereview.adblockplus.org/29532767/diff/29551564/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29551564/popup.js#newcode24 popup.js:24: function getPref(key, callback) On 2017/09/21 22:57:17, Sebastian Noack wrote: > On 2017/09/21 08:06:25, Manish Jethani wrote: > > By the way, since we moved getPref and togglePref into popup.js, now we're > > getting a linter error in stats.js for these functions as they are not > declared > > global. > > > > Any thoughts on how to deal with this? > > Where accessing globals from other scripts cannot be avoided, it is possible to > annotate these at the top of the script like this: > > /* global getPref, toggelPref */ Done. > However, we should avoid cases like this as much as possible. And FWIW, I don't > see why the code for the popup UI is spread across 3 scripts in the first place. > So please consider merging these files, but this should probably happen in a > separate review. OK, I'll look into this separately. https://codereview.adblockplus.org/29532767/diff/29551564/popup.js#newcode41 popup.js:41: let pageId = tab.id; On 2017/09/21 22:57:18, Sebastian Noack wrote: > Why do we have to chache tab.id into a local variable here? It was not necessary, removed. https://codereview.adblockplus.org/29532767/diff/29551564/popup.js#newcode45 popup.js:45: let handlePageReady = () => On 2017/09/21 22:57:17, Sebastian Noack wrote: > I'm not sure if it's worth to move these two trivial lines into a function. I > feel that the code is slightly easier to follow with this code inlined. Done. https://codereview.adblockplus.org/29532767/diff/29551564/popup.js#newcode74 popup.js:74: tab = tabs[0] && {id: tabs[0].id, url: tabs[0].url}; On 2017/09/21 22:57:18, Sebastian Noack wrote: > On 2017/09/21 08:02:37, Manish Jethani wrote: > > I don't like to hold on to an object passed in by the browser, as it may cause > > memory leaks, especially when it represents a browser tab, so I'm creating a > new > > "tab" object with just the id and the url (both primitive types). > > Alternatively, you could use two global variables, `tabUrl` and `tabId`. As a > side effect, this would also simplify checks like below, instead for `tab && > tab.url`, you could just check for `tabUrl` there then. We could do that, but we're using the tab object in isPageWhitelisted and toggleEnabled, so then we'd have to recreate that object locally. https://codereview.adblockplus.org/29532767/diff/29554598/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29554598/popup.js#newcode74 popup.js:74: if (tabs.length > 0) I thought this reads better.
https://codereview.adblockplus.org/29532767/diff/29549882/notification.js File notification.js (right): https://codereview.adblockplus.org/29532767/diff/29549882/notification.js#new... notification.js:85: chrome.tabs.create({url: link.href}); On 2017/09/24 22:37:23, Manish Jethani wrote: > On 2017/09/21 22:57:16, Sebastian Noack wrote: > > On 2017/09/21 06:11:16, Manish Jethani wrote: > > > On 2017/09/20 18:57:02, Sebastian Noack wrote: > > > > I'm all for replacing usage of ext.* wit chrome.* (eventually getting rid > > > > completely of the ext namepsace). But this change seems to be unrelated > > here. > > > > > > Actually the goal of this set of changes was to get rid of any dependence on > > the > > > background page, therefore we had to get rid of uses of ext.pages or > ext.Page > > as > > > part of these changes. It is definitely not unrelated. > > > > > > > Can you please submit a separate patch, that is replacing ext.pages.open > > with > > > > chrome.tabs.create, not only here, but everywhere? > > > > > > I can file an issue for the general case of getting rid of ext.pages and > > others > > > throughout the codebase. > > > > Sure, but there seems to be much more to do than done in this patch in order > to > > get rid of that dependency. So changing this call here seemed unrelated to the > > other changes here. It's not too important, but if you split it out we could > get > > rid of other usage of ext.tabs.open() in the same go. > > OK, I thought we were going to make all the changes to remove the dependence on > the background page in one commit, and one code review, but do it incrementally. > Is this still true? If we've decided to split it into multiple commits and code > reviews then it makes sense, in that case I'll move these changes over to > another code review. Well, Wladimir wasn't happy with that approach, and at the current point I don't care if we land this effort in progressive steps. However, we should break it down in logically contained patches at least, and the change here seems slightly too much unrelated of the messaging changes, in particular if we could get rid of other usage of chrome.tabs.create() in the same go. Other than that, the changes look good to me now. https://codereview.adblockplus.org/29532767/diff/29551564/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29551564/popup.js#newcode74 popup.js:74: tab = tabs[0] && {id: tabs[0].id, url: tabs[0].url}; On 2017/09/24 22:37:27, Manish Jethani wrote: > On 2017/09/21 22:57:18, Sebastian Noack wrote: > > On 2017/09/21 08:02:37, Manish Jethani wrote: > > > I don't like to hold on to an object passed in by the browser, as it may > cause > > > memory leaks, especially when it represents a browser tab, so I'm creating a > > new > > > "tab" object with just the id and the url (both primitive types). > > > > Alternatively, you could use two global variables, `tabUrl` and `tabId`. As a > > side effect, this would also simplify checks like below, instead for `tab && > > tab.url`, you could just check for `tabUrl` there then. > > We could do that, but we're using the tab object in isPageWhitelisted and > toggleEnabled, so then we'd have to recreate that object locally. I see, I don't have a strong opinion then.
Patch Set 18: Revert to using ext.pages.open https://codereview.adblockplus.org/29532767/diff/29549882/notification.js File notification.js (right): https://codereview.adblockplus.org/29532767/diff/29549882/notification.js#new... notification.js:85: chrome.tabs.create({url: link.href}); On 2017/09/25 17:50:51, Sebastian Noack wrote: > On 2017/09/24 22:37:23, Manish Jethani wrote: > > On 2017/09/21 22:57:16, Sebastian Noack wrote: > > > On 2017/09/21 06:11:16, Manish Jethani wrote: > > > > On 2017/09/20 18:57:02, Sebastian Noack wrote: > > > > > I'm all for replacing usage of ext.* wit chrome.* (eventually getting > rid > > > > > completely of the ext namepsace). But this change seems to be unrelated > > > here. > > > > > > > > Actually the goal of this set of changes was to get rid of any dependence > on > > > the > > > > background page, therefore we had to get rid of uses of ext.pages or > > ext.Page > > > as > > > > part of these changes. It is definitely not unrelated. > > > > > > > > > Can you please submit a separate patch, that is replacing ext.pages.open > > > with > > > > > chrome.tabs.create, not only here, but everywhere? > > > > > > > > I can file an issue for the general case of getting rid of ext.pages and > > > others > > > > throughout the codebase. > > > > > > Sure, but there seems to be much more to do than done in this patch in order > > to > > > get rid of that dependency. So changing this call here seemed unrelated to > the > > > other changes here. It's not too important, but if you split it out we could > > get > > > rid of other usage of ext.tabs.open() in the same go. > > > > OK, I thought we were going to make all the changes to remove the dependence > on > > the background page in one commit, and one code review, but do it > incrementally. > > Is this still true? If we've decided to split it into multiple commits and > code > > reviews then it makes sense, in that case I'll move these changes over to > > another code review. > > Well, Wladimir wasn't happy with that approach, and at the current point I don't > care if we land this effort in progressive steps. However, we should break it > down in logically contained patches at least, and the change here seems slightly > too much unrelated of the messaging changes, in particular if we could get rid > of other usage of chrome.tabs.create() in the same go. OK, backed out this change. I like doing it in separate commits as well FWIW, makes it easier overall.
Patch Set 19: Move require to notification.js https://codereview.adblockplus.org/29532767/diff/29556789/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29556789/popup.js#newcode20 popup.js:20: const {require} = ext.backgroundPage.getWindow(); popup.js doesn't need this now.
LGTM
Patch Set 20: Revert one more instance of ext.pages.open https://codereview.adblockplus.org/29532767/diff/29557569/stats.js File stats.js (right): https://codereview.adblockplus.org/29532767/diff/29557569/stats.js#newcode136 stats.js:136: ext.pages.open(createShareLink(ev.target.dataset.social, blockedTotal)); Sorry, I forgot this one.
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
