Issue 2853 - Settings changes are sometimes not saved if the user quits the app

adblockplus/Api.jsm

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 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
@@ skipping 19 matching lines @@
 
 function require(module)
 {
   let result = {};
   result.wrappedJSObject = result;
   Services.obs.notifyObservers(result, "adblockplus-require", module);
   return result.exports;
 }
 
 let {Filter} = require("filterClasses");
+let {FilterNotifier} = require("filterNotifier");
 let {FilterStorage} = require("filterStorage");
 let {defaultMatcher} = require("matcher");
 let {Prefs} = require("prefs");
 let {Subscription, SpecialSubscription, RegularSubscription, DownloadableSubscription, ExternalSubscription} = require("subscriptionClasses");
 let {Synchronizer} = require("synchronizer");
 let {UI} = require("ui");
 
+let shouldSaveFiltersPref = "should_save_filters";

[Felix Dahlke, 2016/12/13 11:25:09]

I think this name is so generic that it could conflict with other prefs we set.
How about `abb_should_save_filters`? I would probably prefer a more technical
term even, maybe `abb_filters_dirty` or `abb_filters_need_saving`?

+
+function initListeners()

[Felix Dahlke, 2016/12/13 11:25:09]

Nit: Maybe call this `initFilterListeners` or something along those lines, since
there are also some other listeners being created here, but not in this
function?

+{
+  FilterNotifier.on("filter.added", onFiltersChanged);
+  FilterNotifier.on("filter.removed", onFiltersChanged);
+  FilterNotifier.on("subscription.added", onFiltersChanged);
+  FilterNotifier.on("subscription.removed", onFiltersChanged);
+  FilterNotifier.on("save", onSave);
+}
+
+function onFiltersChanged()
+{
+  setBoolPref(shouldSaveFiltersPref, true);
+}
+
+function onSave()

[Felix Dahlke, 2016/12/13 11:25:09]

Nit: Shouldn't this be called `onFiltersSave` for consistency? Just a thought,
wouldn't insist.

+{
+  setBoolPref(shouldSaveFiltersPref, false);
+}
+
+function getBoolPref(name)

[Felix Dahlke, 2016/12/13 11:25:09]

It's a bit of a shame that we can apparently not use our own Prefs object for
this. Duplicating that logic is not great. I guess we can live with that for
now, but could you create a follow-up issue for resolving this, and link to it
here?

My first hunch would be to modify `lib/prefs.json` in the Firefox extension
during the build (i.e. in `adblockplus/build.py` in the ABB repository).

+{
+  let branch = getPrefsBranch();
+  try
+  {
+    return branch.getBoolPref(name);
+  }
+  catch (e)
+  {
+    return null;
+  }
+}
+
+function setBoolPref(name, value)
+{
+  let branch = getPrefsBranch();
+  branch.setBoolPref(name, value);
+  Services.prefs.savePrefFile(null);
+}
+
+function getPrefsBranch()
+{
+  let {addonRoot, addonName} = require("info");
+  let branchName = "extensions." + addonName + ".";
+  return Services.prefs.getBranch(branchName);
+}
+
 function getWhitelistingFilter(url)
 {
   let uriObject = Services.io.newURI(url, null, null);
   try
   {
     return defaultMatcher.whitelist.matchesAny(
       uriObject.spec, "DOCUMENT", uriObject.host, false, null);
   }
   catch (e)
   {
     return null;
   }
 }
 
 var AdblockPlusApi =
 {
   get filtersLoaded()
   {
     return !FilterStorage._loading;
   },
+  get requestsSaved()

[Felix Dahlke, 2016/12/13 11:25:09]

Naming: Do we actually "save requests" here? Seems we rather "process requests"
or "save filters". So maybe we could call this `filtersSaved`?
`requestsProcessed` would be another option, but I feel this could be confusing,
since we don't actually check whether there are requests being process right
now, we just check whether the filters have been saved.

+  {
+    return !getBoolPref(shouldSaveFiltersPref) && !FilterStorage._saving && !FilterStorage._needsSave;
+  },
   get acceptableAdsEnabled()
   {
     return FilterStorage.subscriptions.some(
       (subscription) => subscription.url == Prefs.subscriptions_exceptionsurl);
   },
   set acceptableAdsEnabled(acceptableAdsEnabled)
   {
     if (acceptableAdsEnabled != AdblockPlusApi.acceptableAdsEnabled)
       UI.toggleAcceptableAds();
   },
@@ skipping 78 matching lines @@
       {
         FilterStorage.removeFilter(filter);
         if (filter.subscriptions.length)
           filter.disabled = true;
         filter = getWhitelistingFilter(url);
       }
     }
   },
   initCommunication: function()
   {
+    initListeners();
+
     Messaging.addListener((function(data)
     {
       if (!data)
         return {"success": false, "error": "malformed request"};
 
       if (data["action"] == "getFiltersLoaded")
         return {"success": true, "value": this.filtersLoaded};
 
       if (!this.filtersLoaded)
         return {"success": false, "error": "filters not loaded"};
 
       switch (data["action"])
       {
+        case "getRequestsSaved":
+          return {"success": true, "value": this.requestsSaved};
         case "getAcceptableAdsEnabled":
           return {"success": true, "value": this.acceptableAdsEnabled};
         case "setAcceptableAdsEnabled":
           if ("enable" in data)
           {
             this.acceptableAdsEnabled = !!data["enable"];
             return {"success": true};
           }
           break;
         case "getSubscriptionsXml":
@@ skipping 36 matching lines @@
           {
             this.whitelistSite(data["url"], data["whitelisted"]);
             return {"success": true};
           }
           break;
       }
       return {"success": false, "error": "malformed request"};
     }).bind(this), "AdblockPlus:Api");
   }
 };