Issue 6775 - Work around filter data stored in IndexedDB getting lost on Microsoft Edge

lib/indexedDBBackup.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/>.
  */
 
 "use strict";
 
 const {FilterNotifier} = require("../adblockpluscore/lib/filterNotifier");
 const {FilterStorage} = require("../adblockpluscore/lib/filterStorage");
-const {DownloadableSubscription,
-  SpecialSubscription} =
+const {DownloadableSubscription, SpecialSubscription} =
   require("../adblockpluscore/lib/subscriptionClasses");
 
 const BACKUP_NAME = "file:indexedDB-backup";

[Sebastian Noack, 2018/08/31 18:07:05]

Nit: IMO it reads better with a blank line separating the constants from the
variables.

[geo, 2018/09/04 16:12:07]

Done.

-let MIN_WRITE_INTERVAL;

[Sebastian Noack, 2018/08/31 18:07:05]

Nit: Since this isn't really a constant I would spell it in lowercase.

[geo, 2018/09/04 16:12:07]

Done.

-let pendingUpdate;
-let isSaving;
-let lastBackup;
-let lastRun;
 
-function init(backupInterval = 60 * 1000)
+let backupDelay;
+let pendingBackup = false;
+
+function setBackupInterval(backupInterval = 60 * 1000)
 {
-  clearInterval(lastBackup);
-  pendingUpdate = false;
-  isSaving = false;
-  lastRun = false;
-  MIN_WRITE_INTERVAL = backupInterval;
+  backupDelay = backupInterval;
 }

[Sebastian Noack, 2018/08/31 18:07:04]

Could we have a setBackupInterval() function instead of an init() function,
which won't cancel any pending backup, but merely changes the interval for
future backups? If that works out, this might be less code, and seems a bit more
robust since the current implementation could potentially cause a backup to be
skipped if init() is called (again) during a pending backup.

[geo, 2018/09/04 16:12:07]

Done.

 
-init();
+setBackupInterval();
 
 function scheduleBackup()
 {
-  pendingUpdate = true;
-  if (!lastRun)
+  if (!pendingBackup)
   {
-    lastRun = performance.now();
-    pendingUpdate = false;
-    isSaving = true;
+    pendingBackup = true;
 
-    saveToStorage().then(() =>
-    {
-      isSaving = false;
-      lastRun = performance.now();
-      if (pendingUpdate)
-        scheduleBackup();
-    });
-  }
-  else
-  {
-    clearInterval(lastBackup);
-
-    lastBackup = setTimeout(() =>
-    {
-      if (!isSaving && (performance.now() - lastRun) >= MIN_WRITE_INTERVAL)
+    setTimeout(
+      () =>
       {
-        isSaving = true;
-        pendingUpdate = false;
-        lastRun = performance.now();
-        saveToStorage().then(() =>
-        {
-          isSaving = false;
-          lastRun = performance.now();
-          if (pendingUpdate)
-            scheduleBackup();
-        });
-      }
-    }, MIN_WRITE_INTERVAL - (performance.now() - lastRun));
+        saveToStorage();
+        pendingBackup = false;
+      },
+      backupDelay
+    );
   }

[Sebastian Noack, 2018/08/31 18:07:04]

Correct me if I'm wrong, but to me it seems the logic here could be simplified:

  let lastRun = -MIN_WRITE_INTERVAL;

  ...

  function scheduleBackup()
  {
    if (!lastBackup)
    {
      lastBackup = setTimeout(
        () =>
        {
          saveToStorage();
          lastBackup = null;
        },
        lastRun + MIN_WRITE_INTERVAL - performance.now()
      );
    }
  }

[geo, 2018/09/04 16:12:07]

Done.

 }
 
 function saveToStorage()
 {
-  return browser.storage.local.set({
+  browser.storage.local.set({
     [BACKUP_NAME]: {
       content: serialize(),
       lastModified: Date.now()
     }
   });
 }
 
 function serialize()
 {
   let buffer = [];
 
   for (let subscription of FilterStorage.subscriptions)
   {
     if (subscription instanceof SpecialSubscription)
     {
       subscription.serialize(buffer);
       buffer.push("[Subscription filters]");
       subscription.serializeFilters(buffer);
     }
     else if (subscription instanceof DownloadableSubscription)
     {
       let {homepage, title, url, disabled} = subscription;
 
       buffer.push(
         "[Subscription]",
         `homepage=${homepage}`,
         `title=${title}`,
         `url=${url}`,
-        `disabled=${disabled}`);

[Sebastian Noack, 2018/08/31 18:07:05]

Nit: We wrap argument lists always with either of the following flavors:

  buffer.push("[Subscription]",
              `homepage=${homepage}`,
              `title=${title}`,
              `url=${url}`,
              `disabled=${disabled}`);

  buffer.push(
    "[Subscription]",
    `homepage=${homepage}`,
    `title=${title}`,
    `url=${url}`,
    `disabled=${disabled}`
  );

[geo, 2018/09/04 16:12:07]

Done.

+        `disabled=${disabled}`
+      );
     }
   }
   return buffer;
 }
 
 function getBackupData()
 {
   return browser.storage.local.get(BACKUP_NAME).then(items =>
   {
     let entry = items[BACKUP_NAME];
@@ skipping 11 matching lines @@
 FilterNotifier.on("subscription.disabled", scheduleBackup);
 FilterNotifier.on("filter.added", scheduleBackup);
 FilterNotifier.on("filter.removed", scheduleBackup);
 FilterNotifier.on("filter.moved", scheduleBackup);
 FilterNotifier.on("filter.disabled", scheduleBackup);
 
 exports.IndexedDBBackup =
 {
   getBackupData,
   // Non-public API, just for tests.
-  init
+  setBackupInterval
 };