Issue 11533106: Prepared adblockplus for Safari (Closed)
|
Created:
Sept. 2, 2013, 5:56 p.m. by Sebastian Noack Modified:
Oct. 31, 2013, 4:29 p.m. Visibility:
Public. |
DescriptionPrepared adblockplus for Safari
Patch Set 1 #Patch Set 2 : Prepared adblockplus for Safari #
Total comments: 9
Patch Set 3 : Changed text of legacy Safari warning as discussed #
Total comments: 1
Patch Set 4 : Fixed broken path to CSS for firefox #
Total comments: 23
Patch Set 5 : #
Total comments: 8
Patch Set 6 : Added another js file for safari and chrome #Patch Set 7 : Fixed some coding style issues #Patch Set 8 : Rebased on upstream #
Total comments: 9
Patch Set 9 : Addressed comments #Patch Set 10 : Made first run page always (also in FF) generated #
MessagesTotal messages: 38
http://codereview.adblockplus.org/11533106/diff/3001/chrome/content/ui/firstR... File chrome/content/ui/firstRun.html (right): http://codereview.adblockplus.org/11533106/diff/3001/chrome/content/ui/firstR... chrome/content/ui/firstRun.html:25: <!-- only required for the ports --> As discussed, this is the part I'm not very happy with. Some background for Wladimir: We cannot use require from the background page in Safari as we do it in Opera/Safari, so the required files need to be included here as well. This will probably cause errors in Firefox. Sebastian and I couldn't come up with great alternatives. Injecting the scripts from JS didn't work, IIRC. Got an idea, Wladimir? http://codereview.adblockplus.org/11533106/diff/3001/chrome/content/ui/i18n.js File chrome/content/ui/i18n.js (right): http://codereview.adblockplus.org/11533106/diff/3001/chrome/content/ui/i18n.j... chrome/content/ui/i18n.js:18: var i18n = (window.ext && ext.i18n) || (window.chrome && chrome.i18n); Why not make ext.i18n point to chrome.i18n in ext.js for Chrome? That way we'd have all the port specific logic in ext.js, and only differentiate between Firefox and the ports here. http://codereview.adblockplus.org/11533106/diff/3001/chrome/locale/en-US/firs... File chrome/locale/en-US/firstRun.properties (right): http://codereview.adblockplus.org/11533106/diff/3001/chrome/locale/en-US/firs... chrome/locale/en-US/firstRun.properties:4: firstRun_legacySafariWarning=The version of Safari that you are using is pretty old and full of bugs. So besides other issues, Adblock Plus might not work correctly or will even break some websites. We strongly recommend to <strong>update to Safari 6 (OSX 10.8) or higher</strong> if possible, or to use the latest version of an other browser, like Firefox or Google Chrome. I'd put this a bit differently. It's probably sufficient to say that this version is unsupported, that the oldest supported version is Safari 6 and that it comes with OSX 10.8. Urging users to update might not help much, a browser update is easier done than an OS upgrade, which is what's necessary to get a newer Safari version.
http://codereview.adblockplus.org/11533106/diff/3001/chrome/content/ui/firstR... File chrome/content/ui/firstRun.html (right): http://codereview.adblockplus.org/11533106/diff/3001/chrome/content/ui/firstR... chrome/content/ui/firstRun.html:25: <!-- only required for the ports --> On 2013/09/06 08:08:24, Felix H. Dahlke wrote: > As discussed, this is the part I'm not very happy with. > > Some background for Wladimir: We cannot use require from the background page in > Safari as we do it in Opera/Safari, so the required files need to be included > here as well. This will probably cause errors in Firefox. > > Sebastian and I couldn't come up with great alternatives. Injecting the scripts > from JS didn't work, IIRC. Got an idea, Wladimir? I'm neither happy with that. I tried to use an inline script tag, that injects those scripts, only when we aren't on Firefox. But for security reasons extensions can not use inline script tags in Chrome. But note that utils.js, which as well doesn't exist in the Firefox version, was already there before. So adding a few more scripts here that only needed by the ports, shouldn't make it worse. http://codereview.adblockplus.org/11533106/diff/3001/chrome/content/ui/i18n.js File chrome/content/ui/i18n.js (right): http://codereview.adblockplus.org/11533106/diff/3001/chrome/content/ui/i18n.j... chrome/content/ui/i18n.js:18: var i18n = (window.ext && ext.i18n) || (window.chrome && chrome.i18n); On 2013/09/06 08:08:24, Felix H. Dahlke wrote: > Why not make ext.i18n point to chrome.i18n in ext.js for Chrome? That way we'd > have all the port specific logic in ext.js, and only differentiate between > Firefox and the ports here. That's what I do. The only reason I check for both here is compatibility. So we can merge this changes before merging my changes on the adblockpluschrome repository. http://codereview.adblockplus.org/11533106/diff/3001/chrome/locale/en-US/firs... File chrome/locale/en-US/firstRun.properties (right): http://codereview.adblockplus.org/11533106/diff/3001/chrome/locale/en-US/firs... chrome/locale/en-US/firstRun.properties:4: firstRun_legacySafariWarning=The version of Safari that you are using is pretty old and full of bugs. So besides other issues, Adblock Plus might not work correctly or will even break some websites. We strongly recommend to <strong>update to Safari 6 (OSX 10.8) or higher</strong> if possible, or to use the latest version of an other browser, like Firefox or Google Chrome. On 2013/09/06 08:08:24, Felix H. Dahlke wrote: > I'd put this a bit differently. It's probably sufficient to say that this > version is unsupported, that the oldest supported version is Safari 6 and that > it comes with OSX 10.8. > > Urging users to update might not help much, a browser update is easier done than > an OS upgrade, which is what's necessary to get a newer Safari version. That's why I alternatively suggest to use Firefox or Google Chrome instead, which are both available for OSX 10.6 and Windows XP or higher. So almost all users that see that message could just migrate to the latest version of Firefox or Chrome.
LGTM, with a suggestion for the warning text http://codereview.adblockplus.org/11533106/diff/3001/chrome/content/ui/firstR... File chrome/content/ui/firstRun.html (right): http://codereview.adblockplus.org/11533106/diff/3001/chrome/content/ui/firstR... chrome/content/ui/firstRun.html:25: <!-- only required for the ports --> On 2013/09/06 09:05:37, sebastian wrote: > On 2013/09/06 08:08:24, Felix H. Dahlke wrote: > > As discussed, this is the part I'm not very happy with. > > > > Some background for Wladimir: We cannot use require from the background page > in > > Safari as we do it in Opera/Safari, so the required files need to be included > > here as well. This will probably cause errors in Firefox. > > > > Sebastian and I couldn't come up with great alternatives. Injecting the > scripts > > from JS didn't work, IIRC. Got an idea, Wladimir? > > I'm neither happy with that. I tried to use an inline script tag, that injects > those scripts, only when we aren't on Firefox. But for security reasons > extensions can not use inline script tags in Chrome. > > But note that utils.js, which as well doesn't exist in the Firefox version, was > already there before. So adding a few more scripts here that only needed by the > ports, shouldn't make it worse. Oh, wasn't aware of that. If we're not making things worse here, it seems fine. Unless we can come up with a better solution. http://codereview.adblockplus.org/11533106/diff/3001/chrome/locale/en-US/firs... File chrome/locale/en-US/firstRun.properties (right): http://codereview.adblockplus.org/11533106/diff/3001/chrome/locale/en-US/firs... chrome/locale/en-US/firstRun.properties:4: firstRun_legacySafariWarning=The version of Safari that you are using is pretty old and full of bugs. So besides other issues, Adblock Plus might not work correctly or will even break some websites. We strongly recommend to <strong>update to Safari 6 (OSX 10.8) or higher</strong> if possible, or to use the latest version of an other browser, like Firefox or Google Chrome. On 2013/09/06 09:05:37, sebastian wrote: > On 2013/09/06 08:08:24, Felix H. Dahlke wrote: > > I'd put this a bit differently. It's probably sufficient to say that this > > version is unsupported, that the oldest supported version is Safari 6 and that > > it comes with OSX 10.8. > > > > Urging users to update might not help much, a browser update is easier done > than > > an OS upgrade, which is what's necessary to get a newer Safari version. > > That's why I alternatively suggest to use Firefox or Google Chrome instead, > which are both available for OSX 10.6 and Windows XP or higher. So almost all > users that see that message could just migrate to the latest version of Firefox > or Chrome. That's true. How about making it more polite though? :D Here's my suggestion: You are using an old version of Safari which is not supported by Adblock Plus, so it might not work correctly. We strongly recommend to either update to Safari 6 (available in Mac OS X 10.8, Mountain Lion) or to use another browser, like Firefox.
http://codereview.adblockplus.org/11533106/diff/3001/chrome/locale/en-US/firs... File chrome/locale/en-US/firstRun.properties (right): http://codereview.adblockplus.org/11533106/diff/3001/chrome/locale/en-US/firs... chrome/locale/en-US/firstRun.properties:4: firstRun_legacySafariWarning=The version of Safari that you are using is pretty old and full of bugs. So besides other issues, Adblock Plus might not work correctly or will even break some websites. We strongly recommend to <strong>update to Safari 6 (OSX 10.8) or higher</strong> if possible, or to use the latest version of an other browser, like Firefox or Google Chrome. On 2013/09/06 09:44:41, Felix H. Dahlke wrote: > That's true. How about making it more polite though? :D Here's my suggestion: > > You are using an old version of Safari which is not supported by Adblock Plus, > so it might not work correctly. We strongly recommend to either update to Safari > 6 (available in Mac OS X 10.8, Mountain Lion) or to use another browser, like > Firefox. I agree that the wording of the message could be a little nicer, but I have some suggestions regarding your text: 1. The issue with the beforeload bug (why we actually added the message) isn't that Adblock Plus will not work correctly (though that might happen as well due to other issues), but that other websites will break when an extension registers a beforeload listeners, like we do. 2. You refer exclusively to Safari 6 and OSX 10.8. So the user might think that he needs exactly this version of Safari and OSX. But what we actually mean is that he should either update to that version or any higher version. 3. The user might dislike Firefox, but might not be aware that ABP is also available for Google Chrome and Opera. So we might want to list all other three browsers for OSX that we support. So what would you think about following text for the warning? You are using an old version of Safari which is not supported by Adblock Plus. So it might not work correctly or impair the user experience on some websites. We strongly recommend to either update to Safari 6 or higher (available since Mac OS X 10.8, Mountain Lion), or to use the latest version of Firefox, Google Chrome or Opera.
On 2013/09/06 15:45:12, sebastian wrote: > http://codereview.adblockplus.org/11533106/diff/3001/chrome/locale/en-US/firs... > File chrome/locale/en-US/firstRun.properties (right): > > http://codereview.adblockplus.org/11533106/diff/3001/chrome/locale/en-US/firs... > chrome/locale/en-US/firstRun.properties:4: firstRun_legacySafariWarning=The > version of Safari that you are using is pretty old and full of bugs. So besides > other issues, Adblock Plus might not work correctly or will even break some > websites. We strongly recommend to <strong>update to Safari 6 (OSX 10.8) or > higher</strong> if possible, or to use the latest version of an other browser, > like Firefox or Google Chrome. > On 2013/09/06 09:44:41, Felix H. Dahlke wrote: > > That's true. How about making it more polite though? :D Here's my suggestion: > > > > You are using an old version of Safari which is not supported by Adblock Plus, > > so it might not work correctly. We strongly recommend to either update to > Safari > > 6 (available in Mac OS X 10.8, Mountain Lion) or to use another browser, like > > Firefox. > > I agree that the wording of the message could be a little nicer, but I have > some suggestions regarding your text: > > 1. The issue with the beforeload bug (why we actually added the message) isn't > that Adblock Plus will not work correctly (though that might happen as well due > to other issues), but that other websites will break when an extension registers > a beforeload listeners, like we do. > 2. You refer exclusively to Safari 6 and OSX 10.8. So the user might think that > he needs exactly this version of Safari and OSX. But what we actually mean is > that he should either update to that version or any higher version. > 3. The user might dislike Firefox, but might not be aware that ABP is also > available for Google Chrome and Opera. So we might want to list all other three > browsers for OSX that we support. > > So what would you think about following text for the warning? > > You are using an old version of Safari which is not supported by Adblock Plus. > So it might not work correctly or impair the user experience on some websites. > We strongly recommend to either update to Safari 6 or higher (available since > Mac OS X 10.8, Mountain Lion), or to use the latest version of Firefox, Google > Chrome or Opera. Agree with everything, looks good now.
I've changed the text as discussed. And I fixed the path of the CSS in firefox, which I broke apparently. The problem is that in Safari you can't use absolute paths (relative to the extension root). But the relative path from the firstRun.html to its CSS is an other in Firefox, than in the ports. So we need to link tags :( So could you review the changes again? Thanks.
LGTM with a wording nit. That is, assuming that Wladimir won't suggest a nice way of including only the files we need in firstRun.html, I'd prefer that. http://codereview.adblockplus.org/11533106/diff/7003/chrome/locale/en-US/firs... File chrome/locale/en-US/firstRun.properties (right): http://codereview.adblockplus.org/11533106/diff/7003/chrome/locale/en-US/firs... chrome/locale/en-US/firstRun.properties:4: firstRun_legacySafariWarning=You are using an old version of Safari which is not supported by Adblock Plus. So it might not work correctly or impair the user experience on some websites. We strongly recommend to either update to Safari 6 or higher (available since Mac OS X 10.8, Mountain Lion), or to use the latest version of Firefox, Google Chrome or Opera. s/So it/It/ sounds better here I think.
http://codereview.adblockplus.org/11533106/diff/12006/chrome/content/ui/first... File chrome/content/ui/firstRun.html (right): http://codereview.adblockplus.org/11533106/diff/12006/chrome/content/ui/first... chrome/content/ui/firstRun.html:28: <link type="text/css" href="skin/firstRun.css" rel="stylesheet"/> I'm not particularly happy with using random relative paths to refer to skin files and assuming that these relative paths will only point somewhere on one platform. If there is no other solution (and seeing Safari documentation explicitly discourage host-relative URLs there probably isn't) I would prefer keeping the paths as they are and convert them automatically in the Safari packager. Note: the comment claiming that we were already loading non-existing files here is wrong. utils.js does exist in Firefox: https://hg.adblockplus.org/adblockplus/file/b823f710a2c7/chrome/content/ui/ut.... The adblockpluschrome repository merely contains a different version of that file. http://codereview.adblockplus.org/11533106/diff/12006/chrome/content/ui/first... chrome/content/ui/firstRun.html:33: <script type="text/javascript" src="lib/adblockplus.js"></script> Loading the core code into the page isn't going to do you any good - it will slow down the loading of this page significantly but you will be operating on a copy of the data, not the data used by the rest of the extension. This cannot possibly work correctly. A proper solution requires this page to communicate with the rest of the extension. From the documentation it isn't clear whether a page will be able to do that by itself, you might need to inject a content script into it. You should be able to abuse safari.self.tab.canLoad() to do synchronous messaging. require() should work by sending a synchronous message to the global page and getting a list of methods and properties back. Using that list it should construct a wrapper: * Property getters use synchronous messaging to retrieve their value from the global page (any non-primitive values should again be resolved into wrappers). * Property setters use synchronous messaging to set the value in the global page. * Method calls are forwarded to the global page via synchronous messaging, their result is returned (any non-primitive values should again be resolved into wrappers). Note that the global page will need to keep a list of objects that wrappers have been created for - otherwise it won't know which object to call a method on. We cannot really know when a wrapper will stop being used, notifying the global page in the "unload" event that it can clear that list should be enough to avoid a memory leak however. From what I can tell, FilterNotifier will be the only object needing special handling. Wrapping it won't do any good, you will need to reimplement it for this page. So FilterNotifier.addListener() will ask the global page to register a listener on its end and send an async message to the page whenever it is called. All in all, not exactly a simple approach, the wrapper logic won't really be trivial. However, I don't see how we are going to avoid doing this. http://codereview.adblockplus.org/11533106/diff/12006/chrome/content/ui/first... File chrome/content/ui/firstRun.js (right): http://codereview.adblockplus.org/11533106/diff/12006/chrome/content/ui/first... chrome/content/ui/firstRun.js:57: var info = require('info'); Nit: Please use double quotation marks unless there is a reason to use single quotation marks. http://codereview.adblockplus.org/11533106/diff/12006/chrome/content/ui/first... chrome/content/ui/firstRun.js:58: if (info.platform == "safari" && info.platformVersion.split(".") < [6]) This is JavaScript, not Python ;) Please have a look at the result of [12] < [6]. Arrays and other non-primitive types aren't comparable directly. This means that JavaScript implicitly converts them to strings before comparing. Ideally you could use Services.vc.compare(info.platformVersion, "6.0") here (requires implementing wrappers to call into the global page as mentioned above). http://codereview.adblockplus.org/11533106/diff/12006/chrome/content/ui/i18n.js File chrome/content/ui/i18n.js (right): http://codereview.adblockplus.org/11533106/diff/12006/chrome/content/ui/i18n.... chrome/content/ui/i18n.js:18: var i18n = (window.ext && ext.i18n) || (window.chrome && chrome.i18n); This will produce a warning in JavaScript - you are accessing an undefined property. Also, I don't really consider this code readable. So I would prefer that you keep the original approach: if (typeof chrome != "undefined") ... else if (typeof ext != "undefined") ... else http://codereview.adblockplus.org/11533106/diff/12006/chrome/locale/en-US/fir... File chrome/locale/en-US/firstRun.properties (right): http://codereview.adblockplus.org/11533106/diff/12006/chrome/locale/en-US/fir... chrome/locale/en-US/firstRun.properties:4: firstRun_legacySafariWarning=You are using an old version of Safari which is not supported by Adblock Plus. So it might not work correctly or impair the user experience on some websites. We strongly recommend to either update to Safari 6 or higher (available since Mac OS X 10.8, Mountain Lion), or to use the latest version of Firefox, Google Chrome or Opera. I agree with Felix, it should be "It might not work", please drop "So" in that sentence. Also, I suggest removing "available since Mac OS X 10.8, Mountain Lion", it is unnecessary IMHO. Finally, it should be "Mozilla Firefox" (the full name of the browser).
http://codereview.adblockplus.org/11533106/diff/12006/chrome/content/ui/i18n.js File chrome/content/ui/i18n.js (right): http://codereview.adblockplus.org/11533106/diff/12006/chrome/content/ui/i18n.... chrome/content/ui/i18n.js:18: var i18n = (window.ext && ext.i18n) || (window.chrome && chrome.i18n); On 2013/09/10 06:53:46, Wladimir Palant wrote: > This will produce a warning in JavaScript A warning in Firefox is what I meant of course.
http://codereview.adblockplus.org/11533106/diff/12006/chrome/content/ui/first... File chrome/content/ui/firstRun.html (right): http://codereview.adblockplus.org/11533106/diff/12006/chrome/content/ui/first... chrome/content/ui/firstRun.html:28: <link type="text/css" href="skin/firstRun.css" rel="stylesheet"/> On 2013/09/10 06:53:46, Wladimir Palant wrote: > I'm not particularly happy with using random relative paths to refer to skin > files and assuming that these relative paths will only point somewhere on one > platform. If there is no other solution (and seeing Safari documentation > explicitly discourage host-relative URLs there probably isn't) I would prefer > keeping the paths as they are and convert them automatically in the Safari > packager. Yes, there isn't an other solution (for safari). Host relative paths lead to an error and the resource doesn't load. I tried it out. I don't like the idea of converting the HTML in the packager, because of it might easily break when the HTML file is changed without the conversion in mind. However it still might be the best option we have. :( > Note: the comment claiming that we were already loading non-existing files here > is wrong. utils.js does exist in Firefox: > https://hg.adblockplus.org/adblockplus/file/b823f710a2c7/chrome/content/ui/ut.... > The adblockpluschrome repository merely contains a different version of that > file. Oh, you are right, my fault. http://codereview.adblockplus.org/11533106/diff/12006/chrome/content/ui/first... chrome/content/ui/firstRun.html:33: <script type="text/javascript" src="lib/adblockplus.js"></script> On 2013/09/10 06:53:46, Wladimir Palant wrote: > Loading the core code into the page isn't going to do you any good - it will > slow down the loading of this page significantly but you will be operating on a > copy of the data, not the data used by the rest of the extension. This cannot > possibly work correctly. > > A proper solution requires this page to communicate with the rest of the > extension. From the documentation it isn't clear whether a page will be able to > do that by itself, you might need to inject a content script into it. > > You should be able to abuse safari.self.tab.canLoad() to do synchronous > messaging. require() should work by sending a synchronous message to the global > page and getting a list of methods and properties back. Using that list it > should construct a wrapper: > > * Property getters use synchronous messaging to retrieve their value from the > global page (any non-primitive values should again be resolved into wrappers). > * Property setters use synchronous messaging to set the value in the global > page. > * Method calls are forwarded to the global page via synchronous messaging, their > result is returned (any non-primitive values should again be resolved into > wrappers). > > Note that the global page will need to keep a list of objects that wrappers have > been created for - otherwise it won't know which object to call a method on. We > cannot really know when a wrapper will stop being used, notifying the global > page in the "unload" event that it can clear that list should be enough to avoid > a memory leak however. > > From what I can tell, FilterNotifier will be the only object needing special > handling. Wrapping it won't do any good, you will need to reimplement it for > this page. So FilterNotifier.addListener() will ask the global page to register > a listener on its end and send an async message to the page whenever it is > called. > > All in all, not exactly a simple approach, the wrapper logic won't really be > trivial. However, I don't see how we are going to avoid doing this. Unfortunately you are right. I didn't thought about it before, well enough. The first run page, currently operates on a local copy of the data, and therefore the UI switches in the firstRun page wouldn't do anything. The same problem exists for the options page. And yes the only solution will be to implement a proxy interface that uses synchronous messaging. I got this idea before, but I tried to avoid it so far, because of this will become really complex, tricky and ugly. But it seems that we don't have any other option, now. :( http://codereview.adblockplus.org/11533106/diff/12006/chrome/content/ui/first... File chrome/content/ui/firstRun.js (right): http://codereview.adblockplus.org/11533106/diff/12006/chrome/content/ui/first... chrome/content/ui/firstRun.js:58: if (info.platform == "safari" && info.platformVersion.split(".") < [6]) On 2013/09/10 06:53:46, Wladimir Palant wrote: > This is JavaScript, not Python ;) > > Please have a look at the result of [12] < [6]. Arrays and other non-primitive > types aren't comparable directly. This means that JavaScript implicitly converts > them to strings before comparing. > > Ideally you could use Services.vc.compare(info.platformVersion, "6.0") here > (requires implementing wrappers to call into the global page as mentioned > above). Services.vc.compare is Firefox specific, isn't it? However info.platformVersion.split(".")[0] < 6 seems to work. Any reasons to don't do it that way? http://codereview.adblockplus.org/11533106/diff/12006/chrome/content/ui/i18n.js File chrome/content/ui/i18n.js (right): http://codereview.adblockplus.org/11533106/diff/12006/chrome/content/ui/i18n.... chrome/content/ui/i18n.js:18: var i18n = (window.ext && ext.i18n) || (window.chrome && chrome.i18n); On 2013/09/10 06:53:46, Wladimir Palant wrote: > This will produce a warning in JavaScript - you are accessing an undefined > property. Also, I don't really consider this code readable. So I would prefer > that you keep the original approach: > > if (typeof chrome != "undefined") > ... > else if (typeof ext != "undefined") > ... > else I couldn't reproduce that. Accessing an undefined property just returns undefined, which evaluates to false. The above code seems to work without any warning, even in Firefox. I have tested it. Also I consider my code slightly more readable, but I guess that is just a matter of taste. http://codereview.adblockplus.org/11533106/diff/12006/chrome/locale/en-US/fir... File chrome/locale/en-US/firstRun.properties (right): http://codereview.adblockplus.org/11533106/diff/12006/chrome/locale/en-US/fir... chrome/locale/en-US/firstRun.properties:4: firstRun_legacySafariWarning=You are using an old version of Safari which is not supported by Adblock Plus. So it might not work correctly or impair the user experience on some websites. We strongly recommend to either update to Safari 6 or higher (available since Mac OS X 10.8, Mountain Lion), or to use the latest version of Firefox, Google Chrome or Opera. On 2013/09/10 06:53:46, Wladimir Palant wrote: > I agree with Felix, it should be "It might not work", please drop "So" in that sentence. Agreed. > Also, I suggest removing "available since Mac OS X 10.8, Mountain Lion", it is unnecessary IMHO. I've mentioned it because of in order to update to Safari 6, what you actually have to do is to update to Mac OS X 10.8. Otherwise the user might try to update Safari, will notice that there aren't any updates for Safari, and assume that he is already on the latest version of Safari. > Finally, it should be "Mozilla Firefox" (the full name of the browser). Agreed.
http://codereview.adblockplus.org/11533106/diff/12006/chrome/content/ui/first... File chrome/content/ui/firstRun.js (right): http://codereview.adblockplus.org/11533106/diff/12006/chrome/content/ui/first... chrome/content/ui/firstRun.js:58: if (info.platform == "safari" && info.platformVersion.split(".") < [6]) On 2013/09/10 10:56:29, sebastian wrote: > Services.vc.compare is Firefox specific, isn't it? No, we also have our own version comparator implementation in compat.js. > However > info.platformVersion.split(".")[0] < 6 seems to work. Any reasons to don't do it > that way? Because the version comparator is the way we compare versions - it works correctly with complicated version numbers as well. And version comparison is non-trivial as you've just proved again. Your new code is also wrong, please look at the result of "12.0".split(".")[0] < 6. Comparing a string to a number implicitly converts the latter to a number. To do a proper numerical comparison you need an explicit conversion to a number: parseInt(info.platformVersion.split(".")[0], 10). So, how about using the standard tool for this job? http://codereview.adblockplus.org/11533106/diff/12006/chrome/content/ui/i18n.js File chrome/content/ui/i18n.js (right): http://codereview.adblockplus.org/11533106/diff/12006/chrome/content/ui/i18n.... chrome/content/ui/i18n.js:18: var i18n = (window.ext && ext.i18n) || (window.chrome && chrome.i18n); On 2013/09/10 10:56:29, sebastian wrote: > I couldn't reproduce that. Accessing an undefined property just returns > undefined, which evaluates to false. You need to enable javascript.option.strict preference which is generally recommended when developing in Firefox (e.g. it will also warn about accessing undeclared global variables). http://codereview.adblockplus.org/11533106/diff/12006/chrome/locale/en-US/fir... File chrome/locale/en-US/firstRun.properties (right): http://codereview.adblockplus.org/11533106/diff/12006/chrome/locale/en-US/fir... chrome/locale/en-US/firstRun.properties:4: firstRun_legacySafariWarning=You are using an old version of Safari which is not supported by Adblock Plus. So it might not work correctly or impair the user experience on some websites. We strongly recommend to either update to Safari 6 or higher (available since Mac OS X 10.8, Mountain Lion), or to use the latest version of Firefox, Google Chrome or Opera. On 2013/09/10 10:56:29, sebastian wrote: > > Also, I suggest removing "available since Mac OS X 10.8, Mountain Lion", it is > unnecessary IMHO. > > I've mentioned it because of in order to update to Safari 6, what you actually > have to do is to update to Mac OS X 10.8. Otherwise the user might try to update > Safari, will notice that there aren't any updates for Safari, and assume that he > is already on the latest version of Safari. Well, then it should at least say "available for Mac OS X 10.8 and higher" (slightly better formulation, no code names).
http://codereview.adblockplus.org/11533106/diff/12006/chrome/content/ui/first... File chrome/content/ui/firstRun.js (right): http://codereview.adblockplus.org/11533106/diff/12006/chrome/content/ui/first... chrome/content/ui/firstRun.js:58: if (info.platform == "safari" && info.platformVersion.split(".") < [6]) On 2013/09/10 11:10:14, Wladimir Palant wrote: > Comparing a string to a number implicitly converts the latter to a number. Argh, should be: "implicitly converts the latter to a string".
http://codereview.adblockplus.org/11533106/diff/12006/chrome/content/ui/first... File chrome/content/ui/firstRun.js (right): http://codereview.adblockplus.org/11533106/diff/12006/chrome/content/ui/first... chrome/content/ui/firstRun.js:58: if (info.platform == "safari" && info.platformVersion.split(".") < [6]) On 2013/09/10 11:10:14, Wladimir Palant wrote: > On 2013/09/10 10:56:29, sebastian wrote: > > However > > info.platformVersion.split(".")[0] < 6 seems to work. Any reasons to don't do > it > > that way? > > Because the version comparator is the way we compare versions - it works > correctly with complicated version numbers as well. And version comparison is > non-trivial as you've just proved again. > > Your new code is also wrong, please look at the result of "12.0".split(".")[0] < > 6. Comparing a string to a number implicitly converts the latter to a number. To > do a proper numerical comparison you need an explicit conversion to a number: > parseInt(info.platformVersion.split(".")[0], 10). Well, "12.0".split(".")[0] < 6, returns false as it should (also in Firefox). And that is because of, as you just said yourself, the string is converted to a number during the comparison. I don't get what you mean. > > Services.vc.compare is Firefox specific, isn't it? > > No, we also have our own version comparator implementation in compat.js. Then I will use it of course. I just assumed that it was Firefox specific and that we can't use it in other browsers. But if we have a compatibility implementation, then it's fine. http://codereview.adblockplus.org/11533106/diff/12006/chrome/content/ui/i18n.js File chrome/content/ui/i18n.js (right): http://codereview.adblockplus.org/11533106/diff/12006/chrome/content/ui/i18n.... chrome/content/ui/i18n.js:18: var i18n = (window.ext && ext.i18n) || (window.chrome && chrome.i18n); On 2013/09/10 11:10:14, Wladimir Palant wrote: > On 2013/09/10 10:56:29, sebastian wrote: > > I couldn't reproduce that. Accessing an undefined property just returns > > undefined, which evaluates to false. > > You need to enable javascript.option.strict preference which is generally > recommended when developing in Firefox (e.g. it will also warn about accessing > undeclared global variables). Sure, in strict mode that wouldn't work anymore. I wasn't aware that we are using strict mode. And at least in i18n.js we apparently don't use it. But it might make sense nevertheless to stay compatible with strict mode. http://codereview.adblockplus.org/11533106/diff/12006/chrome/locale/en-US/fir... File chrome/locale/en-US/firstRun.properties (right): http://codereview.adblockplus.org/11533106/diff/12006/chrome/locale/en-US/fir... chrome/locale/en-US/firstRun.properties:4: firstRun_legacySafariWarning=You are using an old version of Safari which is not supported by Adblock Plus. So it might not work correctly or impair the user experience on some websites. We strongly recommend to either update to Safari 6 or higher (available since Mac OS X 10.8, Mountain Lion), or to use the latest version of Firefox, Google Chrome or Opera. On 2013/09/10 11:10:14, Wladimir Palant wrote: > On 2013/09/10 10:56:29, sebastian wrote: > > > Also, I suggest removing "available since Mac OS X 10.8, Mountain Lion", it > is > > unnecessary IMHO. > > > > I've mentioned it because of in order to update to Safari 6, what you actually > > have to do is to update to Mac OS X 10.8. Otherwise the user might try to > update > > Safari, will notice that there aren't any updates for Safari, and assume that > he > > is already on the latest version of Safari. > > Well, then it should at least say "available for Mac OS X 10.8 and higher" > (slightly better formulation, no code names). The initial version of the text said "Safari 6 (OS X 10.8) or higher", adding the codename was a suggestion of Felix.
http://codereview.adblockplus.org/11533106/diff/12006/chrome/content/ui/first... File chrome/content/ui/firstRun.js (right): http://codereview.adblockplus.org/11533106/diff/12006/chrome/content/ui/first... chrome/content/ui/firstRun.js:58: if (info.platform == "safari" && info.platformVersion.split(".") < [6]) Never mind, my bad. Regardless, the version comparator should be used for version comparisons. http://codereview.adblockplus.org/11533106/diff/12006/chrome/content/ui/i18n.js File chrome/content/ui/i18n.js (right): http://codereview.adblockplus.org/11533106/diff/12006/chrome/content/ui/i18n.... chrome/content/ui/i18n.js:18: var i18n = (window.ext && ext.i18n) || (window.chrome && chrome.i18n); On 2013/09/10 14:20:57, sebastian wrote: > Sure, in strict mode that wouldn't work anymore. That's just a development setting, not strict mode. We certainly want to switch to strict mode eventually however. http://codereview.adblockplus.org/11533106/diff/12006/chrome/locale/en-US/fir... File chrome/locale/en-US/firstRun.properties (right): http://codereview.adblockplus.org/11533106/diff/12006/chrome/locale/en-US/fir... chrome/locale/en-US/firstRun.properties:4: firstRun_legacySafariWarning=You are using an old version of Safari which is not supported by Adblock Plus. So it might not work correctly or impair the user experience on some websites. We strongly recommend to either update to Safari 6 or higher (available since Mac OS X 10.8, Mountain Lion), or to use the latest version of Firefox, Google Chrome or Opera. On 2013/09/10 14:20:57, sebastian wrote: > The initial version of the text said "Safari 6 (OS X 10.8) or higher", adding > the codename was a suggestion of Felix. I see. Maybe Felix can explain his reasoning then. Personally, I feel that we shouldn't use code names in a consumer product. Also, I doubt that many users can compare the actuality OS releases by their code name. Then again, Apple is apparently putting these code names on the DVD boxes.
http://codereview.adblockplus.org/11533106/diff/12006/chrome/locale/en-US/fir... File chrome/locale/en-US/firstRun.properties (right): http://codereview.adblockplus.org/11533106/diff/12006/chrome/locale/en-US/fir... chrome/locale/en-US/firstRun.properties:4: firstRun_legacySafariWarning=You are using an old version of Safari which is not supported by Adblock Plus. So it might not work correctly or impair the user experience on some websites. We strongly recommend to either update to Safari 6 or higher (available since Mac OS X 10.8, Mountain Lion), or to use the latest version of Firefox, Google Chrome or Opera. On 2013/09/10 14:34:21, Wladimir Palant wrote: > On 2013/09/10 14:20:57, sebastian wrote: > > The initial version of the text said "Safari 6 (OS X 10.8) or higher", adding > > the codename was a suggestion of Felix. > > I see. Maybe Felix can explain his reasoning then. Personally, I feel that we > shouldn't use code names in a consumer product. Also, I doubt that many users > can compare the actuality OS releases by their code name. Then again, Apple is > apparently putting these code names on the DVD boxes. From my experience, most OSX users know the code name, almost nobody knows the version number. Apple uses the code name everywhere in their communication, the version number itself is treated more like a technical detail.
http://codereview.adblockplus.org/11533106/diff/28001/chrome/content/ui/first... File chrome/content/ui/firstRun.html (right): http://codereview.adblockplus.org/11533106/diff/28001/chrome/content/ui/first... chrome/content/ui/firstRun.html:24: <!-- {% if type == 'safari' %} --> Why are the Jinja tags in HTML comments? I presume that's handled in adblockpluschrome? IMO, always loading this as a template would make more sense, that way we'd have a general mechanism for platform-specific differences. http://codereview.adblockplus.org/11533106/diff/28001/chrome/content/ui/first... chrome/content/ui/firstRun.html:26: <!-- {% else %} --> How about something shorter like this instead? <link type="text/css" href="{{ '/' if type == 'safari' }}skin/firstRun.css" rel="stylesheet"/> Also, what about the JS files below, shouldn't we use an absolute path for them as well? http://codereview.adblockplus.org/11533106/diff/28001/chrome/content/ui/first... File chrome/content/ui/firstRun.js (right): http://codereview.adblockplus.org/11533106/diff/28001/chrome/content/ui/first... chrome/content/ui/firstRun.js:57: var info = require('info'); Wladimir requested double quotes here, that's in line with the Mozilla JavaScript-style which we mostly follow, and also consistent with the surrounding code. http://codereview.adblockplus.org/11533106/diff/28001/chrome/content/ui/first... chrome/content/ui/firstRun.js:58: if (info.platform == "safari" && Services.vc.compare(info.platformVersion, "6.0") < 0) Looks like the indentation is off here, should be 4 spaces.
http://codereview.adblockplus.org/11533106/diff/28001/chrome/content/ui/first... File chrome/content/ui/firstRun.html (right): http://codereview.adblockplus.org/11533106/diff/28001/chrome/content/ui/first... chrome/content/ui/firstRun.html:24: <!-- {% if type == 'safari' %} --> On 2013/10/15 10:31:23, Felix H. Dahlke wrote: > Why are the Jinja tags in HTML comments? I presume that's handled in > adblockpluschrome? IMO, always loading this as a template would make more sense, > that way we'd have a general mechanism for platform-specific differences. See the discussion in the other review. http://codereview.adblockplus.org/11544056/diff/32001/packagerChrome.py#newco... http://codereview.adblockplus.org/11533106/diff/28001/chrome/content/ui/first... chrome/content/ui/firstRun.html:26: <!-- {% else %} --> On 2013/10/15 10:31:23, Felix H. Dahlke wrote: > How about something shorter like this instead? > > <link type="text/css" href="{{ '/' if type == 'safari' }}skin/firstRun.css" > rel="stylesheet"/> That wouldn't work, because for compatibility jinja expressions must be wrapped by an HTML comment. And comments within attributes are not possible in HTML. > Also, what about the JS files below, shouldn't we use an absolute path for them > as well? 1. In Safari absolute (relative to the extension root) URLs are not possible in extension pages. That's the reason why we have the browser switch above. 2. The absolute path would be different for Chrome and Firefox, if I recall correctly. So another browser switch would be required. 3. As you can see most of the JS files below were already in there with a relative path. http://codereview.adblockplus.org/11533106/diff/28001/chrome/content/ui/first... File chrome/content/ui/firstRun.js (right): http://codereview.adblockplus.org/11533106/diff/28001/chrome/content/ui/first... chrome/content/ui/firstRun.js:57: var info = require('info'); On 2013/10/15 10:31:23, Felix H. Dahlke wrote: > Wladimir requested double quotes here, that's in line with the Mozilla > JavaScript-style which we mostly follow, and also consistent with the > surrounding code. I must have been overlooked that string. I'll fix that. http://codereview.adblockplus.org/11533106/diff/28001/chrome/content/ui/first... chrome/content/ui/firstRun.js:58: if (info.platform == "safari" && Services.vc.compare(info.platformVersion, "6.0") < 0) On 2013/10/15 10:31:23, Felix H. Dahlke wrote: > Looks like the indentation is off here, should be 4 spaces. I'll fix that.
I have split ext.js into ext/common.js, ext/background.js and ext/content.js. Therefore I had to add two instead of one js file to the first run page. Please review again.
LGTM
I've rebased my changes. So you might want to review it again.
Still LGTM :)
I won't insist on addressing these issues before you push these changes but they should be addressed timely nevertheless. http://codereview.adblockplus.org/11533106/diff/151001/chrome/content/ui/firs... File chrome/content/ui/firstRun.html (right): http://codereview.adblockplus.org/11533106/diff/151001/chrome/content/ui/firs... chrome/content/ui/firstRun.html:28: <!-- {% endif %} --> As mentioned before, I would prefer a solution within build tools here - we shouldn't have to think about all platforms when writing basic code code. The Safari packager can easily process HTML files and replace host-relative links to skin by relative ones. If you don't like that solution, some code triggered from utils.js can check require("info").platform and go through stylesheet links correcting the href attribute - that's at least a generic approach not requiring additional markup. http://codereview.adblockplus.org/11533106/diff/151001/chrome/content/ui/firs... chrome/content/ui/firstRun.html:33: <!-- {% endif %} --> If we are adding these script tags dynamically, we can simply do it at the top of utils.js (the one in adblockpluschrome repository, not adblockplus). A templating language for that is an overkill. http://codereview.adblockplus.org/11533106/diff/151001/chrome/content/ui/i18n.js File chrome/content/ui/i18n.js (right): http://codereview.adblockplus.org/11533106/diff/151001/chrome/content/ui/i18n... chrome/content/ui/i18n.js:23: i18n = chrome.i18n; I guess that this branch should be removed once adblockpluschrome repository is updated? Please add a corresponding TODO comment. Note that the goal would be to use ext.i18n directly everywhere. We probably want to move the Firefox branch below into a Firefox version of ext/common.js that will define that variable as well.
http://codereview.adblockplus.org/11533106/diff/151001/chrome/content/ui/firs... File chrome/content/ui/firstRun.html (right): http://codereview.adblockplus.org/11533106/diff/151001/chrome/content/ui/firs... chrome/content/ui/firstRun.html:28: <!-- {% endif %} --> On 2013/10/29 12:59:52, Wladimir Palant wrote: > As mentioned before, I would prefer a solution within build tools here - we > shouldn't have to think about all platforms when writing basic code code. The > Safari packager can easily process HTML files and replace host-relative links to > skin by relative ones. I would rather not hide the logic from the code of the first run page, replacing it with some buildtools magic. Otherwise it can simply break accidentally when changing the first run page. > If you don't like that solution, some code triggered from utils.js can check > require("info").platform and go through stylesheet links correcting the href > attribute - that's at least a generic approach not requiring additional markup. I think that aproach wouldn't work for the js files below (see comment below). So I would rather stay to one solution for both. http://codereview.adblockplus.org/11533106/diff/151001/chrome/content/ui/firs... chrome/content/ui/firstRun.html:33: <!-- {% endif %} --> On 2013/10/29 12:59:52, Wladimir Palant wrote: > If we are adding these script tags dynamically, we can simply do it at the top > of utils.js (the one in adblockpluschrome repository, not adblockplus). A > templating language for that is an overkill. I might be wrong. But I don't think that this will actually work. Those two js files must be loaded and executed, before any other js code runs. And as far as I know, dynamically injecting a script tag at the top of utils.js, wouldn't block further js execution until those files are loaded and executed.
http://codereview.adblockplus.org/11533106/diff/151001/chrome/content/ui/firs... File chrome/content/ui/firstRun.html (right): http://codereview.adblockplus.org/11533106/diff/151001/chrome/content/ui/firs... chrome/content/ui/firstRun.html:28: <!-- {% endif %} --> On 2013/10/29 13:37:23, sebastian wrote: > I would rather not hide the logic from the code of the first run page, replacing > it with some buildtools magic. Otherwise it can simply break accidentally when > changing the first run page. We have a number of conventions in place that work across all supported platforms thanks to "some buildtools magic". One such convention is: "use host-relative paths to refer to skin files." IMHO, having to keep in mind all the details of all platforms is worse than having to stick to these conventions. http://codereview.adblockplus.org/11533106/diff/151001/chrome/content/ui/firs... chrome/content/ui/firstRun.html:33: <!-- {% endif %} --> On 2013/10/29 13:37:23, sebastian wrote: > I might be wrong. But I don't think that this will actually work. Those two js > files must be loaded and executed, before any other js code runs. And as far as > I know, dynamically injecting a script tag at the top of utils.js, wouldn't > block further js execution until those files are loaded and executed. Setting |script.async = false| should actually do what you need. I tested the following in Firefox and Chrome: var s = document.createElement("script"); s.async = false; s.src="data:text/javascript,alert(1)"; document.head.appendChild(s); alert(2); setTimeout(function() {alert(3)}, 0); Firefox will execute the injected script immediately, Chrome will only do it after the execution context is done. I assume that Safari's approach will be similar to Chrome. Unfortunately, this means that loading scripts dynamically cannot be done at the top of utils.js, it rather has to be done by a separate script loading before utils.js. Side-note: now that I've found the implementation of these conditional comments I consider it a huge hack. If kept: a) It shouldn't use special templating syntax but rather keep the standard syntax. Firefox packager should be updated accordingly. b) It shouldn't be hardcoded to run on the first-run page, the metadata file should rather define the files to use Jinja2 on.
http://codereview.adblockplus.org/11533106/diff/151001/chrome/content/ui/firs... File chrome/content/ui/firstRun.html (right): http://codereview.adblockplus.org/11533106/diff/151001/chrome/content/ui/firs... chrome/content/ui/firstRun.html:33: <!-- {% endif %} --> On 2013/10/29 14:05:45, Wladimir Palant wrote: > On 2013/10/29 13:37:23, sebastian wrote: > > I might be wrong. But I don't think that this will actually work. Those two js > > files must be loaded and executed, before any other js code runs. And as far > as > > I know, dynamically injecting a script tag at the top of utils.js, wouldn't > > block further js execution until those files are loaded and executed. > > Setting |script.async = false| should actually do what you need. I tested the > following in Firefox and Chrome: > > var s = document.createElement("script"); > s.async = false; > s.src="data:text/javascript,alert(1)"; > document.head.appendChild(s); > alert(2); > setTimeout(function() {alert(3)}, 0); > > Firefox will execute the injected script immediately, Chrome will only do it > after the execution context is done. I assume that Safari's approach will be > similar to Chrome. Unfortunately, this means that loading scripts dynamically > cannot be done at the top of utils.js, it rather has to be done by a separate > script loading before utils.js. > > Side-note: now that I've found the implementation of these conditional comments > I consider it a huge hack. If kept: > > a) It shouldn't use special templating syntax but rather keep the standard > syntax. Firefox packager should be updated accordingly. They are just wrapped by HTML comments, for backwards compatibility. It was my plan anyway to make the first run page always generated, later. And as you said you don't insist on having it done before merging this changes. > b) It shouldn't be hardcoded to run on the first-run page, the metadata file > should rather define the files to use Jinja2 on. Agreed, that certainly would be nicer. So it sounds that you have accepted the need for this approach or do you still have a better idea?
http://codereview.adblockplus.org/11533106/diff/151001/chrome/content/ui/firs... File chrome/content/ui/firstRun.html (right): http://codereview.adblockplus.org/11533106/diff/151001/chrome/content/ui/firs... chrome/content/ui/firstRun.html:33: <!-- {% endif %} --> On 2013/10/29 15:44:56, sebastian wrote: > So it sounds that you have accepted the need for this approach or do you still > have a better idea? For the scripts: yes, processing the page with Jinja2 should be a cleaner solution (at least once the comments are gone). Ideally, soon Firefox won't be "special" any more. For the styles: no, requiring special code for Safari everywhere isn't something I'm happy with. As I said, the host-relative URLs should be converted automatically, either in build tools or in a script like ext/content.js.
LGTM assuming that the skin path is being changed by the packager (I haven't seen any code for that in the buildtools review).
On 2013/10/31 06:12:26, Wladimir Palant wrote: > LGTM assuming that the skin path is being changed by the packager (I haven't > seen any code for that in the buildtools review). Ah, I see - it's being changed by JavaScript. Well, still good enough for this release.
On 2013/10/31 06:13:26, Wladimir Palant wrote: > Ah, I see - it's being changed by JavaScript. Well, still good enough for this > release. What means good enough? It was actually one of your suggestions to do that with javascript. Also that way I can convert host-relative URLs just to absolute URLs (by adding the base URI), instead of replacing them with relative urls, which would work in that particular case but not in all possible cases.
Even more LGTM now :)
On 2013/10/31 07:43:19, sebastian wrote: > What means good enough? It was actually one of your suggestions to do that with > javascript. Yes, and there is a reason why I listed the option to do it in build tools first ;) The biggest issue with this solution is that there will be some flickering when the page loads as it is first displayed without any styles. > Also that way I can convert host-relative URLs just to absolute URLs > (by adding the base URI), instead of replacing them with relative urls, which > would work in that particular case but not in all possible cases. You can construct a valid relative URL for all the possible cases, it merely needs a slightly more elaborate logic ;)
> On 2013/10/31 07:43:19, sebastian wrote: > > What means good enough? It was actually one of your suggestions to do that > with > > javascript. > > Yes, and there is a reason why I listed the option to do it in build tools first > ;) > > The biggest issue with this solution is that there will be some flickering when > the page loads as it is first displayed without any styles. You are right. I thought this wouldn't actually be the case because I fixed the URLs immediately on DOMContentLoaded and because the page is loaded locally from the extension bundle, so I considered page load time to be always fast enough to avoid that. However it turned out that it is still flickering, before the URL is fixed an the CSS is loaded. So as suggested, I made the buildtools fix the absolute URLs, now. > > Also that way I can convert host-relative URLs just to absolute URLs > > (by adding the base URI), instead of replacing them with relative urls, which > > would work in that particular case but not in all possible cases. > > You can construct a valid relative URL for all the possible cases, it merely > needs a slightly more elaborate logic ;) So, I did.
Still LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
