Issue 29340571: Issue 3687 - Add experimental support for Safari content blockers (Closed)
|
Created:
April 19, 2016, 1:59 p.m. by kzar Modified:
May 18, 2016, 12:47 p.m. Reviewers:
Sebastian Noack Visibility:
Public. |
DescriptionIssue 3687 - Add experimental support for Safari content blockers
Patch Set 1 #
Total comments: 49
Patch Set 2 : Rebased #Patch Set 3 : Addressed feedback #Patch Set 4 : Improve comment about strange API behaviour #Patch Set 5 : Added note about bug report #
Total comments: 7
Patch Set 6 : Improve comments about buggy behavouir #
Total comments: 4
Patch Set 7 : Addressed further feedback #Patch Set 8 : Don't reuse strings from new options page #Patch Set 9 : Avoid type conversion when handling Safari bug #
Total comments: 4
Patch Set 10 : Don't avoid type conversion #Patch Set 11 : Remove onChange logic for checkboxes #
Total comments: 13
Patch Set 12 : Addressed Nits #
MessagesTotal messages: 21
Patch Set 1 https://codereview.adblockplus.org/29340571/diff/29340572/metadata.chrome File metadata.chrome (right): https://codereview.adblockplus.org/29340571/diff/29340572/metadata.chrome#new... metadata.chrome:45: lib/adblockplus.js -= abp2blocklist/lib/abp2blocklist.js safari/contentBlocking.js I'm guessing there's a better way to achieve the same thing, any suggestions? https://codereview.adblockplus.org/29340571/diff/29340572/safari/contentBlock... File safari/contentBlocking.js (right): https://codereview.adblockplus.org/29340571/diff/29340572/safari/contentBlock... safari/contentBlocking.js:29: let legacyApiSupported = "onbeforeload" in Element.prototype; I remember you said we shouldn't assume both onbeforeload and canLoad will be removed at the same time, but I can't see how else to do this. (We don't know about canLoad here and we need to make a decision.)
https://codereview.adblockplus.org/29340571/diff/29340572/_locales/en_US/mess... File _locales/en_US/messages.json (left): https://codereview.adblockplus.org/29340571/diff/29340572/_locales/en_US/mess... _locales/en_US/messages.json:124: "safari_content_blocker": { Why did you remove the message? Also what's about the message to restart Safari? https://codereview.adblockplus.org/29340571/diff/29340572/metadata.chrome File metadata.chrome (right): https://codereview.adblockplus.org/29340571/diff/29340572/metadata.chrome#new... metadata.chrome:45: lib/adblockplus.js -= abp2blocklist/lib/abp2blocklist.js safari/contentBlocking.js On 2016/04/19 14:03:49, kzar wrote: > I'm guessing there's a better way to achieve the same thing, any suggestions? I agree that it's kinda backwards to remove scripts that are only necessary for one platform in the configuration for another platform. However, since the convert_js options take some arguments that need to be given in the end, we cannot append to those options. However, feel free to change the logic in buildtools to make stuff like this more straight-forward. But how it's currently implemented this is the only way to do it. https://codereview.adblockplus.org/29340571/diff/29340572/metadata.common File metadata.common (right): https://codereview.adblockplus.org/29340571/diff/29340572/metadata.common#new... metadata.common:101: lib/punycode.js So you wrap punycode.js into a module, however it still defines a global that is used by most of our code? Perhaps we should prevent it from creating a global, and always use it as a module? https://codereview.adblockplus.org/29340571/diff/29340572/options.js File options.js (right): https://codereview.adblockplus.org/29340571/diff/29340572/options.js#newcode158 options.js:158: if (!checkbox.checked) As discussed with Thomas on the review for the related change to the new options page, we explictily decided against persistent logic and agreed that it's sufficient to notify the user only once when he disabled content blockers. If we want to persistently notify the user about this state, the options page is the wrong place to do so. But given that disabling an experimental feature that have previously be enabled on Safari can be considered edge case, notifying the user once should be sufficient. https://codereview.adblockplus.org/29340571/diff/29340572/options.js#newcode544 options.js:544: // Apparently modifying the checked attribute for a checkbox does not If you want to emulate user actions that's what the click() method is for. However, it would be better to call the change handler explicitly here. https://codereview.adblockplus.org/29340571/diff/29340572/safari/contentBlock... File safari/contentBlocking.js (right): https://codereview.adblockplus.org/29340571/diff/29340572/safari/contentBlock... safari/contentBlocking.js:29: let legacyApiSupported = "onbeforeload" in Element.prototype; On 2016/04/19 14:03:49, kzar wrote: > I remember you said we shouldn't assume both onbeforeload and canLoad will be > removed at the same time, but I can't see how else to do this. (We don't know > about canLoad here and we need to make a decision.) You could detect the legacy API in the content script, send the result with the "loading" message, make legacyApiSupported a promise and resolve it when the first loading message arrives. https://codereview.adblockplus.org/29340571/diff/29340572/safari/contentBlock... safari/contentBlocking.js:31: let afterContentBlockingFinished; If I understand the logic correctly, this variable might be accessed before assigned. So initialize it with null here? https://codereview.adblockplus.org/29340571/diff/29340572/safari/contentBlock... safari/contentBlocking.js:65: JSON.stringify(contentBlockerList.generateRules()), That needs to be tracked down properly. If there is no way around then there is no way around. But I'd rather avoid converting this huge array to a string just to have Safari internally parse it again, if possible. https://codereview.adblockplus.org/29340571/diff/29340572/safari/contentBlock... safari/contentBlocking.js:75: function updateContentBlocker(onStartup) Nit: onStartup -> isStartup, since this value indicated whether this is the startup not what to do on startup. https://codereview.adblockplus.org/29340571/diff/29340572/safari/contentBlock... safari/contentBlocking.js:127: if (Prefs["safariContentBlocker"]) Nit: Prefs.safariContentBlocker https://codereview.adblockplus.org/29340571/diff/29340572/safari/contentBlock... safari/contentBlocking.js:130: Prefs.on("safariContentBlocker", onSafariContentBlocker); I wonder whether the logic wouldn't be simpler if you simply always register Prefs.on("safariContentBlocker", ) if content blocking is supported and never unregister it, rather than registering and unregistering it in different places. https://codereview.adblockplus.org/29340571/diff/29340572/safari/contentBlock... safari/contentBlocking.js:132: for (let action of ["load", "subscription.added", "subscription.removed", As usual, jsHydra will duplicate that array in the generated code. Anyway, it seems that you only need to listen for "filter.behaviorChanged" here. https://codereview.adblockplus.org/29340571/diff/29340572/safari/ext/backgrou... File safari/ext/background.js (right): https://codereview.adblockplus.org/29340571/diff/29340572/safari/ext/backgrou... safari/ext/background.js:423: case "useContentBlockerAPI": I think this should rather go into safari/contentBlocking.js. Note that you can send any message synchronously on Safari, e.g. see how the include.youtube.js does it. https://codereview.adblockplus.org/29340571/diff/29340572/safari/ext/content.js File safari/ext/content.js (right): https://codereview.adblockplus.org/29340571/diff/29340572/safari/ext/content.... safari/ext/content.js:35: try { Nit: Open brace on newline. https://codereview.adblockplus.org/29340571/diff/29340572/safari/include.yout... File safari/include.youtube.js (right): https://codereview.adblockplus.org/29340571/diff/29340572/safari/include.yout... safari/include.youtube.js:23: try { Nit: Open brace on new line.
Patch Set 2 : Rebased (edit) https://codereview.adblockplus.org/29340571/diff/29340572/_locales/en_US/mess... File _locales/en_US/messages.json (left): https://codereview.adblockplus.org/29340571/diff/29340572/_locales/en_US/mess... _locales/en_US/messages.json:124: "safari_content_blocker": { On 2016/05/12 11:12:22, Sebastian Noack wrote: > Why did you remove the message? Also what's about the message to restart Safari? The strings are provided by adblockplusui/locale/*/options.json, no need to duplicate them here. https://codereview.adblockplus.org/29340571/diff/29340572/metadata.chrome File metadata.chrome (right): https://codereview.adblockplus.org/29340571/diff/29340572/metadata.chrome#new... metadata.chrome:45: lib/adblockplus.js -= abp2blocklist/lib/abp2blocklist.js safari/contentBlocking.js On 2016/05/12 11:12:22, Sebastian Noack wrote: > On 2016/04/19 14:03:49, kzar wrote: > > I'm guessing there's a better way to achieve the same thing, any suggestions? > > I agree that it's kinda backwards to remove scripts that are only necessary for > one platform in the configuration for another platform. However, since the > convert_js options take some arguments that need to be given in the end, we > cannot append to those options. However, feel free to change the logic in > buildtools to make stuff like this more straight-forward. But how it's currently > implemented this is the only way to do it. Acknowledged. (Considering time constraints I will leave this as-is for now.) https://codereview.adblockplus.org/29340571/diff/29340572/metadata.common File metadata.common (right): https://codereview.adblockplus.org/29340571/diff/29340572/metadata.common#new... metadata.common:101: lib/punycode.js On 2016/05/12 11:12:22, Sebastian Noack wrote: > So you wrap punycode.js into a module, however it still defines a global that is > used by most of our code? Perhaps we should prevent it from creating a global, > and always use it as a module? Hmm guess you're right, Done. https://codereview.adblockplus.org/29340571/diff/29340572/options.js File options.js (right): https://codereview.adblockplus.org/29340571/diff/29340572/options.js#newcode158 options.js:158: if (!checkbox.checked) On 2016/05/12 11:12:22, Sebastian Noack wrote: > As discussed with Thomas on the review for the related change to the new options > page, we explictily decided against persistent logic and agreed that it's > sufficient to notify the user only once when he disabled content blockers. > > If we want to persistently notify the user about this state, the options page is > the wrong place to do so. But given that disabling an experimental feature that > have previously be enabled on Safari can be considered edge case, notifying the > user once should be sufficient. I don't understand what you mean by "persistent logic" here. The behaviour I've implemented displays a red "Please restart Safari" message beside the content blocking option when necessary and it handles the various edge-cases pretty well. (For example what if the user clicks to enable content blocking but there's an exception, the old API isn't disabled and the content blocking feature is automatically disabled.) https://codereview.adblockplus.org/29340571/diff/29340572/options.js#newcode544 options.js:544: // Apparently modifying the checked attribute for a checkbox does not On 2016/05/12 11:12:23, Sebastian Noack wrote: > If you want to emulate user actions that's what the click() method is for. > However, it would be better to call the change handler explicitly here. Calling .click() would also toggle the checkbox right? (I guess I could move the checkbox.checked assignment below to force the correct state but it seems kind of pointless.) As for calling the change handler directly here, sounds good to me except I can't see an easy way to get to the handler here. Maybe I'm missing something? (I guess we could assign an extra property to the element, or assign onchange instead of using addEventListener but both of those seem maybe worse than just dispatching the change event.) https://codereview.adblockplus.org/29340571/diff/29340572/safari/contentBlock... File safari/contentBlocking.js (right): https://codereview.adblockplus.org/29340571/diff/29340572/safari/contentBlock... safari/contentBlocking.js:29: let legacyApiSupported = "onbeforeload" in Element.prototype; On 2016/05/12 11:12:23, Sebastian Noack wrote: > On 2016/04/19 14:03:49, kzar wrote: > > I remember you said we shouldn't assume both onbeforeload and canLoad will be > > removed at the same time, but I can't see how else to do this. (We don't know > > about canLoad here and we need to make a decision.) > > You could detect the legacy API in the content script, send the result with the > "loading" message, make legacyApiSupported a promise and resolve it when the > first loading message arrives. Done. https://codereview.adblockplus.org/29340571/diff/29340572/safari/contentBlock... safari/contentBlocking.js:31: let afterContentBlockingFinished; On 2016/05/12 11:12:23, Sebastian Noack wrote: > If I understand the logic correctly, this variable might be accessed before > assigned. So initialize it with null here? Done. https://codereview.adblockplus.org/29340571/diff/29340572/safari/contentBlock... safari/contentBlocking.js:65: JSON.stringify(contentBlockerList.generateRules()), On 2016/05/12 11:12:23, Sebastian Noack wrote: > That needs to be tracked down properly. If there is no way around then there is > no way around. But I'd rather avoid converting this huge array to a string just > to have Safari internally parse it again, if possible. OK, I'll try again to figure out what's going on. https://codereview.adblockplus.org/29340571/diff/29340572/safari/contentBlock... safari/contentBlocking.js:75: function updateContentBlocker(onStartup) On 2016/05/12 11:12:23, Sebastian Noack wrote: > Nit: onStartup -> isStartup, since this value indicated whether this is the > startup not what to do on startup. Done. https://codereview.adblockplus.org/29340571/diff/29340572/safari/contentBlock... safari/contentBlocking.js:127: if (Prefs["safariContentBlocker"]) On 2016/05/12 11:12:23, Sebastian Noack wrote: > Nit: Prefs.safariContentBlocker Done. https://codereview.adblockplus.org/29340571/diff/29340572/safari/contentBlock... safari/contentBlocking.js:130: Prefs.on("safariContentBlocker", onSafariContentBlocker); On 2016/05/12 11:12:23, Sebastian Noack wrote: > I wonder whether the logic wouldn't be simpler if you simply always register > Prefs.on("safariContentBlocker", ) if content blocking is supported and never > unregister it, rather than registering and unregistering it in different places. Done. https://codereview.adblockplus.org/29340571/diff/29340572/safari/contentBlock... safari/contentBlocking.js:132: for (let action of ["load", "subscription.added", "subscription.removed", On 2016/05/12 11:12:23, Sebastian Noack wrote: > As usual, jsHydra will duplicate that array in the generated code. Anyway, it > seems that you only need to listen for "filter.behaviorChanged" here. Done. https://codereview.adblockplus.org/29340571/diff/29340572/safari/ext/backgrou... File safari/ext/background.js (right): https://codereview.adblockplus.org/29340571/diff/29340572/safari/ext/backgrou... safari/ext/background.js:423: case "useContentBlockerAPI": On 2016/05/12 11:12:23, Sebastian Noack wrote: > I think this should rather go into safari/contentBlocking.js. Note that you can > send any message synchronously on Safari, e.g. see how the include.youtube.js > does it. As discussed in IRC we can actually use prefs.get instead of adding this redundant message. https://codereview.adblockplus.org/29340571/diff/29340572/safari/ext/content.js File safari/ext/content.js (right): https://codereview.adblockplus.org/29340571/diff/29340572/safari/ext/content.... safari/ext/content.js:35: try { On 2016/05/12 11:12:23, Sebastian Noack wrote: > Nit: Open brace on newline. Done. https://codereview.adblockplus.org/29340571/diff/29340572/safari/include.yout... File safari/include.youtube.js (right): https://codereview.adblockplus.org/29340571/diff/29340572/safari/include.yout... safari/include.youtube.js:23: try { On 2016/05/12 11:12:23, Sebastian Noack wrote: > Nit: Open brace on new line. Done.
Patch Set 3 : Addressed feedback
Patch Set 4 : Improve comment about strange API behaviour
Patch Set 5 : Added note about bug report
https://codereview.adblockplus.org/29340571/diff/29342098/safari/contentBlock... File safari/contentBlocking.js (right): https://codereview.adblockplus.org/29340571/diff/29342098/safari/contentBlock... safari/contentBlocking.js:78: // Safari 9 performs the callback twice under some conditions, first with As far as I can tell the behaviour of the callback is something like this: - If callback is provided and rules aren't converted to JSON the callback will first be called with an empty string "" and then an Error. Further calls will result in a callback of "" followed by a callback of null. - If the callback is provided and the rules are converted to JSON the callback will be performed with null on success or Error on error. Further calls with the same rule string will always result in a callback of null until a call is performed with a different JSON string. (Even if there was a problem.) - If the callback isn't provided things generally work as expected, but there's no longer a way to verify that! My only other idea to further improve how things work is to add a flag `buggySetContentBlocker` that defaults to false. When the callback is performed with the value of "" the flag is set to true and we start converting the passed rules to JSON. This adds yet more code however and could make things more fragile. (It also would mean we are generating the content list twice in a row for buggy versions and it's already pretty slow when generating it once.)
Patch Set 6 : Improve comments about buggy behaviour
https://codereview.adblockplus.org/29340571/diff/29340572/_locales/en_US/mess... File _locales/en_US/messages.json (left): https://codereview.adblockplus.org/29340571/diff/29340572/_locales/en_US/mess... _locales/en_US/messages.json:124: "safari_content_blocker": { On 2016/05/17 15:15:36, kzar wrote: > On 2016/05/12 11:12:22, Sebastian Noack wrote: > > Why did you remove the message? Also what's about the message to restart > Safari? > > The strings are provided by adblockplusui/locale/*/options.json, no need to > duplicate them here. Those strings aren't imported yet. And even if we add the new options behind a flag soon, though unlikely before the next release, the strings there won't be translated as they aren't final yet. https://codereview.adblockplus.org/29340571/diff/29340572/metadata.chrome File metadata.chrome (right): https://codereview.adblockplus.org/29340571/diff/29340572/metadata.chrome#new... metadata.chrome:45: lib/adblockplus.js -= abp2blocklist/lib/abp2blocklist.js safari/contentBlocking.js On 2016/05/17 15:15:36, kzar wrote: > On 2016/05/12 11:12:22, Sebastian Noack wrote: > > On 2016/04/19 14:03:49, kzar wrote: > > > I'm guessing there's a better way to achieve the same thing, any > suggestions? > > > > I agree that it's kinda backwards to remove scripts that are only necessary > for > > one platform in the configuration for another platform. However, since the > > convert_js options take some arguments that need to be given in the end, we > > cannot append to those options. However, feel free to change the logic in > > buildtools to make stuff like this more straight-forward. But how it's > currently > > implemented this is the only way to do it. > > Acknowledged. (Considering time constraints I will leave this as-is for now.) Feel free to file an issue though. I like the idea, and perhaps it's something for Jon. FWIW, I think this would be a more reasonable way to specify the options, wouldn't insist though: [convert_js] lib/adblockplus.js = <input files> lib/adblockplus.js:module = true lib/adblockplus.js:source_repo = https://hg.adblockplus.org/adblockpluscore/ https://codereview.adblockplus.org/29340571/diff/29340572/options.js File options.js (right): https://codereview.adblockplus.org/29340571/diff/29340572/options.js#newcode158 options.js:158: if (!checkbox.checked) On 2016/05/17 15:15:38, kzar wrote: > On 2016/05/12 11:12:22, Sebastian Noack wrote: > > As discussed with Thomas on the review for the related change to the new > options > > page, we explictily decided against persistent logic and agreed that it's > > sufficient to notify the user only once when he disabled content blockers. > > > > If we want to persistently notify the user about this state, the options page > is > > the wrong place to do so. But given that disabling an experimental feature > that > > have previously be enabled on Safari can be considered edge case, notifying > the > > user once should be sufficient. > > I don't understand what you mean by "persistent logic" here. > > The behaviour I've implemented displays a red "Please restart Safari" message > beside the content blocking option when necessary and it handles the various > edge-cases pretty well. (For example what if the user clicks to enable content > blocking but there's an exception, the old API isn't disabled and the content > blocking feature is automatically disabled.) At the very least this logic is inconsistent with the new options page, where we agreed to ignore edge cases. But fine with me, as long as you keep the new options page in sync with the logic here. https://codereview.adblockplus.org/29340571/diff/29340572/options.js#newcode544 options.js:544: // Apparently modifying the checked attribute for a checkbox does not On 2016/05/17 15:15:37, kzar wrote: > On 2016/05/12 11:12:23, Sebastian Noack wrote: > > If you want to emulate user actions that's what the click() method is for. > > However, it would be better to call the change handler explicitly here. > > Calling .click() would also toggle the checkbox right? (I guess I could move the > checkbox.checked assignment below to force the correct state but it seems kind > of pointless.) > > As for calling the change handler directly here, sounds good to me except I > can't see an easy way to get to the handler here. Maybe I'm missing something? > (I guess we could assign an extra property to the element, or assign onchange > instead of using addEventListener but both of those seem maybe worse than just > dispatching the change event.) I wonder whether the change listener is even necessary. Why don't you simply move the related logic here? If the preference is changed the code here is triggered anyway. https://codereview.adblockplus.org/29340571/diff/29342098/safari/contentBlock... File safari/contentBlocking.js (right): https://codereview.adblockplus.org/29340571/diff/29342098/safari/contentBlock... safari/contentBlocking.js:76: function (result) Nit: I think this variable should be called "error". https://codereview.adblockplus.org/29340571/diff/29342098/safari/contentBlock... safari/contentBlocking.js:76: function (result) Nit: We usually don't put a space before the arguments list. https://codereview.adblockplus.org/29340571/diff/29342098/safari/contentBlock... safari/contentBlocking.js:78: // Safari 9 performs the callback twice under some conditions, first with On 2016/05/17 17:13:06, kzar wrote: > As far as I can tell the behaviour of the callback is something like this: > > - If callback is provided and rules aren't converted to JSON the callback will > first be called with an empty string "" and then an Error. Further calls will > result in a callback of "" followed by a callback of null. > - If the callback is provided and the rules are converted to JSON the callback > will be performed with null on success or Error on error. Further calls with the > same rule string will always result in a callback of null until a call is > performed with a different JSON string. (Even if there was a problem.) > - If the callback isn't provided things generally work as expected, but there's > no longer a way to verify that! > > My only other idea to further improve how things work is to add a flag > `buggySetContentBlocker` that defaults to false. When the callback is performed > with the value of "" the flag is set to true and we start converting the passed > rules to JSON. This adds yet more code however and could make things more > fragile. (It also would mean we are generating the content list twice in a row > for buggy versions and it's already pretty slow when generating it once.) Well, if future versions of Safari won't have that bug, while we still have to target versions with that bug, I suppose we should do something like that. However, when doing so we can simply keep the list of rules around until the callback get's called rather than generating it twice. But I agree, that for now we should leave it like that. https://codereview.adblockplus.org/29340571/diff/29342139/lib/tldjs.js File lib/tldjs.js (right): https://codereview.adblockplus.org/29340571/diff/29342139/lib/tldjs.js#newcode27 lib/tldjs.js:27: exports.getDomain = function (hostname) Nit: Redundant space before argument list. https://codereview.adblockplus.org/29340571/diff/29342139/safari/include.yout... File safari/include.youtube.js (right): https://codereview.adblockplus.org/29340571/diff/29342139/safari/include.yout... safari/include.youtube.js:31: catch (e) { }; Nit: Redundant semicolon. Also I wonder whether we should put those braces on new lines. I think our coding style actually requires it, but I wouldn't insist. (Both, same for the code in safari/ext/content.js)
Patch Set 7 : Addressed further feedback https://codereview.adblockplus.org/29340571/diff/29340572/_locales/en_US/mess... File _locales/en_US/messages.json (left): https://codereview.adblockplus.org/29340571/diff/29340572/_locales/en_US/mess... _locales/en_US/messages.json:124: "safari_content_blocker": { On 2016/05/17 18:35:24, Sebastian Noack wrote: > On 2016/05/17 15:15:36, kzar wrote: > > On 2016/05/12 11:12:22, Sebastian Noack wrote: > > > Why did you remove the message? Also what's about the message to restart > > Safari? > > > > The strings are provided by adblockplusui/locale/*/options.json, no need to > > duplicate them here. > > Those strings aren't imported yet. And even if we add the new options behind a > flag soon, though unlikely before the next release, the strings there won't be > translated as they aren't final yet. The strings are imported, I added "adblockplusui/locale/*/options.json = =*" to metadata.common in Patch Set 1. Would you rather I duplicate them here instead? https://codereview.adblockplus.org/29340571/diff/29340572/metadata.chrome File metadata.chrome (right): https://codereview.adblockplus.org/29340571/diff/29340572/metadata.chrome#new... metadata.chrome:45: lib/adblockplus.js -= abp2blocklist/lib/abp2blocklist.js safari/contentBlocking.js On 2016/05/17 18:35:24, Sebastian Noack wrote: > On 2016/05/17 15:15:36, kzar wrote: > > On 2016/05/12 11:12:22, Sebastian Noack wrote: > > > On 2016/04/19 14:03:49, kzar wrote: > > > > I'm guessing there's a better way to achieve the same thing, any > > suggestions? > > > > > > I agree that it's kinda backwards to remove scripts that are only necessary > > for > > > one platform in the configuration for another platform. However, since the > > > convert_js options take some arguments that need to be given in the end, we > > > cannot append to those options. However, feel free to change the logic in > > > buildtools to make stuff like this more straight-forward. But how it's > > currently > > > implemented this is the only way to do it. > > > > Acknowledged. (Considering time constraints I will leave this as-is for now.) > > Feel free to file an issue though. I like the idea, and perhaps it's something > for Jon. > > FWIW, I think this would be a more reasonable way to specify the options, > wouldn't insist though: > > [convert_js] > lib/adblockplus.js = <input files> > lib/adblockplus.js:module = true > lib/adblockplus.js:source_repo = https://hg.adblockplus.org/adblockpluscore/ There you go https://issues.adblockplus.org/ticket/4047 https://codereview.adblockplus.org/29340571/diff/29340572/options.js File options.js (right): https://codereview.adblockplus.org/29340571/diff/29340572/options.js#newcode158 options.js:158: if (!checkbox.checked) On 2016/05/17 18:35:24, Sebastian Noack wrote: > On 2016/05/17 15:15:38, kzar wrote: > > On 2016/05/12 11:12:22, Sebastian Noack wrote: > > > As discussed with Thomas on the review for the related change to the new > > options > > > page, we explictily decided against persistent logic and agreed that it's > > > sufficient to notify the user only once when he disabled content blockers. > > > > > > If we want to persistently notify the user about this state, the options > page > > is > > > the wrong place to do so. But given that disabling an experimental feature > > that > > > have previously be enabled on Safari can be considered edge case, notifying > > the > > > user once should be sufficient. > > > > I don't understand what you mean by "persistent logic" here. > > > > The behaviour I've implemented displays a red "Please restart Safari" message > > beside the content blocking option when necessary and it handles the various > > edge-cases pretty well. (For example what if the user clicks to enable content > > blocking but there's an exception, the old API isn't disabled and the content > > blocking feature is automatically disabled.) > > At the very least this logic is inconsistent with the new options page, where we > agreed to ignore edge cases. But fine with me, as long as you keep the new > options page in sync with the logic here. Acknowledged. https://codereview.adblockplus.org/29340571/diff/29340572/options.js#newcode544 options.js:544: // Apparently modifying the checked attribute for a checkbox does not On 2016/05/17 18:35:25, Sebastian Noack wrote: > On 2016/05/17 15:15:37, kzar wrote: > > On 2016/05/12 11:12:23, Sebastian Noack wrote: > > > If you want to emulate user actions that's what the click() method is for. > > > However, it would be better to call the change handler explicitly here. > > > > Calling .click() would also toggle the checkbox right? (I guess I could move > the > > checkbox.checked assignment below to force the correct state but it seems kind > > of pointless.) > > > > As for calling the change handler directly here, sounds good to me except I > > can't see an easy way to get to the handler here. Maybe I'm missing something? > > (I guess we could assign an extra property to the element, or assign onchange > > instead of using addEventListener but both of those seem maybe worse than just > > dispatching the change event.) > > I wonder whether the change listener is even necessary. Why don't you simply > move the related logic here? If the preference is changed the code here is > triggered anyway. We also need to call the logic when the checkbox is first initialized. Even if we moved the logic into a separate function and called it here we would struggle to call it when the checkbox is initialized. (We would have to add a key like `afterInit` to initCheckbox and pass the function there too. Hardly seems less work than the existing approach.) https://codereview.adblockplus.org/29340571/diff/29342098/safari/contentBlock... File safari/contentBlocking.js (right): https://codereview.adblockplus.org/29340571/diff/29342098/safari/contentBlock... safari/contentBlocking.js:76: function (result) On 2016/05/17 18:35:25, Sebastian Noack wrote: > Nit: We usually don't put a space before the arguments list. Done. https://codereview.adblockplus.org/29340571/diff/29342098/safari/contentBlock... safari/contentBlocking.js:76: function (result) On 2016/05/17 18:35:25, Sebastian Noack wrote: > Nit: I think this variable should be called "error". Done. https://codereview.adblockplus.org/29340571/diff/29342098/safari/contentBlock... safari/contentBlocking.js:78: // Safari 9 performs the callback twice under some conditions, first with On 2016/05/17 18:35:25, Sebastian Noack wrote: > On 2016/05/17 17:13:06, kzar wrote: > > As far as I can tell the behaviour of the callback is something like this: > > > > - If callback is provided and rules aren't converted to JSON the callback > will > > first be called with an empty string "" and then an Error. Further calls will > > result in a callback of "" followed by a callback of null. > > - If the callback is provided and the rules are converted to JSON the > callback > > will be performed with null on success or Error on error. Further calls with > the > > same rule string will always result in a callback of null until a call is > > performed with a different JSON string. (Even if there was a problem.) > > - If the callback isn't provided things generally work as expected, but > there's > > no longer a way to verify that! > > > > My only other idea to further improve how things work is to add a flag > > `buggySetContentBlocker` that defaults to false. When the callback is > performed > > with the value of "" the flag is set to true and we start converting the > passed > > rules to JSON. This adds yet more code however and could make things more > > fragile. (It also would mean we are generating the content list twice in a row > > for buggy versions and it's already pretty slow when generating it once.) > > Well, if future versions of Safari won't have that bug, while we still have to > target versions with that bug, I suppose we should do something like that. > However, when doing so we can simply keep the list of rules around until the > callback get's called rather than generating it twice. But I agree, that for now > we should leave it like that. Acknowledged. https://codereview.adblockplus.org/29340571/diff/29342139/lib/tldjs.js File lib/tldjs.js (right): https://codereview.adblockplus.org/29340571/diff/29342139/lib/tldjs.js#newcode27 lib/tldjs.js:27: exports.getDomain = function (hostname) On 2016/05/17 18:35:26, Sebastian Noack wrote: > Nit: Redundant space before argument list. Done. https://codereview.adblockplus.org/29340571/diff/29342139/safari/include.yout... File safari/include.youtube.js (right): https://codereview.adblockplus.org/29340571/diff/29342139/safari/include.yout... safari/include.youtube.js:31: catch (e) { }; On 2016/05/17 18:35:26, Sebastian Noack wrote: > Nit: Redundant semicolon. > > Also I wonder whether we should put those braces on new lines. I think our > coding style actually requires it, but I wouldn't insist. > > (Both, same for the code in safari/ext/content.js) Done.
https://codereview.adblockplus.org/29340571/diff/29340572/_locales/en_US/mess... File _locales/en_US/messages.json (left): https://codereview.adblockplus.org/29340571/diff/29340572/_locales/en_US/mess... _locales/en_US/messages.json:124: "safari_content_blocker": { On 2016/05/17 19:20:21, kzar wrote: > On 2016/05/17 18:35:24, Sebastian Noack wrote: > > On 2016/05/17 15:15:36, kzar wrote: > > > On 2016/05/12 11:12:22, Sebastian Noack wrote: > > > > Why did you remove the message? Also what's about the message to restart > > > Safari? > > > > > > The strings are provided by adblockplusui/locale/*/options.json, no need to > > > duplicate them here. > > > > Those strings aren't imported yet. And even if we add the new options behind a > > flag soon, though unlikely before the next release, the strings there won't be > > translated as they aren't final yet. > > The strings are imported, I added "adblockplusui/locale/*/options.json = =*" to > metadata.common in Patch Set 1. Would you rather I duplicate them here instead? I see. But still the strings there won't be translated. So yes, we have to duplicate them here. https://codereview.adblockplus.org/29340571/diff/29340572/metadata.chrome File metadata.chrome (right): https://codereview.adblockplus.org/29340571/diff/29340572/metadata.chrome#new... metadata.chrome:45: lib/adblockplus.js -= abp2blocklist/lib/abp2blocklist.js safari/contentBlocking.js On 2016/05/17 19:20:21, kzar wrote: > On 2016/05/17 18:35:24, Sebastian Noack wrote: > > On 2016/05/17 15:15:36, kzar wrote: > > > On 2016/05/12 11:12:22, Sebastian Noack wrote: > > > > On 2016/04/19 14:03:49, kzar wrote: > > > > > I'm guessing there's a better way to achieve the same thing, any > > > suggestions? > > > > > > > > I agree that it's kinda backwards to remove scripts that are only > necessary > > > for > > > > one platform in the configuration for another platform. However, since the > > > > convert_js options take some arguments that need to be given in the end, > we > > > > cannot append to those options. However, feel free to change the logic in > > > > buildtools to make stuff like this more straight-forward. But how it's > > > currently > > > > implemented this is the only way to do it. > > > > > > Acknowledged. (Considering time constraints I will leave this as-is for > now.) > > > > Feel free to file an issue though. I like the idea, and perhaps it's something > > for Jon. > > > > FWIW, I think this would be a more reasonable way to specify the options, > > wouldn't insist though: > > > > [convert_js] > > lib/adblockplus.js = <input files> > > lib/adblockplus.js:module = true > > lib/adblockplus.js:source_repo = https://hg.adblockplus.org/adblockpluscore/ > > There you go https://issues.adblockplus.org/ticket/4047 Thanks! https://codereview.adblockplus.org/29340571/diff/29340572/options.js File options.js (right): https://codereview.adblockplus.org/29340571/diff/29340572/options.js#newcode544 options.js:544: // Apparently modifying the checked attribute for a checkbox does not On 2016/05/17 19:20:21, kzar wrote: > On 2016/05/17 18:35:25, Sebastian Noack wrote: > > On 2016/05/17 15:15:37, kzar wrote: > > > On 2016/05/12 11:12:23, Sebastian Noack wrote: > > > > If you want to emulate user actions that's what the click() method is for. > > > > However, it would be better to call the change handler explicitly here. > > > > > > Calling .click() would also toggle the checkbox right? (I guess I could move > > the > > > checkbox.checked assignment below to force the correct state but it seems > kind > > > of pointless.) > > > > > > As for calling the change handler directly here, sounds good to me except I > > > can't see an easy way to get to the handler here. Maybe I'm missing > something? > > > (I guess we could assign an extra property to the element, or assign > onchange > > > instead of using addEventListener but both of those seem maybe worse than > just > > > dispatching the change event.) > > > > I wonder whether the change listener is even necessary. Why don't you simply > > move the related logic here? If the preference is changed the code here is > > triggered anyway. > > We also need to call the logic when the checkbox is first initialized. Even if > we moved the logic into a separate function and called it here we would struggle > to call it when the checkbox is initialized. (We would have to add a key like > `afterInit` to initCheckbox and pass the function there too. Hardly seems less > work than the existing approach.) Well, in the new options page we call the same function that is called on preference change also when initially retrieving the preferences in order to initialize the checkboxes. Couldn't you do the same here?
Patch Set 8 : Don't reuse strings from new options page https://codereview.adblockplus.org/29340571/diff/29340572/_locales/en_US/mess... File _locales/en_US/messages.json (left): https://codereview.adblockplus.org/29340571/diff/29340572/_locales/en_US/mess... _locales/en_US/messages.json:124: "safari_content_blocker": { On 2016/05/18 07:03:29, Sebastian Noack wrote: > On 2016/05/17 19:20:21, kzar wrote: > > On 2016/05/17 18:35:24, Sebastian Noack wrote: > > > On 2016/05/17 15:15:36, kzar wrote: > > > > On 2016/05/12 11:12:22, Sebastian Noack wrote: > > > > > Why did you remove the message? Also what's about the message to restart > > > > Safari? > > > > > > > > The strings are provided by adblockplusui/locale/*/options.json, no need > to > > > > duplicate them here. > > > > > > Those strings aren't imported yet. And even if we add the new options behind > a > > > flag soon, though unlikely before the next release, the strings there won't > be > > > translated as they aren't final yet. > > > > The strings are imported, I added "adblockplusui/locale/*/options.json = =*" > to > > metadata.common in Patch Set 1. Would you rather I duplicate them here > instead? > > I see. But still the strings there won't be translated. So yes, we have to > duplicate them here. Done. https://codereview.adblockplus.org/29340571/diff/29340572/options.js File options.js (right): https://codereview.adblockplus.org/29340571/diff/29340572/options.js#newcode544 options.js:544: // Apparently modifying the checked attribute for a checkbox does not On 2016/05/18 07:03:29, Sebastian Noack wrote: > On 2016/05/17 19:20:21, kzar wrote: > > On 2016/05/17 18:35:25, Sebastian Noack wrote: > > > On 2016/05/17 15:15:37, kzar wrote: > > > > On 2016/05/12 11:12:23, Sebastian Noack wrote: > > > > > If you want to emulate user actions that's what the click() method is > for. > > > > > However, it would be better to call the change handler explicitly here. > > > > > > > > Calling .click() would also toggle the checkbox right? (I guess I could > move > > > the > > > > checkbox.checked assignment below to force the correct state but it seems > > kind > > > > of pointless.) > > > > > > > > As for calling the change handler directly here, sounds good to me except > I > > > > can't see an easy way to get to the handler here. Maybe I'm missing > > something? > > > > (I guess we could assign an extra property to the element, or assign > > onchange > > > > instead of using addEventListener but both of those seem maybe worse than > > just > > > > dispatching the change event.) > > > > > > I wonder whether the change listener is even necessary. Why don't you simply > > > move the related logic here? If the preference is changed the code here is > > > triggered anyway. > > > > We also need to call the logic when the checkbox is first initialized. Even if > > we moved the logic into a separate function and called it here we would > struggle > > to call it when the checkbox is initialized. (We would have to add a key like > > `afterInit` to initCheckbox and pass the function there too. Hardly seems less > > work than the existing approach.) > > Well, in the new options page we call the same function that is called on > preference change also when initially retrieving the preferences in order to > initialize the checkboxes. Couldn't you do the same here? Yes, I already do that here.
Patch Set 9 : Avoid type conversion when handling Safari bug
https://codereview.adblockplus.org/29340571/diff/29340572/options.js File options.js (right): https://codereview.adblockplus.org/29340571/diff/29340572/options.js#newcode544 options.js:544: // Apparently modifying the checked attribute for a checkbox does not On 2016/05/18 08:02:48, kzar wrote: > On 2016/05/18 07:03:29, Sebastian Noack wrote: > > On 2016/05/17 19:20:21, kzar wrote: > > > On 2016/05/17 18:35:25, Sebastian Noack wrote: > > > > On 2016/05/17 15:15:37, kzar wrote: > > > > > On 2016/05/12 11:12:23, Sebastian Noack wrote: > > > > > > If you want to emulate user actions that's what the click() method is > > for. > > > > > > However, it would be better to call the change handler explicitly > here. > > > > > > > > > > Calling .click() would also toggle the checkbox right? (I guess I could > > move > > > > the > > > > > checkbox.checked assignment below to force the correct state but it > seems > > > kind > > > > > of pointless.) > > > > > > > > > > As for calling the change handler directly here, sounds good to me > except > > I > > > > > can't see an easy way to get to the handler here. Maybe I'm missing > > > something? > > > > > (I guess we could assign an extra property to the element, or assign > > > onchange > > > > > instead of using addEventListener but both of those seem maybe worse > than > > > just > > > > > dispatching the change event.) > > > > > > > > I wonder whether the change listener is even necessary. Why don't you > simply > > > > move the related logic here? If the preference is changed the code here is > > > > triggered anyway. > > > > > > We also need to call the logic when the checkbox is first initialized. Even > if > > > we moved the logic into a separate function and called it here we would > > struggle > > > to call it when the checkbox is initialized. (We would have to add a key > like > > > `afterInit` to initCheckbox and pass the function there too. Hardly seems > less > > > work than the existing approach.) > > > > Well, in the new options page we call the same function that is called on > > preference change also when initially retrieving the preferences in order to > > initialize the checkboxes. Couldn't you do the same here? > > Yes, I already do that here. But why can't you put the logic simply here then instead of passing a callback to initCheckbox() and emulating a "change" even here? Note that we already have logic that depends on the preference name above. https://codereview.adblockplus.org/29340571/diff/29342686/safari/contentBlock... File safari/contentBlocking.js (right): https://codereview.adblockplus.org/29340571/diff/29342686/safari/contentBlock... safari/contentBlocking.js:84: if (error === "") We always use == unless it makes a difference, as per Mozilla's coding practices.
Patch Set 10 : Don't avoid type conversion https://codereview.adblockplus.org/29340571/diff/29340572/options.js File options.js (right): https://codereview.adblockplus.org/29340571/diff/29340572/options.js#newcode544 options.js:544: // Apparently modifying the checked attribute for a checkbox does not On 2016/05/18 08:13:37, Sebastian Noack wrote: > On 2016/05/18 08:02:48, kzar wrote: > > On 2016/05/18 07:03:29, Sebastian Noack wrote: > > > On 2016/05/17 19:20:21, kzar wrote: > > > > On 2016/05/17 18:35:25, Sebastian Noack wrote: > > > > > On 2016/05/17 15:15:37, kzar wrote: > > > > > > On 2016/05/12 11:12:23, Sebastian Noack wrote: > > > > > > > If you want to emulate user actions that's what the click() method > is > > > for. > > > > > > > However, it would be better to call the change handler explicitly > > here. > > > > > > > > > > > > Calling .click() would also toggle the checkbox right? (I guess I > could > > > move > > > > > the > > > > > > checkbox.checked assignment below to force the correct state but it > > seems > > > > kind > > > > > > of pointless.) > > > > > > > > > > > > As for calling the change handler directly here, sounds good to me > > except > > > I > > > > > > can't see an easy way to get to the handler here. Maybe I'm missing > > > > something? > > > > > > (I guess we could assign an extra property to the element, or assign > > > > onchange > > > > > > instead of using addEventListener but both of those seem maybe worse > > than > > > > just > > > > > > dispatching the change event.) > > > > > > > > > > I wonder whether the change listener is even necessary. Why don't you > > simply > > > > > move the related logic here? If the preference is changed the code here > is > > > > > triggered anyway. > > > > > > > > We also need to call the logic when the checkbox is first initialized. > Even > > if > > > > we moved the logic into a separate function and called it here we would > > > struggle > > > > to call it when the checkbox is initialized. (We would have to add a key > > like > > > > `afterInit` to initCheckbox and pass the function there too. Hardly seems > > less > > > > work than the existing approach.) > > > > > > Well, in the new options page we call the same function that is called on > > > preference change also when initially retrieving the preferences in order to > > > initialize the checkboxes. Couldn't you do the same here? > > > > Yes, I already do that here. > > But why can't you put the logic simply here then instead of passing a callback > to initCheckbox() and emulating a "change" even here? Note that we already have > logic that depends on the preference name above. Oh I finally understand, I think you mean that we should call `onPrefMessage` when initializing the checkboxes. I misunderstood, I thought by "the same function" you meant `onChange`. We don't call the `onPrefMessage` when the checkboxes are initialized so far. Well I really think the change you suggest here would make the code more confusing without any real benefit. It would be less clear in edge cases what should happen in `onPrefMessage`, for example with normalizing the key and value for the "notifications_ignoredcategories" preference. (Note that logic that depends on the preference name as it stands adjusts the preference name in some cases, so we can't rely on the name until afterwards.) It would also add slightly to the delay after clicking the checkbox before the "safari.contentBlockingActive" message comes back and the "Please restart Safari" message is shown. As it is we immediately send that message when the user clicks, instead of waiting for the `prefs.toggle` message to be sent and then for the `prefs.respond` message to come back in. https://codereview.adblockplus.org/29340571/diff/29342686/safari/contentBlock... File safari/contentBlocking.js (right): https://codereview.adblockplus.org/29340571/diff/29342686/safari/contentBlock... safari/contentBlocking.js:84: if (error === "") On 2016/05/18 08:13:37, Sebastian Noack wrote: > We always use == unless it makes a difference, as per Mozilla's coding > practices. I wrongly thought that `(new Error("")).toString()` would result in an empty string but actually it gives "Error". So I guess you're right that this can never make a difference. Done.
https://codereview.adblockplus.org/29340571/diff/29340572/options.js File options.js (right): https://codereview.adblockplus.org/29340571/diff/29340572/options.js#newcode544 options.js:544: // Apparently modifying the checked attribute for a checkbox does not On 2016/05/18 09:28:47, kzar wrote: > On 2016/05/18 08:13:37, Sebastian Noack wrote: > > On 2016/05/18 08:02:48, kzar wrote: > > > On 2016/05/18 07:03:29, Sebastian Noack wrote: > > > > On 2016/05/17 19:20:21, kzar wrote: > > > > > On 2016/05/17 18:35:25, Sebastian Noack wrote: > > > > > > On 2016/05/17 15:15:37, kzar wrote: > > > > > > > On 2016/05/12 11:12:23, Sebastian Noack wrote: > > > > > > > > If you want to emulate user actions that's what the click() method > > is > > > > for. > > > > > > > > However, it would be better to call the change handler explicitly > > > here. > > > > > > > > > > > > > > Calling .click() would also toggle the checkbox right? (I guess I > > could > > > > move > > > > > > the > > > > > > > checkbox.checked assignment below to force the correct state but it > > > seems > > > > > kind > > > > > > > of pointless.) > > > > > > > > > > > > > > As for calling the change handler directly here, sounds good to me > > > except > > > > I > > > > > > > can't see an easy way to get to the handler here. Maybe I'm missing > > > > > something? > > > > > > > (I guess we could assign an extra property to the element, or assign > > > > > onchange > > > > > > > instead of using addEventListener but both of those seem maybe worse > > > than > > > > > just > > > > > > > dispatching the change event.) > > > > > > > > > > > > I wonder whether the change listener is even necessary. Why don't you > > > simply > > > > > > move the related logic here? If the preference is changed the code > here > > is > > > > > > triggered anyway. > > > > > > > > > > We also need to call the logic when the checkbox is first initialized. > > Even > > > if > > > > > we moved the logic into a separate function and called it here we would > > > > struggle > > > > > to call it when the checkbox is initialized. (We would have to add a key > > > like > > > > > `afterInit` to initCheckbox and pass the function there too. Hardly > seems > > > less > > > > > work than the existing approach.) > > > > > > > > Well, in the new options page we call the same function that is called on > > > > preference change also when initially retrieving the preferences in order > to > > > > initialize the checkboxes. Couldn't you do the same here? > > > > > > Yes, I already do that here. > > > > But why can't you put the logic simply here then instead of passing a callback > > to initCheckbox() and emulating a "change" even here? Note that we already > have > > logic that depends on the preference name above. > > Oh I finally understand, I think you mean that we should call `onPrefMessage` > when initializing the checkboxes. I misunderstood, I thought by "the same > function" you meant `onChange`. We don't call the `onPrefMessage` when the > checkboxes are initialized so far. > > Well I really think the change you suggest here would make the code more > confusing without any real benefit. It would be less clear in edge cases what > should happen in `onPrefMessage`, for example with normalizing the key and value > for the "notifications_ignoredcategories" preference. (Note that logic that > depends on the preference name as it stands adjusts the preference name in some > cases, so we can't rely on the name until afterwards.) > > It would also add slightly to the delay after clicking the checkbox before the > "safari.contentBlockingActive" message comes back and the "Please restart > Safari" message is shown. As it is we immediately send that message when the > user clicks, instead of waiting for the `prefs.toggle` message to be sent and > then for the `prefs.respond` message to come back in. I don't see how that makes anything more confusing. In fact, I think the way you have it implemented now is quite confusing. You introduced yet another method to respond to preference changes, which however is by design only triggered by user gesture, and then you work around that by emulating user gesture. That is certainly backwards. https://codereview.adblockplus.org/29340571/diff/29342686/safari/contentBlock... File safari/contentBlocking.js (right): https://codereview.adblockplus.org/29340571/diff/29342686/safari/contentBlock... safari/contentBlocking.js:84: if (error === "") On 2016/05/18 09:28:47, kzar wrote: > On 2016/05/18 08:13:37, Sebastian Noack wrote: > > We always use == unless it makes a difference, as per Mozilla's coding > > practices. > > I wrongly thought that `(new Error("")).toString()` would result in an empty > string but actually it gives "Error". So I guess you're right that this can > never make a difference. Done. Even if error.toString() returns "", error == "" would still be false if error is an object.
https://codereview.adblockplus.org/29340571/diff/29342686/safari/contentBlock... File safari/contentBlocking.js (right): https://codereview.adblockplus.org/29340571/diff/29342686/safari/contentBlock... safari/contentBlocking.js:84: if (error === "") On 2016/05/18 10:07:24, Sebastian Noack wrote: > On 2016/05/18 09:28:47, kzar wrote: > > On 2016/05/18 08:13:37, Sebastian Noack wrote: > > > We always use == unless it makes a difference, as per Mozilla's coding > > > practices. > > > > I wrongly thought that `(new Error("")).toString()` would result in an empty > > string but actually it gives "Error". So I guess you're right that this can > > never make a difference. Done. > > Even if error.toString() returns "", error == "" would still be false if error > is an object. Never mind, you are actually right that == calls the toString() method of objects. But yeah in the case here it still doesn't matter.
Patch Set 11 : Remove onChange logic for checkboxes https://codereview.adblockplus.org/29340571/diff/29340572/options.js File options.js (right): https://codereview.adblockplus.org/29340571/diff/29340572/options.js#newcode544 options.js:544: // Apparently modifying the checked attribute for a checkbox does not On 2016/05/18 10:07:24, Sebastian Noack wrote: > On 2016/05/18 09:28:47, kzar wrote: > > On 2016/05/18 08:13:37, Sebastian Noack wrote: > > > On 2016/05/18 08:02:48, kzar wrote: > > > > On 2016/05/18 07:03:29, Sebastian Noack wrote: > > > > > On 2016/05/17 19:20:21, kzar wrote: > > > > > > On 2016/05/17 18:35:25, Sebastian Noack wrote: > > > > > > > On 2016/05/17 15:15:37, kzar wrote: > > > > > > > > On 2016/05/12 11:12:23, Sebastian Noack wrote: > > > > > > > > > If you want to emulate user actions that's what the click() > method > > > is > > > > > for. > > > > > > > > > However, it would be better to call the change handler > explicitly > > > > here. > > > > > > > > > > > > > > > > Calling .click() would also toggle the checkbox right? (I guess I > > > could > > > > > move > > > > > > > the > > > > > > > > checkbox.checked assignment below to force the correct state but > it > > > > seems > > > > > > kind > > > > > > > > of pointless.) > > > > > > > > > > > > > > > > As for calling the change handler directly here, sounds good to me > > > > except > > > > > I > > > > > > > > can't see an easy way to get to the handler here. Maybe I'm > missing > > > > > > something? > > > > > > > > (I guess we could assign an extra property to the element, or > assign > > > > > > onchange > > > > > > > > instead of using addEventListener but both of those seem maybe > worse > > > > than > > > > > > just > > > > > > > > dispatching the change event.) > > > > > > > > > > > > > > I wonder whether the change listener is even necessary. Why don't > you > > > > simply > > > > > > > move the related logic here? If the preference is changed the code > > here > > > is > > > > > > > triggered anyway. > > > > > > > > > > > > We also need to call the logic when the checkbox is first initialized. > > > Even > > > > if > > > > > > we moved the logic into a separate function and called it here we > would > > > > > struggle > > > > > > to call it when the checkbox is initialized. (We would have to add a > key > > > > like > > > > > > `afterInit` to initCheckbox and pass the function there too. Hardly > > seems > > > > less > > > > > > work than the existing approach.) > > > > > > > > > > Well, in the new options page we call the same function that is called > on > > > > > preference change also when initially retrieving the preferences in > order > > to > > > > > initialize the checkboxes. Couldn't you do the same here? > > > > > > > > Yes, I already do that here. > > > > > > But why can't you put the logic simply here then instead of passing a > callback > > > to initCheckbox() and emulating a "change" even here? Note that we already > > have > > > logic that depends on the preference name above. > > > > Oh I finally understand, I think you mean that we should call `onPrefMessage` > > when initializing the checkboxes. I misunderstood, I thought by "the same > > function" you meant `onChange`. We don't call the `onPrefMessage` when the > > checkboxes are initialized so far. > > > > Well I really think the change you suggest here would make the code more > > confusing without any real benefit. It would be less clear in edge cases what > > should happen in `onPrefMessage`, for example with normalizing the key and > value > > for the "notifications_ignoredcategories" preference. (Note that logic that > > depends on the preference name as it stands adjusts the preference name in > some > > cases, so we can't rely on the name until afterwards.) > > > > It would also add slightly to the delay after clicking the checkbox before the > > "safari.contentBlockingActive" message comes back and the "Please restart > > Safari" message is shown. As it is we immediately send that message when the > > user clicks, instead of waiting for the `prefs.toggle` message to be sent and > > then for the `prefs.respond` message to come back in. > > I don't see how that makes anything more confusing. In fact, I think the way you > have it implemented now is quite confusing. You introduced yet another method to > respond to preference changes, which however is by design only triggered by user > gesture, and then you work around that by emulating user gesture. That is > certainly backwards. Done.
https://codereview.adblockplus.org/29340571/diff/29342736/metadata.common File metadata.common (right): https://codereview.adblockplus.org/29340571/diff/29342736/metadata.common#new... metadata.common:151: adblockplusui/locale/*/firstRun.json = =* Nit: It seems you removed the new line at the end of the file. https://codereview.adblockplus.org/29340571/diff/29342736/options.js File options.js (right): https://codereview.adblockplus.org/29340571/diff/29342736/options.js#newcode248 options.js:248: key = key || id; Note that when we retrieve the checkbox later in onPrefMessage() we assume the key and id to be the same, which will fail for "notifications_ignoredcategories". So perhaps remove that logic here as well and just rename the ID in the HTML? https://codereview.adblockplus.org/29340571/diff/29342736/options.js#newcode513 options.js:513: function (contentBlockingActive) Nit: Redundant space before arguments list. https://codereview.adblockplus.org/29340571/diff/29342736/safari/ext/content.js File safari/ext/content.js (right): https://codereview.adblockplus.org/29340571/diff/29342736/safari/ext/content.... safari/ext/content.js:44: { } Nit: The closing brace should go on a new line as well. https://codereview.adblockplus.org/29340571/diff/29342736/safari/ext/content.... safari/ext/content.js:157: }, 0); Nit: 0 is the default for the second argument to setTimeout(). So we usually omit it. https://codereview.adblockplus.org/29340571/diff/29342736/safari/include.yout... File safari/include.youtube.js (right): https://codereview.adblockplus.org/29340571/diff/29342736/safari/include.yout... safari/include.youtube.js:32: { } Nit: The closing brace should go on a new line as well.
Patch Set 12 : Addressed Nits https://codereview.adblockplus.org/29340571/diff/29342736/metadata.common File metadata.common (right): https://codereview.adblockplus.org/29340571/diff/29342736/metadata.common#new... metadata.common:151: adblockplusui/locale/*/firstRun.json = =* On 2016/05/18 11:16:32, Sebastian Noack wrote: > Nit: It seems you removed the new line at the end of the file. Done. https://codereview.adblockplus.org/29340571/diff/29342736/options.js File options.js (right): https://codereview.adblockplus.org/29340571/diff/29342736/options.js#newcode248 options.js:248: key = key || id; On 2016/05/18 11:16:32, Sebastian Noack wrote: > Note that when we retrieve the checkbox later in onPrefMessage() we assume the > key and id to be the same, which will fail for > "notifications_ignoredcategories". So perhaps remove that logic here as well and > just rename the ID in the HTML? Actually onPrefMessage already handles that, changing the key to shouldShowNotifications. https://codereview.adblockplus.org/29340571/diff/29342736/options.js#newcode513 options.js:513: function (contentBlockingActive) On 2016/05/18 11:16:32, Sebastian Noack wrote: > Nit: Redundant space before arguments list. Done. https://codereview.adblockplus.org/29340571/diff/29342736/safari/ext/content.js File safari/ext/content.js (right): https://codereview.adblockplus.org/29340571/diff/29342736/safari/ext/content.... safari/ext/content.js:44: { } On 2016/05/18 11:16:33, Sebastian Noack wrote: > Nit: The closing brace should go on a new line as well. Done. https://codereview.adblockplus.org/29340571/diff/29342736/safari/ext/content.... safari/ext/content.js:157: }, 0); On 2016/05/18 11:16:33, Sebastian Noack wrote: > Nit: 0 is the default for the second argument to setTimeout(). So we usually > omit it. Done. https://codereview.adblockplus.org/29340571/diff/29342736/safari/include.yout... File safari/include.youtube.js (right): https://codereview.adblockplus.org/29340571/diff/29342736/safari/include.yout... safari/include.youtube.js:32: { } On 2016/05/18 11:16:33, Sebastian Noack wrote: > Nit: The closing brace should go on a new line as well. Done.
LGTM https://codereview.adblockplus.org/29340571/diff/29342736/options.js File options.js (right): https://codereview.adblockplus.org/29340571/diff/29342736/options.js#newcode248 options.js:248: key = key || id; On 2016/05/18 11:32:07, kzar wrote: > On 2016/05/18 11:16:32, Sebastian Noack wrote: > > Note that when we retrieve the checkbox later in onPrefMessage() we assume the > > key and id to be the same, which will fail for > > "notifications_ignoredcategories". So perhaps remove that logic here as well > and > > just rename the ID in the HTML? > > Actually onPrefMessage already handles that, changing the key to > shouldShowNotifications. Hm, seems that logic could be simplified altogether then, by just adapting the HTML ID. But up to you, as it's not directly related to your changes. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
