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.

[Sebastian Noack, 2019/02/04 07:16:41]

Nit: This comment seems somewhat redundant. Perhaps make "fps" part of the
variable name if you think it isn't clear otherwise.

[Manish Jethani, 2019/02/04 07:36:14]

I think it's fine to just remove the comment.

Done.

 const badgeRefreshRate = 4;
 
 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;
 
 let activeTabIds = new Set();
 let activeTabIdByWindowId = new Map();
 
 let badgeUpdateScheduled = false;
 
-function updateBadge(tabId = -1)

[Sebastian Noack, 2019/02/04 07:16:41]

Nit: What would you think about not setting a default value, but checking for
undefined below?

[Manish Jethani, 2019/02/04 07:36:14]

Works for me, done.

+function updateBadge(tabId)
 {
   if (!Prefs.show_statsinicon)
     return;

[Sebastian Noack, 2019/02/04 07:16:41]

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?

[Manish Jethani, 2019/02/04 07:36:14]

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.

 
-  for (let id of (tabId == -1 ? activeTabIds : [tabId]))
+  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 scheduleBadgeUpdate(tabId = -1)
+function scheduleBadgeUpdate(tabId)
 {
   if (!badgeUpdateScheduled && Prefs.show_statsinicon &&
-      (tabId == -1 || activeTabIds.has(tabId)))
+      (typeof tabId == "undefined" || activeTabIds.has(tabId)))
   {
     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.
@@ skipping 63 matching lines @@
 });
 
 if ("windows" in browser)
 {
   browser.windows.onRemoved.addListener(windowId =>
   {
     activeTabIds.delete(activeTabIdByWindowId.get(windowId));
     activeTabIdByWindowId.delete(windowId);
   });
 }