Issue 6180766664884224: Issue 1663 - Made first-run page use an asynchronous messaging protocol (Closed)
|
Created:
Dec. 16, 2014, 2:07 p.m. by Wladimir Palant Modified:
Dec. 19, 2014, 1:32 p.m. Visibility:
Public. |
DescriptionI considered your interface proposal here but I guess you will notice a few inconsistencies. Most important one is that we should keep the current messaging API, at least for now.
Patch Set 1 : #
Total comments: 42
Patch Set 2 : More common approach to wrapping code #Patch Set 3 : Documentation fixes #Patch Set 4 : Adressed comments #
MessagesTotal messages: 12
I suppose you stick with ext.*, just because the new UI abstraction layer isn't implemented yet, and this change is rather urgent? http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firs... File firstRun.js (right): http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firs... firstRun.js:97: info.platformVersion == "6.1" || // extensions are broken in 6.1 and 7.0 The old code used Services.vc.compare() here to match 6.1.0 as well. Same below for 7.0.0. Maybe we should do the check in the core and keep using Services.vc.compare(). http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firs... firstRun.js:145: if (!/[.\/]adblockplus\.org$/.test(event.origin)) This would match http://example.com/www.adblockplus.org. How about |/(^|\.)adblockplus\.org$/.test(document.domain)| ?
http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firs... File firstRun.js (right): http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firs... firstRun.js:145: if (!/[.\/]adblockplus\.org$/.test(event.origin)) On 2014/12/16 14:35:07, Sebastian Noack wrote: > This would match http://example.com/www.adblockplus.org. How about > |/(^|\.)adblockplus\.org$/.test(document.domain)| ? event.origin isn't a URL - see https://developer.mozilla.org/en-US/docs/Web/API/Window.postMessage#The_dispa...
http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firs... File firstRun.js (right): http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firs... firstRun.js:145: if (!/[.\/]adblockplus\.org$/.test(event.origin)) On 2014/12/16 15:17:18, Wladimir Palant wrote: > On 2014/12/16 14:35:07, Sebastian Noack wrote: > > This would match http://example.com/www.adblockplus.org. How about > > |/(^|\.)adblockplus\.org$/.test(document.domain)| ? > > event.origin isn't a URL - see > https://developer.mozilla.org/en-US/docs/Web/API/Window.postMessage#The_dispa... Got ya. Feel free to ignore this comment then.
I changed the way ext/content.js wraps its code, addressing the comment from http://codereview.adblockplus.org/6427347985104896/ here as well. http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firs... File firstRun.js (right): http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firs... firstRun.js:97: info.platformVersion == "6.1" || // extensions are broken in 6.1 and 7.0 On 2014/12/16 14:35:07, Sebastian Noack wrote: > The old code used Services.vc.compare() here to match 6.1.0 as well. Same below > for 7.0.0. Maybe we should do the check in the core and keep using > Services.vc.compare(). That's way more message passing than this is worth. I checked and Apple doesn't seem to use either "6.1.0" or "7.0.0" as version strings - it's "6.1" and "7.0".
http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firs... File firstRun.js (right): http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firs... firstRun.js:97: info.platformVersion == "6.1" || // extensions are broken in 6.1 and 7.0 On 2014/12/17 13:13:37, Wladimir Palant wrote: > On 2014/12/16 14:35:07, Sebastian Noack wrote: > > The old code used Services.vc.compare() here to match 6.1.0 as well. Same > below > > for 7.0.0. Maybe we should do the check in the core and keep using > > Services.vc.compare(). > > That's way more message passing than this is worth. Not necessarily. You could just add a new message type like: ext.backgroundPage.sendMessage( {type: "app.isLegacySafariVersion"}, function(isLegacySafariVersion) { if (!isLegacySafariVersion) ... } ); Or if you want to keep it more flexible: ext.backgroundPage.sendMessage( { type: "app.compareVersion", versions: [{lt: true, version: "6"}, "6.1.0", "7.0.0"] }, function(isLegacySafariVersion) { if (!isLegacySafariVersion) ... } );
http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firs... File firstRun.js (right): http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firs... firstRun.js:97: info.platformVersion == "6.1" || // extensions are broken in 6.1 and 7.0 Frankly, neither approach sounds like worth doing to me, not for a minor detail like this one (I actually considered removing the 6.1/7.0 check altogether, given that the issue affects only a handful of users by now). So this is up to Thomas, he is both reviewer and module owner here.
http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firs... File firstRun.js (right): http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firs... firstRun.js:97: info.platformVersion == "6.1" || // extensions are broken in 6.1 and 7.0 On 2014/12/17 14:37:04, Wladimir Palant wrote: > Frankly, neither approach sounds like worth doing to me, not for a minor detail like this one Well, the former approach wouldn't add any complexity, but merely moves existing logic to the core. > (I actually considered removing the 6.1/7.0 check altogether, > given that the issue affects only a handful of users by now). Please don't. It affects all users on OS X Mavericks, which didn't install any updates yet. Anyway, it's not important, since you made sure that the code works. So LGTM.
Just to reiterate: The requirements for the interface were that it 1) could allow grouping of similar messages 2) could allow making precise commands (e.g. retrieve only information that was needed) So apart from naming suggestions to keep those consistent those were the main points my feedback is about. http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/READ... File README.md (right): http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/READ... README.md:46: setup), but it can also happen in the user's settings get lost for some reason. "in case the user's settings get lost" http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/ext/... File ext/content.js (right): http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/ext/... ext/content.js:20: function getURLParameters(data) This name is not reflecting what the function is doing. I'd suggest changing it to "overwriteResponse", "considerURLParameters" or even "modifyResponseWithURLParameters". http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/ext/... ext/content.js:50: var dispatchListenerNotification = function(action) Nit: Since this is only used for subscription notification I'd suggest renaming this function to "dispatchSubscriptionNotification". http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/ext/... ext/content.js:52: var match = /^subscription\.(.*)/.exec(action); In addition to my comment above, by directly passing in "added" or "removed" you could avoid having to match the string. http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/ext/... ext/content.js:63: switch (message.type) Introducing new method names should not be the norm. Therefore following the interface proposal I'd suggest the following changes: - app.info -> app.info with arguments "platform", "platformVersion", "application" and "applicationVersion" - app.issues -> app.get with argument "issues" - app.options -> app.open with argument "options" - subscriptions.toggle -> subscriptions.set with argument {disabled: BOOLEAN, url: STRING} The general idea is that the method defines what should be done to allow grouping together similar messages. Whereas the arguments define what an action should be applied to and also to pass further information if necessary. http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/ext/... ext/content.js:66: respond("https://adblockplus.org/redirect?link=" + encodeURIComponent(message.args[0])); Nit: This line doesn't need to be that long. Coding style: "Lines can be longer than the limit, if limiting line length [to 80 characters] would hurt readability in a particular case." http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/ext/... ext/content.js:69: var response = {platform: "gecko", platformVersion: "34.0", application: "firefox", applicationVersion: "34.0"}; Nit: This line doesn't need to be that long. http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/ext/... ext/content.js:74: var response = {seenDataCorruption: false, filterlistsReinitialized: false}; Nit: This line doesn't need to be that long. http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firs... File firstRun.js (right): http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firs... firstRun.js:46: ext.backgroundPage.sendMessage({type: "app.doclink", args: [link]}, callback); Isn't that exactly what we didn't want to do, send a message for each single information? One idea behind the new interface was to send one message to request multiple things of the same kind. e.g. ext.backgroundPage.sendMessage({ type: "app.doclink", args: [ "donate", "contributors", "acceptableAdsExplanation", "share-headline" ] }, function(values) { if (values.donate) E("donate").href = values.donate; ... }); http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firs... firstRun.js:97: info.platformVersion == "6.1" || // extensions are broken in 6.1 and 7.0 On 2014/12/17 15:08:55, Sebastian Noack wrote: > On 2014/12/17 14:37:04, Wladimir Palant wrote: > > Frankly, neither approach sounds like worth doing to me, not for a minor > detail like this one > > Well, the former approach wouldn't add any complexity, but merely moves existing > logic to the core. > > > (I actually considered removing the 6.1/7.0 check altogether, > > given that the issue affects only a handful of users by now). > > Please don't. It affects all users on OS X Mavericks, which didn't install any > updates yet. > > Anyway, it's not important, since you made sure that the code works. So LGTM. I could think of a nice middleway. The current implementation always returns the full `info` object but since you can pass arguments with the message you could pass a list of properties that you'd like to have and in addition also some additional information like whether it's a legacy Safari version. e.g. ext.backgroundPage.sendMessage({ type: "app.info", args: ["isLegacySafari"] }, function(values) { if (values.isLegacySafari) E("legacySafariWarning").removeAttribute("hidden"); }); http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firs... firstRun.js:118: ext.backgroundPage.sendMessage({type: "subscriptions.listen", filter: ["added", "removed", "disabled"]}); Nit: This line doesn't need to be that long. http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firs... firstRun.js:118: ext.backgroundPage.sendMessage({type: "subscriptions.listen", filter: ["added", "removed", "disabled"]}); By renaming "filter" to "args" it would become consistent with my other comments. http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firs... firstRun.js:133: }); I wouldn't put the reserved "type" property in the same level as the arguments to avoid collisions. e.g. (without considering comments from ext/content.js) ext.backgroundPage.sendMessage({ type: "subscriptions.toggle", args: [ { homepage: featureSubscription.homepage, title: featureSubscription.title, url: featureSubscription.url } ] }); http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firs... firstRun.js:193: var message = { Same as above regarding "type" property collisions. http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firs... firstRun.js:204: link.removeEventListener("click", onSocialLinkClick, false); No need to remove the event listener because it won't be added unless `blocked` is `false` http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firs... firstRun.js:253: ext.backgroundPage.sendMessage({type: "subscriptions.get"}, function(subscriptions) What you want here is only downloadable subscriptions therefore I'd suggest specifying "downloadable" as an argument (as specified in the interface proposal)
http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/READ... File README.md (right): http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/READ... README.md:46: setup), but it can also happen in the user's settings get lost for some reason. On 2014/12/18 10:17:48, Thomas Greiner wrote: > "in case the user's settings get lost" Done. http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/ext/... File ext/content.js (right): http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/ext/... ext/content.js:20: function getURLParameters(data) On 2014/12/18 10:17:48, Thomas Greiner wrote: > This name is not reflecting what the function is doing. I'd suggest changing it > to "overwriteResponse", "considerURLParameters" or even > "modifyResponseWithURLParameters". I went with updateFromURL() which isn't quite as verbose. http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/ext/... ext/content.js:50: var dispatchListenerNotification = function(action) On 2014/12/18 10:17:48, Thomas Greiner wrote: > Nit: Since this is only used for subscription notification I'd suggest renaming > this function to "dispatchSubscriptionNotification". I'm ignoring this nit because that callback is being renamed into onFilterChange by the next patch anyway. http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/ext/... ext/content.js:52: var match = /^subscription\.(.*)/.exec(action); On 2014/12/18 10:17:48, Thomas Greiner wrote: > In addition to my comment above, by directly passing in "added" or "removed" you > could avoid having to match the string. Same here, this is no longer relevant because the next patch implements a "proper" FilterNotifier. http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/ext/... ext/content.js:63: switch (message.type) I've mostly changed it like this. However, I'm not working with an array of parameters but an object - so I decided to use what: "issues" and what: "options". Also, subscription.toggle is being kept as discussed before. http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/ext/... ext/content.js:66: respond("https://adblockplus.org/redirect?link=" + encodeURIComponent(message.args[0])); On 2014/12/18 10:17:48, Thomas Greiner wrote: > Nit: This line doesn't need to be that long. This code is being rewritten by the next patch, no longer too long there. http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/ext/... ext/content.js:69: var response = {platform: "gecko", platformVersion: "34.0", application: "firefox", applicationVersion: "34.0"}; On 2014/12/18 10:17:48, Thomas Greiner wrote: > Nit: This line doesn't need to be that long. This code is being rewritten by the next patch, no longer too long there. http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/ext/... ext/content.js:74: var response = {seenDataCorruption: false, filterlistsReinitialized: false}; On 2014/12/18 10:17:48, Thomas Greiner wrote: > Nit: This line doesn't need to be that long. This code is being rewritten by the next patch, no longer too long there. http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firs... File firstRun.js (right): http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firs... firstRun.js:46: ext.backgroundPage.sendMessage({type: "app.doclink", args: [link]}, callback); On 2014/12/18 10:17:48, Thomas Greiner wrote: > Isn't that exactly what we didn't want to do, send a message for each single > information? Sure, but that's more relevant for the scenarios where there is lots of data - here it doesn't seem worth the added complexity. Documentation links are never many. http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firs... firstRun.js:97: info.platformVersion == "6.1" || // extensions are broken in 6.1 and 7.0 Changed that, as discussed with Thomas: app.info message is gone, added legacySafariVersion to app.issues response. http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firs... firstRun.js:118: ext.backgroundPage.sendMessage({type: "subscriptions.listen", filter: ["added", "removed", "disabled"]}); On 2014/12/18 10:17:48, Thomas Greiner wrote: > By renaming "filter" to "args" it would become consistent with my other > comments. It would also become inconsistent with my objections ;) As I said before, we want to send structured data, not an unstructured list of arguments. Here we want to send a filter to be applied, and there might be more parameters in future. http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firs... firstRun.js:133: }); On 2014/12/18 10:17:48, Thomas Greiner wrote: > I wouldn't put the reserved "type" property in the same level as the arguments > to avoid collisions. Fine with me if we have a new API that will accept the type as a separate parameter and generate that structure automatically (and decompile it on the receiving end again). However, until then I definitely don't want to specify overly complicated data structures in the code - and we can easily avoid collisions by not specifying parameters that are named "type". http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firs... firstRun.js:204: link.removeEventListener("click", onSocialLinkClick, false); On 2014/12/18 10:17:48, Thomas Greiner wrote: > No need to remove the event listener because it won't be added unless `blocked` > is `false` This function is being called multiple times, whenever user's subscriptions change. So the event listener *can* be there, added by the previous invocation of this function. This is a somewhat awkward construct but the user can add the subscription to block social buttons right on this page. If he chooses to do that before clicking the social links we should adapt. And we cannot just query the filters when the button is clicked because this will introduce delays and we won't be able to open the original link target in a new tab any more (pop-up blocker).
LGTM http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/ext/... File ext/content.js (right): http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/ext/... ext/content.js:20: function getURLParameters(data) On 2014/12/18 19:31:35, Wladimir Palant wrote: > On 2014/12/18 10:17:48, Thomas Greiner wrote: > > This name is not reflecting what the function is doing. I'd suggest changing > it > > to "overwriteResponse", "considerURLParameters" or even > > "modifyResponseWithURLParameters". > > I went with updateFromURL() which isn't quite as verbose. Thanks, that's great. http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/ext/... ext/content.js:63: switch (message.type) On 2014/12/18 19:31:35, Wladimir Palant wrote: > I've mostly changed it like this. However, I'm not working with an array of > parameters but an object - so I decided to use what: "issues" and what: > "options". Also, subscription.toggle is being kept as discussed before. Looks good. Not sure about whether an array for "app.get" at least might not be the better choice since instead of `message.what == "issues"` it's just as easy to do `message.what.indexOf("issues") > -1`. However, I do agree with your argument that this might be unnecessary complexity for now so fine with me not to use an array until it becomes necessary (see http://codereview.adblockplus.org/6180766664884224/diff2/5741031244955648:575...) http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firs... File firstRun.js (right): http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firs... firstRun.js:133: }); On 2014/12/18 19:31:35, Wladimir Palant wrote: > On 2014/12/18 10:17:48, Thomas Greiner wrote: > > I wouldn't put the reserved "type" property in the same level as the arguments > > to avoid collisions. > > Fine with me if we have a new API that will accept the type as a separate > parameter and generate that structure automatically (and decompile it on the > receiving end again). However, until then I definitely don't want to specify > overly complicated data structures in the code - and we can easily avoid > collisions by not specifying parameters that are named "type". Sounds like a good compromise to me. http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/firs... firstRun.js:204: link.removeEventListener("click", onSocialLinkClick, false); On 2014/12/18 19:31:35, Wladimir Palant wrote: > On 2014/12/18 10:17:48, Thomas Greiner wrote: > > No need to remove the event listener because it won't be added unless > `blocked` > > is `false` > > This function is being called multiple times, whenever user's subscriptions > change. So the event listener *can* be there, added by the previous invocation > of this function. > > This is a somewhat awkward construct but the user can add the subscription to > block social buttons right on this page. If he chooses to do that before > clicking the social links we should adapt. And we cannot just query the filters > when the button is clicked because this will introduce delays and we won't be > able to open the original link target in a new tab any more (pop-up blocker). Ok and thanks for adapting the function name to reflect that.
http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/ext/... File ext/content.js (right): http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/ext/... ext/content.js:63: switch (message.type) On 2014/12/19 10:53:38, Thomas Greiner wrote: > Looks good. Not sure about whether an array for "app.get" at least might not be > the better choice since instead of `message.what == "issues"` it's just as easy > to do `message.what.indexOf("issues") > -1`. The problem isn't checking what was requested - the problem is rather sending the result back in a meaningful way, as this API allows requesting multiple things at the same time. After considering the possibilities here, all of them seem unnecessarily complicated - particularly given that using app.get to request multiple things at the same time seems rather unusual. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
