Issue 7257 - Throttle badge updates

lib/stats.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 /** @module stats */
 
 "use strict";
 
 const {Prefs} = require("./prefs");
 const {BlockingFilter} = require("../adblockpluscore/lib/filterClasses");
 const {filterNotifier} = require("../adblockpluscore/lib/filterNotifier");
 const {port} = require("./messaging");
 
 const badgeColor = "#646464";
-
-// 4 fps.
 const badgeRefreshRate = 4;

[Manish Jethani, 2019/02/03 13:20:21]

I found this to be a reasonable refresh rate, YMMV.

 
 let blockedPerPage = new ext.PageMap();
 
 let getBlockedPerPage =
 /**
  * Gets the number of requests blocked on the given page.
  *
  * @param  {Page} page
  * @return {Number}
  */
 exports.getBlockedPerPage = page => blockedPerPage.get(page) || 0;
 
-// The ID of the focused window. If all windows lose focus, this is the ID of
-// the last focused window.
-let focusedWindowId = -1;
+let activeTabIds = new Set();
+let activeTabIdByWindowId = new Map();
 
-// The ID of the active tab in the focused window. This can be unknown (-1) on
-// startup and (briefly) when the focus shifts to a different window.
-let activeTabId = -1;
+let badgeUpdateScheduled = false;
 
-let badgeUpdateScheduler = new ext.Scheduler();
-
-function updateBadgeNow()
+function updateBadge(tabId)
 {
-  if (activeTabId == -1)
+  if (!Prefs.show_statsinicon)
     return;
 
-  let page = new ext.Page({id: activeTabId});
-  let blockedCount = blockedPerPage.get(page);
+  for (let id of (typeof tabId == "undefined" ? activeTabIds : [tabId]))
+  {
+    let page = new ext.Page({id});
+    let blockedCount = blockedPerPage.get(page);
 
-  page.browserAction.setBadge(blockedCount && {
-    color: badgeColor,
-    number: blockedCount
-  });
-}
-
-function updateBadge(tabId = -1)
-{
-  if (!badgeUpdateScheduler.scheduled &&

[Manish Jethani, 2019/02/03 13:20:21]

The conditions are ordered from most likely to least likely.

-      (tabId == activeTabId || tabId == -1) && activeTabId != -1 &&
-      Prefs.show_statsinicon)
-  {
-    // Schedule an update.
-    badgeUpdateScheduler.schedule(1000 / badgeRefreshRate, updateBadgeNow);
+    page.browserAction.setBadge(blockedCount && {
+      color: badgeColor,
+      number: blockedCount
+    });
   }
 }
 
-function findAndUpdateActiveTab()
+function scheduleBadgeUpdate(tabId)
 {
-  browser.tabs.query({active: true, lastFocusedWindow: true}, ([tab]) =>
+  if (!badgeUpdateScheduled && Prefs.show_statsinicon &&
+      (typeof tabId == "undefined" || activeTabIds.has(tabId)))
   {
-    if (tab && activeTabId != tab.id)
-    {
-      activeTabId = tab.id;
-      focusedWindowId = tab.windowId;
-
-      updateBadge();
-    }
-  });
+    setTimeout(() => { badgeUpdateScheduled = false; updateBadge(); },
+               1000 / badgeRefreshRate);
+    badgeUpdateScheduled = true;
+  }
 }
 
 // Once nagivation for the tab has been committed to (e.g. it's no longer
 // being prerendered) we clear its badge, or if some requests were already
 // blocked beforehand we display those on the badge now.
 browser.webNavigation.onCommitted.addListener(details =>
 {
   if (details.frameId == 0)
     updateBadge(details.tabId);
 });
 
 filterNotifier.on("filter.hitCount", (filter, newValue, oldValue, tabIds) =>
 {
   if (!(filter instanceof BlockingFilter))
     return;
 
   for (let tabId of tabIds)
   {
     let page = new ext.Page({id: tabId});
     let blocked = blockedPerPage.get(page) || 0;
 
     blockedPerPage.set(page, ++blocked);
-    updateBadge(tabId);
+    scheduleBadgeUpdate(tabId);
   }
 
   Prefs.blocked_total++;
 });
 
 Prefs.on("show_statsinicon", () =>
 {
   browser.tabs.query({}, tabs =>
   {
     for (let tab of tabs)
     {
       let page = new ext.Page(tab);
 
       if (Prefs.show_statsinicon)
         updateBadge(tab.id);
       else
         page.browserAction.setBadge(null);
     }
   });
 });
 
 port.on("stats.getBlockedPerPage",
         message => getBlockedPerPage(new ext.Page(message.tab)));
 
+browser.tabs.query({active: true}, tabs =>
+{
+  for (let tab of tabs)
+  {
+    activeTabIds.add(tab.id);
+    activeTabIdByWindowId.set(tab.windowId, tab.id);
+  }
+
+  scheduleBadgeUpdate();
+});
+
 browser.tabs.onActivated.addListener(tab =>
 {
-  if (tab.windowId == focusedWindowId && activeTabId != tab.tabId)

[Manish Jethani, 2019/02/03 13:20:21]

We care about only the focused window, not other windows.

[Sebastian Noack, 2019/02/03 23:18:03]

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?

[Manish Jethani, 2019/02/04 05:33:04]

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.
Yes, it does make a significant difference, see the description in the Trac
ticket [1]

[1]: https://issues.adblockplus.org/ticket/7257
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.

[Sebastian Noack, 2019/02/04 06:32:04]

I think my case is at least not any less common than having another browser
window in the background.

[Manish Jethani, 2019/02/04 06:57:17]

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. 

-  {
-    activeTabId = tab.tabId;
-    updateBadge();
-  }
+  let lastActiveTabId = activeTabIdByWindowId.get(tab.windowId);
+  if (typeof lastActiveTabId != "undefined")
+    activeTabIds.delete(lastActiveTabId);
+
+  activeTabIds.add(tab.tabId);
+  activeTabIdByWindowId.set(tab.windowId, tab.tabId);
+
+  scheduleBadgeUpdate();
 });
 
 if ("windows" in browser)
 {
-  browser.windows.onFocusChanged.addListener(windowId =>
+  browser.windows.onRemoved.addListener(windowId =>
   {
-    if (windowId == browser.windows.WINDOW_ID_NONE)

[Manish Jethani, 2019/02/03 13:20:21]

Make sure focusedWindowId always points to a valid window. If no window is
focused, it's the last focused window.

-      return;
-
-    focusedWindowId = windowId;
-    activeTabId = -1;
-
-    findAndUpdateActiveTab();
+    activeTabIds.delete(activeTabIdByWindowId.get(windowId));
+    activeTabIdByWindowId.delete(windowId);
   });
 }
-
-findAndUpdateActiveTab();