Add filtersLoaded and filterLoadListener to AdblockPlusApi

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 @@
   result.wrappedJSObject = result;
   Services.obs.notifyObservers(result, "adblockplus-require", module);
   return result.exports;
 }
 
 let {FilterNotifier} = require("filterNotifier");
 let {FilterStorage} = require("filterStorage");
 let {Prefs} = require("prefs");
 let {UI} = require("ui");
 
-let filtersLoaded = false;
-
 var AdblockPlusApi =
 {
   get filtersLoaded()
   {
-    return filtersLoaded;

[Wladimir Palant, 2015/03/20 16:38:20]

return FilterStorage._loading?

[Felix Dahlke, 2015/03/20 16:54:57]

What we want to know here is whether the filters have been loaded. How I see it,
_loading is false initially, true while filters are being loaded, then false
again. So we could return !FilterStorage._loading - assuming
FilterStorage.loadFromDisk is always called before this getter. Seems like
asking for trouble to me.

[Felix Dahlke, 2015/03/20 21:13:02]

Well actually, you're right. Api.jsm is not necessarily executed before
FilterStorage.loadFromDisk, so we're quite likely to miss "load" events in the
listener below... Compared to that, the risk of accessing filtersLoaded before
FilterStorage.loadFromDisk is first called seems negligible in comparison.

+    return !FilterStorage._loading;
   },
   filterLoadListener: null,
   get acceptableAdsEnabled()
   {
     return FilterStorage.subscriptions.some(
       (subscription) => subscription.url == Prefs.subscriptions_exceptionsurl);
   },
   set acceptableAdsEnabled(acceptableAdsEnabled)
   {
     if (acceptableAdsEnabled != AdblockPlusApi.acceptableAdsEnabled)
       UI.toggleAcceptableAds();
   }
 };
 
-FilterNotifier.addListener(function(action)
+FilterNotifier.addListener(function filterListener(action)

[René Jeschke, 2015/03/22 21:01:46]

Oh, and from my side: we do not even really need this listener, anyway. I really
only need 'get filtersLoaded()' above.

[Felix Dahlke, 2015/03/22 21:13:28]

Right, we _can't_ use it there, actually - interaction between Java and JS is
worse than I thought.

In the spirit of not adding unused code, I'll close this review. René can add
the filtersLoaded getter above (as it is there) to his patch.

 {
   if (action != "load")
     return;
 
-  filtersLoaded = true;

[Wladimir Palant, 2015/03/20 16:38:20]

This listener should be removed after the first call rather than staying in
memory and being called for each change.

[Felix Dahlke, 2015/03/20 16:54:57]

Done.

[Felix Dahlke, 2015/03/20 17:13:06]

Well, NOW it's done. Misunderstood you, unregistered the filter listener now.
I've reverted the part about setting filterLoadListener to null here. Don't have
a strong opinion on this, but it seems like something the client should do.

-  if ("filterLoadListener" in AdblockPlusApi &&
-      typeof AdblockPlusApi.filterLoadListener == "function")

[Wladimir Palant, 2015/03/20 16:38:20]

Using typeof is sufficient, "filterLoadListener" in AdblockPlusApi is mostly
equivalent to typeof AdblockPlusApi.filterLoadListener != "undefined".

[Felix Dahlke, 2015/03/20 16:54:57]

Ouch, getting rusty...

+  FilterNotifier.removeListener(filterListener);
+  if (typeof AdblockPlusApi.filterLoadListener == "function")
     AdblockPlusApi.filterLoadListener();
 });