Issue 29338734: Issue 3839 - Adapt for Prefs event changes (Closed)
|
Created:
March 19, 2016, 7:14 p.m. by Sebastian Noack Modified:
March 23, 2016, 1:33 p.m. Reviewers:
Thomas Greiner Visibility:
Public. |
DescriptionIssue 3839 - Adapt for Prefs event changes
Patch Set 1 #
Total comments: 2
Patch Set 2 : #
Total comments: 9
Patch Set 3 : app.listen doesn't need a FilterNotifier listener #Patch Set 4 : Reuse event emitter logic #Patch Set 5 : Simplified FilterNotifier logic #Patch Set 6 : Cleaned up onFilterChange() #
Total comments: 8
Patch Set 7 : Addresed comments #MessagesTotal messages: 8
https://codereview.adblockplus.org/29338734/diff/29338735/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29338734/diff/29338735/messageResponder.js... messageResponder.js:65: var pages = changeListeners.keys(); This is unrelated, but since page is never specified, I used the chance to get rid of the dead code here. https://codereview.adblockplus.org/29338734/diff/29338735/messageResponder.js... messageResponder.js:70: if (actions && !(actions instanceof Array && actions.indexOf(action) == -1)) filters.pref is an object now, not an array anymore. And filtering is done by Prefs.on, so it can/must be skipped here.
I forgot to adapt the mock implementation in background.js (I only tested with the extension). Moreover, I figured that we should rather only register one listener per preference (than one per preference per page) as we won't remove the listeners anymore. Also this minimizes the code verge between the filter listener code.
https://codereview.adblockplus.org/29338734/diff/29338841/background.js File background.js (right): https://codereview.adblockplus.org/29338734/diff/29338841/background.js#newco... background.js:98: on: function(preference, callback) What about extending `Notifier` instead so that `Notifier.addListener()` and `Notifier.removeListener()` accept an optional name parameter at the first position? That way we could reuse it when we decide to migrate other APIs to the new calling scheme. We'd also avoid duplicating some of the logic. https://codereview.adblockplus.org/29338734/diff/29338841/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29338734/diff/29338841/messageResponder.js... messageResponder.js:122: } I guess we could just unconditionally start listening when the background page starts up and stop it when the extension shuts down. That way we'd get rid of some unnecessary overhead around determining when we actually need it. https://codereview.adblockplus.org/29338734/diff/29338841/messageResponder.js... messageResponder.js:315: if (listenedPreferences.indexOf(preference) == -1) So only the first UI that registers a listener for a preference will actually get it? https://codereview.adblockplus.org/29338734/diff/29338841/messageResponder.js... messageResponder.js:321: }); This listener is never removed. As you pointed out `FilterNotifier.removeListener()` and `Prefs.onChanged.removeListener()` were never called. So far that hasn't been an issue since the messageResponder couldn't accumulate more than a single listener instance and since Chrome (unlike Firefox) throws those away on extension shutdown anyway. This change, however, would turn it into a memory leak if we end up having one listener per preference per UI. Just FYI: Currently, stopping a listener is achieved by sending "prefs.listen" without "filter" - probably better to have an explicit "prefs.unlisten" method but that's how it was implemented so far. https://codereview.adblockplus.org/29338734/diff/29338841/messageResponder.js... messageResponder.js:372: var listenerFilters = getListenerFilters(sender.page, true); Nice. No idea why we didn't implement it like this before already.
https://codereview.adblockplus.org/29338734/diff/29338841/background.js File background.js (right): https://codereview.adblockplus.org/29338734/diff/29338841/background.js#newco... background.js:98: on: function(preference, callback) On 2016/03/21 17:55:55, Thomas Greiner wrote: > What about extending `Notifier` instead so that `Notifier.addListener()` and > `Notifier.removeListener()` accept an optional name parameter at the first > position? > > That way we could reuse it when we decide to migrate other APIs to the new > calling scheme. We'd also avoid duplicating some of the logic. Well, changing the calling convention of existing APIs across core and platform is not the scope of this patch. However, we can move the this logic into a class and use it under the hood to implement Notifier. Did that. https://codereview.adblockplus.org/29338734/diff/29338841/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29338734/diff/29338841/messageResponder.js... messageResponder.js:122: } On 2016/03/21 17:55:55, Thomas Greiner wrote: > I guess we could just unconditionally start listening when the background page > starts up and stop it when the extension shuts down. > > That way we'd get rid of some unnecessary overhead around determining when we > actually need it. I considered it, but I rather avoid running the listener on each filter notification when the options page has never been opened, considering that most users don't often use the options page, that we dispatch plenty filter notifications, and that this particular listener does quite some stuff before it might bail out. https://codereview.adblockplus.org/29338734/diff/29338841/messageResponder.js... messageResponder.js:315: if (listenedPreferences.indexOf(preference) == -1) On 2016/03/21 17:55:55, Thomas Greiner wrote: > So only the first UI that registers a listener for a preference will actually > get it? Yes, the first UI that listens for a particular preference, causes a listener for that preference to be added. But note that the listener calls sendMessage() which notifies all pages that are listening for that preference. https://codereview.adblockplus.org/29338734/diff/29338841/messageResponder.js... messageResponder.js:321: }); On 2016/03/21 17:55:55, Thomas Greiner wrote: > This listener is never removed. > > As you pointed out `FilterNotifier.removeListener()` and > `Prefs.onChanged.removeListener()` were never called. So far that hasn't been an > issue since the messageResponder couldn't accumulate more than a single listener > instance and since Chrome (unlike Firefox) throws those away on extension > shutdown anyway. > This change, however, would turn it into a memory leak if we end up having one > listener per preference per UI. > > Just FYI: Currently, stopping a listener is achieved by sending "prefs.listen" > without "filter" - probably better to have an explicit "prefs.unlisten" method > but that's how it was implemented so far. Removing the listeners isn't trivial, since you cannot send messages in on unload. Also there are scenarios (e.g. when a tab runs out of memory, crashes for a different reason, etc.) where onload isn't triggered at all. So we would have to listen for the tab to be removed, replaced or navigating somewhere else. The PageMap object accounts for all of these scenarios. However, the abstraction layer doesn't expose some of these events directly. Neither, do I think that the addional complexity would be justified. After all, leaking one listener per preference isn't too bad, and IMO not any worse than before, considering that the listener we leaked before was called for any preference. Now we have multiple listeners but they are only called for specific preferences. hence add less overhead.
https://codereview.adblockplus.org/29338734/diff/29338897/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29338734/diff/29338897/messageResponder.js... messageResponder.js:101: for (var i = 1; i < arguments.length; i++) Note that passing arguments to other functions will cause this function from beeing deoptimized in V8. So while cleaning up here, I fixed that as well. https://codereview.adblockplus.org/29338734/diff/29338897/messageResponder.js... messageResponder.js:284: FilterNotifier.addListener(onFilterChange); Since FilterNotifier.addListener() only adds the listener if it hasn't been added before, we can just add onFilterChange when we need it, without bothering whether we added it before. https://codereview.adblockplus.org/29338734/diff/29338897/messageResponder.js... messageResponder.js:341: sendMessage("app", "addSubscription", [convertSubscription(subscription)]); Sorry for the unrelated change, but it seems inappropriate to call onFilterChange() here. Also if we don't the logic of onFilterChange() can be simplified a little.
Thanks for addressing the comments. Just two smaller ones before we're good. https://codereview.adblockplus.org/29338734/diff/29338897/background.js File background.js (right): https://codereview.adblockplus.org/29338734/diff/29338897/background.js#newco... background.js:60: var listeners = this._eventEmitter[""]; Detail: Shouldn't this be `var listeners = this._eventEmitter._listeners[""];`? Functionality-wise there's no difference but it doesn't seem to be done on purpose. https://codereview.adblockplus.org/29338734/diff/29338897/messageResponder.js File messageResponder.js (left): https://codereview.adblockplus.org/29338734/diff/29338897/messageResponder.js... messageResponder.js:87: if (parts.length == 1) What about other events dispatched by `FilterNotifier`? e.g. "elemhideupdate" and "save" So far we've been converting them to "app.*" to ensure that all event names have two parts. However, we could simply throw them away if you think that's more reasonable.
https://codereview.adblockplus.org/29338734/diff/29338897/background.js File background.js (right): https://codereview.adblockplus.org/29338734/diff/29338897/background.js#newco... background.js:60: var listeners = this._eventEmitter[""]; On 2016/03/22 13:29:40, Thomas Greiner wrote: > Detail: Shouldn't this be `var listeners = this._eventEmitter._listeners[""];`? > > Functionality-wise there's no difference but it doesn't seem to be done on > purpose. Well spotted, and actually it is a bug. https://codereview.adblockplus.org/29338734/diff/29338897/messageResponder.js File messageResponder.js (left): https://codereview.adblockplus.org/29338734/diff/29338897/messageResponder.js... messageResponder.js:87: if (parts.length == 1) On 2016/03/22 13:29:40, Thomas Greiner wrote: > What about other events dispatched by `FilterNotifier`? e.g. "elemhideupdate" > and "save" > > So far we've been converting them to "app.*" to ensure that all event names have > two parts. However, we could simply throw them away if you think that's more > reasonable. Currently "app.addSubscription" is the only "app.*" event the UI is interested in. And it might be cleaner to leave that namespace to events not coming from FilterNotifier. As for "saved" I think it should rather be "filters.saved" for consistency with "filters.loaded" but as long as we don't need it in the UI, I'd prefer to ingore that case and bail out early here. https://codereview.adblockplus.org/29338734/diff/29338897/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29338734/diff/29338897/messageResponder.js... messageResponder.js:92: var parts = action.split(".", 2); I just realized that the second argument is redundant as .split(".", 2) is just a shorthand for .split(".").slice(0,2) in JavaScript.
LGTM 
|
