Issue 29996582: Issue 7257 - Throttle badge updates (Closed)
|
Created:
Feb. 2, 2019, 7:02 p.m. by Manish Jethani Modified:
Feb. 4, 2019, 8:20 a.m. Reviewers:
Sebastian Noack Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
Patch Set 1 : #
Total comments: 12
Patch Set 2 : Move Scheduler to lib/stats.js #Patch Set 3 : Alternative implementation, update all active tabs #
Total comments: 6
Patch Set 4 : Check for undefined #MessagesTotal messages: 9
Patch Set 1 https://codereview.adblockplus.org/29996582/diff/29997558/lib/stats.js File lib/stats.js (right): https://codereview.adblockplus.org/29996582/diff/29997558/lib/stats.js#newcode30 lib/stats.js:30: const badgeRefreshRate = 4; I found this to be a reasonable refresh rate, YMMV. https://codereview.adblockplus.org/29996582/diff/29997558/lib/stats.js#newcode69 lib/stats.js:69: if (!badgeUpdateScheduler.scheduled && The conditions are ordered from most likely to least likely. https://codereview.adblockplus.org/29996582/diff/29997558/lib/stats.js#newcod... lib/stats.js:139: if (tab.windowId == focusedWindowId && activeTabId != tab.tabId) We care about only the focused window, not other windows. https://codereview.adblockplus.org/29996582/diff/29997558/lib/stats.js#newcod... lib/stats.js:150: if (windowId == browser.windows.WINDOW_ID_NONE) Make sure focusedWindowId always points to a valid window. If no window is focused, it's the last focused window.
https://codereview.adblockplus.org/29996582/diff/29997558/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29996582/diff/29997558/ext/background.js#n... ext/background.js:548: }; Please don't add new stuff to ext.*, we eventually want to get rid of it. https://codereview.adblockplus.org/29996582/diff/29997558/lib/stats.js File lib/stats.js (right): https://codereview.adblockplus.org/29996582/diff/29997558/lib/stats.js#newcod... lib/stats.js:139: if (tab.windowId == focusedWindowId && activeTabId != tab.tabId) On 2019/02/03 13:20:21, Manish Jethani wrote: > We care about only the focused window, not other windows. But it's possible that several windows showing the browser action are visible (e.g. I quite often split my screen with one browser window on each side). As for only updating the active tab, does that add much performance benefits? Does updating the badge for an inactive tab even cause anything to be drawn?
Patch Set 2: Move Scheduler to lib/stats.js https://codereview.adblockplus.org/29996582/diff/29997558/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29996582/diff/29997558/ext/background.js#n... ext/background.js:548: }; On 2019/02/03 23:18:03, Sebastian Noack wrote: > Please don't add new stuff to ext.*, we eventually want to get rid of it. I've moved it into lib/stats.js for now, although I think (1) it should be in its own file, because I see this pattern in lib/indexedDBBackup.js and lib/requestBlocker.js, and (2) it should be an ES2015 class. https://codereview.adblockplus.org/29996582/diff/29997558/lib/stats.js File lib/stats.js (right): https://codereview.adblockplus.org/29996582/diff/29997558/lib/stats.js#newcod... lib/stats.js:139: if (tab.windowId == focusedWindowId && activeTabId != tab.tabId) On 2019/02/03 23:18:03, Sebastian Noack wrote: > On 2019/02/03 13:20:21, Manish Jethani wrote: > > We care about only the focused window, not other windows. > > But it's possible that several windows showing the browser action are visible > (e.g. I quite often split my screen with one browser window on each side). Yes, that's possible, and in that case only the number in the focused window will get updated. How common is it for people to (1) split their screens and (2) care about every window's toolbar showing the most up-to-date number of ads blocked? I doubt that there are too many people who would care about this. The moment you focus on the window of course the number gets updated immediately. > As for only updating the active tab, does that add much performance benefits? Yes, it does make a significant difference, see the description in the Trac ticket [1] [1]: https://issues.adblockplus.org/ticket/7257 > Does updating the badge for an inactive tab even cause anything to be drawn? I hope not, especially after Trac #7253, but the API call and the processing that comes with it is also significant. It starts to show in performance benchmarks. It is also common for users to click "Open Link in New Tab" (Command + click on macOS) while they are reading an article; I often open 4-5 links from an article in a new tab (also often from Google search results) while I continue to read. All that processing can be avoided.
https://codereview.adblockplus.org/29996582/diff/29997558/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29996582/diff/29997558/ext/background.js#n... ext/background.js:548: }; On 2019/02/04 05:33:03, Manish Jethani wrote: > On 2019/02/03 23:18:03, Sebastian Noack wrote: > > Please don't add new stuff to ext.*, we eventually want to get rid of it. > > I've moved it into lib/stats.js for now, although I think (1) it should be in > its own file, because I see this pattern in lib/indexedDBBackup.js and > lib/requestBlocker.js, and (2) it should be an ES2015 class. Well, IMO this pattern isn't worth to be abstracted in a class in the first place. I'd ust use setTimeout() directly as we do in those other places. https://codereview.adblockplus.org/29996582/diff/29997558/lib/stats.js File lib/stats.js (right): https://codereview.adblockplus.org/29996582/diff/29997558/lib/stats.js#newcod... lib/stats.js:139: if (tab.windowId == focusedWindowId && activeTabId != tab.tabId) On 2019/02/04 05:33:04, Manish Jethani wrote: > On 2019/02/03 23:18:03, Sebastian Noack wrote: > > On 2019/02/03 13:20:21, Manish Jethani wrote: > > > We care about only the focused window, not other windows. > > > > But it's possible that several windows showing the browser action are visible > > (e.g. I quite often split my screen with one browser window on each side). > > Yes, that's possible, and in that case only the number in the focused window > will get updated. How common is it for people to (1) split their screens and (2) > care about every window's toolbar showing the most up-to-date number of ads > blocked? I doubt that there are too many people who would care about this. > > The moment you focus on the window of course the number gets updated > immediately. I think my case is at least not any less common than having another browser window in the background.
Patch Set 3: Alternative implementation, update all active tabs This is an alternative implementation that also updates active tabs in unfocused windows. You might say it's even simpler. But the downside is that it updates _all_ active tabs, even if there is a filter hit in only one of the (possibly many) active tabs. It is still an improvement over what we have now. I personally think that people have way more inactive tabs than unfocused windows so overall this is still beneficial. But I still prefer the previous version. https://codereview.adblockplus.org/29996582/diff/29997558/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29996582/diff/29997558/ext/background.js#n... ext/background.js:548: }; On 2019/02/04 06:32:04, Sebastian Noack wrote: > On 2019/02/04 05:33:03, Manish Jethani wrote: > > On 2019/02/03 23:18:03, Sebastian Noack wrote: > > > Please don't add new stuff to ext.*, we eventually want to get rid of it. > > > > I've moved it into lib/stats.js for now, although I think (1) it should be in > > its own file, because I see this pattern in lib/indexedDBBackup.js and > > lib/requestBlocker.js, and (2) it should be an ES2015 class. > > Well, IMO this pattern isn't worth to be abstracted in a class in the first > place. I'd ust use setTimeout() directly as we do in those other places. Acknowledged. https://codereview.adblockplus.org/29996582/diff/29997558/lib/stats.js File lib/stats.js (right): https://codereview.adblockplus.org/29996582/diff/29997558/lib/stats.js#newcod... lib/stats.js:139: if (tab.windowId == focusedWindowId && activeTabId != tab.tabId) On 2019/02/04 06:32:04, Sebastian Noack wrote: > On 2019/02/04 05:33:04, Manish Jethani wrote: > > On 2019/02/03 23:18:03, Sebastian Noack wrote: > > > On 2019/02/03 13:20:21, Manish Jethani wrote: > > > > We care about only the focused window, not other windows. > > > > > > But it's possible that several windows showing the browser action are > visible > > > (e.g. I quite often split my screen with one browser window on each side). > > > > Yes, that's possible, and in that case only the number in the focused window > > will get updated. How common is it for people to (1) split their screens and > (2) > > care about every window's toolbar showing the most up-to-date number of ads > > blocked? I doubt that there are too many people who would care about this. > > > > The moment you focus on the window of course the number gets updated > > immediately. > > I think my case is at least not any less common than having another browser > window in the background. Yes, but those are two different things. Windows in the background are almost entirely obscured. You can't even see them, much less pay attention to the incrementing counter.
https://codereview.adblockplus.org/29996582/diff/29998557/lib/stats.js File lib/stats.js (right): https://codereview.adblockplus.org/29996582/diff/29998557/lib/stats.js#newcode29 lib/stats.js:29: // 4 fps. Nit: This comment seems somewhat redundant. Perhaps make "fps" part of the variable name if you think it isn't clear otherwise. https://codereview.adblockplus.org/29996582/diff/29998557/lib/stats.js#newcode48 lib/stats.js:48: function updateBadge(tabId = -1) Nit: What would you think about not setting a default value, but checking for undefined below? https://codereview.adblockplus.org/29996582/diff/29998557/lib/stats.js#newcode51 lib/stats.js:51: return; The only purpose of the check here seems to be avoiding a race condition when the preference is disabled? If so couldn't you just cancel the timeout instead?
Patch Set 4: Check for undefined https://codereview.adblockplus.org/29996582/diff/29998557/lib/stats.js File lib/stats.js (right): https://codereview.adblockplus.org/29996582/diff/29998557/lib/stats.js#newcode29 lib/stats.js:29: // 4 fps. On 2019/02/04 07:16:41, Sebastian Noack wrote: > Nit: This comment seems somewhat redundant. Perhaps make "fps" part of the > variable name if you think it isn't clear otherwise. I think it's fine to just remove the comment. Done. https://codereview.adblockplus.org/29996582/diff/29998557/lib/stats.js#newcode48 lib/stats.js:48: function updateBadge(tabId = -1) On 2019/02/04 07:16:41, Sebastian Noack wrote: > Nit: What would you think about not setting a default value, but checking for > undefined below? Works for me, done. https://codereview.adblockplus.org/29996582/diff/29998557/lib/stats.js#newcode51 lib/stats.js:51: return; On 2019/02/04 07:16:41, Sebastian Noack wrote: > The only purpose of the check here seems to be avoiding a race condition when > the preference is disabled? If so couldn't you just cancel the timeout instead? updateBadge is also called directly, not necessarily only from the timeout handler. For those cases we still need to check if the preference is enabled, as we were doing before.
LGTM 
|
