Issue 4826014056185856: Add filtersLoaded and filterLoadListener to AdblockPlusApi (Closed)
|
Created:
March 20, 2015, 3:59 p.m. by Felix Dahlke Modified:
May 21, 2015, 11:49 a.m. Visibility:
Public. |
DescriptionAdd filtersLoaded and filterLoadListener to AdblockPlusApi
Patch Set 1 : #
Total comments: 8
Patch Set 2 : Discard listener, remove redundant check #Patch Set 3 : Discard the filter listener, not the filter load listener #Patch Set 4 : Use FilterStorage._loading to check if filters have been loaded #
Total comments: 2
Patch Set 5 : Remove filterListener before invoking filterLoadListener #
Total comments: 2
MessagesTotal messages: 9
These allow us to check whether ABP is fully loaded, and wait for it if that's not the case.
http://codereview.adblockplus.org/4826014056185856/diff/5668600916475904/adbl... File adblockplus/Api.jsm (right): http://codereview.adblockplus.org/4826014056185856/diff/5668600916475904/adbl... adblockplus/Api.jsm:46: return filtersLoaded; return FilterStorage._loading? http://codereview.adblockplus.org/4826014056185856/diff/5668600916475904/adbl... adblockplus/Api.jsm:66: filtersLoaded = true; This listener should be removed after the first call rather than staying in memory and being called for each change. http://codereview.adblockplus.org/4826014056185856/diff/5668600916475904/adbl... adblockplus/Api.jsm:68: typeof AdblockPlusApi.filterLoadListener == "function") Using typeof is sufficient, "filterLoadListener" in AdblockPlusApi is mostly equivalent to typeof AdblockPlusApi.filterLoadListener != "undefined".
New patch set is up. http://codereview.adblockplus.org/4826014056185856/diff/5668600916475904/adbl... File adblockplus/Api.jsm (right): http://codereview.adblockplus.org/4826014056185856/diff/5668600916475904/adbl... adblockplus/Api.jsm:46: return filtersLoaded; On 2015/03/20 16:38:20, Wladimir Palant wrote: > return FilterStorage._loading? 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. http://codereview.adblockplus.org/4826014056185856/diff/5668600916475904/adbl... adblockplus/Api.jsm:66: filtersLoaded = true; On 2015/03/20 16:38:20, Wladimir Palant wrote: > This listener should be removed after the first call rather than staying in > memory and being called for each change. Done. http://codereview.adblockplus.org/4826014056185856/diff/5668600916475904/adbl... adblockplus/Api.jsm:68: typeof AdblockPlusApi.filterLoadListener == "function") On 2015/03/20 16:38:20, Wladimir Palant wrote: > Using typeof is sufficient, "filterLoadListener" in AdblockPlusApi is mostly > equivalent to typeof AdblockPlusApi.filterLoadListener != "undefined". Ouch, getting rusty...
Another patch set :) http://codereview.adblockplus.org/4826014056185856/diff/5668600916475904/adbl... File adblockplus/Api.jsm (right): http://codereview.adblockplus.org/4826014056185856/diff/5668600916475904/adbl... adblockplus/Api.jsm:66: filtersLoaded = true; On 2015/03/20 16:54:57, Felix H. Dahlke wrote: > On 2015/03/20 16:38:20, Wladimir Palant wrote: > > This listener should be removed after the first call rather than staying in > > memory and being called for each change. > > Done. 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.
New patch set for a change! http://codereview.adblockplus.org/4826014056185856/diff/5668600916475904/adbl... File adblockplus/Api.jsm (right): http://codereview.adblockplus.org/4826014056185856/diff/5668600916475904/adbl... adblockplus/Api.jsm:46: return filtersLoaded; On 2015/03/20 16:54:57, Felix H. Dahlke wrote: > On 2015/03/20 16:38:20, Wladimir Palant wrote: > > return FilterStorage._loading? > > 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. 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.
http://codereview.adblockplus.org/4826014056185856/diff/5733935958982656/adbl... File adblockplus/Api.jsm (right): http://codereview.adblockplus.org/4826014056185856/diff/5733935958982656/adbl... adblockplus/Api.jsm:66: FilterNotifier.removeListener(filterListener); Please remove listener as the very first thing - filterLoadListener might trigger an exception or a recursive call.
New patch set. http://codereview.adblockplus.org/4826014056185856/diff/5733935958982656/adbl... File adblockplus/Api.jsm (right): http://codereview.adblockplus.org/4826014056185856/diff/5733935958982656/adbl... adblockplus/Api.jsm:66: FilterNotifier.removeListener(filterListener); On 2015/03/22 19:23:34, Wladimir Palant wrote: > Please remove listener as the very first thing - filterLoadListener might > trigger an exception or a recursive call. Done.
http://codereview.adblockplus.org/4826014056185856/diff/5673385510043648/adbl... File adblockplus/Api.jsm (right): http://codereview.adblockplus.org/4826014056185856/diff/5673385510043648/adbl... adblockplus/Api.jsm:59: FilterNotifier.addListener(function filterListener(action) Oh, and from my side: we do not even really need this listener, anyway. I really only need 'get filtersLoaded()' above.
http://codereview.adblockplus.org/4826014056185856/diff/5673385510043648/adbl... File adblockplus/Api.jsm (right): http://codereview.adblockplus.org/4826014056185856/diff/5673385510043648/adbl... adblockplus/Api.jsm:59: FilterNotifier.addListener(function filterListener(action) On 2015/03/22 21:01:46, René Jeschke wrote: > Oh, and from my side: we do not even really need this listener, anyway. I really > only need 'get filtersLoaded()' above. 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. 
|
