Issue 6427347985104896: Issue 1663 - Making first-run page testable in the adblockplusui repository (Closed)
|
Created:
Dec. 16, 2014, 2:05 p.m. by Wladimir Palant Modified:
Dec. 17, 2014, 10:01 p.m. Reviewers:
Thomas Greiner Visibility:
Public. |
DescriptionA few pretty straightforward changes here. ext/common.js isn`t trivial to follow unfortunately - that`s code from Safari but modified, mostly it`s locale identifier mangling and some Gecko-specific syntax removed. The test server is only necessary for Chrome, in other browsers the page can be tested directly.
Patch Set 1 #
Total comments: 11
Patch Set 2 : More common approach to wrapping code #
Total comments: 8
Patch Set 3 : Addressed comments #
MessagesTotal messages: 9
http://codereview.adblockplus.org/6427347985104896/diff/5629499534213120/ext/... File ext/common.js (right): http://codereview.adblockplus.org/6427347985104896/diff/5629499534213120/ext/... ext/common.js:18: this.ext = function(ext) Why do you use `this.ext` here when we usually use `window.ext`? http://codereview.adblockplus.org/6427347985104896/diff/5629499534213120/ext/... ext/common.js:90: xhr.open("GET", "locale/" + locale + "/" + catalogFile, false); I suppose this is the reason why test_server.py is required to test it on Chrome. :) http://codereview.adblockplus.org/6427347985104896/diff/5629499534213120/ext/... ext/common.js:141: }(this.ext || {}); Unless you expect this to run in a context where `window` is not the global namespace I'd suggest that rather than passing values around all over the place it would be simpler to have this be a function that looks like this to have all of that code be in one place: (function() { if (!window.ext) window.ext = {}; var ext = window.ext; ... ext.i18n = { ... }; })(); http://codereview.adblockplus.org/6427347985104896/diff/5629499534213120/firs... File firstRun.html (right): http://codereview.adblockplus.org/6427347985104896/diff/5629499534213120/firs... firstRun.html:25: <script type="text/javascript" src="ext/content.js"></script> ext/content.js doesn't exist in adblockplusui so adding an empty file should be sufficient to avoid errors http://codereview.adblockplus.org/6427347985104896/diff/5629499534213120/test... File test_server.py (right): http://codereview.adblockplus.org/6427347985104896/diff/5629499534213120/test... test_server.py:3: # This file is part of the Adblock Plus <https://adblockplus.org/>, Interestingly, you used the HTTPS version of adblockplus.org here while we still use the HTTP version in (most) other files. We should change that in all other headers at some point but for this review I'd consider it inconsistent.
http://codereview.adblockplus.org/6427347985104896/diff/5629499534213120/ext/... File ext/common.js (right): http://codereview.adblockplus.org/6427347985104896/diff/5629499534213120/ext/... ext/common.js:18: this.ext = function(ext) On 2014/12/17 11:24:44, Thomas Greiner wrote: > Why do you use `this.ext` here when we usually use `window.ext`? I simply don't like assuming that this code is running in window context - this is the more universal way to access the global object. http://codereview.adblockplus.org/6427347985104896/diff/5629499534213120/ext/... ext/common.js:90: xhr.open("GET", "locale/" + locale + "/" + catalogFile, false); On 2014/12/17 11:24:44, Thomas Greiner wrote: > I suppose this is the reason why test_server.py is required to test it on > Chrome. :) You are correct, Chrome won't allow accessing any files unless a special command line flag is passed. http://codereview.adblockplus.org/6427347985104896/diff/5629499534213120/ext/... ext/common.js:141: }(this.ext || {}); On 2014/12/17 11:24:44, Thomas Greiner wrote: > Unless you expect this to run in a context where `window` is not the global > namespace I'd suggest that rather than passing values around all over the place > it would be simpler to have this be a function that looks like this to have all > of that code be in one place: It's probably cleaner to simply pass in the global object. http://codereview.adblockplus.org/6427347985104896/diff/5629499534213120/firs... File firstRun.html (right): http://codereview.adblockplus.org/6427347985104896/diff/5629499534213120/firs... firstRun.html:25: <script type="text/javascript" src="ext/content.js"></script> On 2014/12/17 11:24:44, Thomas Greiner wrote: > ext/content.js doesn't exist in adblockplusui so adding an empty file should be > sufficient to avoid errors It's being added in the next review. http://codereview.adblockplus.org/6427347985104896/diff/5629499534213120/test... File test_server.py (right): http://codereview.adblockplus.org/6427347985104896/diff/5629499534213120/test... test_server.py:3: # This file is part of the Adblock Plus <https://adblockplus.org/>, On 2014/12/17 11:24:44, Thomas Greiner wrote: > Interestingly, you used the HTTPS version of http://adblockplus.org here while we still > use the HTTP version in (most) other files. We should change that in all other > headers at some point but for this review I'd consider it inconsistent. Yes, I noticed the wrong URL - I want to fix it everywhere else in January when we'll have to update the copyright year anyway.
http://codereview.adblockplus.org/6427347985104896/diff/5629499534213120/test... File test_server.py (right): http://codereview.adblockplus.org/6427347985104896/diff/5629499534213120/test... test_server.py:3: # This file is part of the Adblock Plus <https://adblockplus.org/>, On 2014/12/17 13:00:38, Wladimir Palant wrote: > On 2014/12/17 11:24:44, Thomas Greiner wrote: > > Interestingly, you used the HTTPS version of http://adblockplus.org here while > we still > > use the HTTP version in (most) other files. We should change that in all other > > headers at some point but for this review I'd consider it inconsistent. > > Yes, I noticed the wrong URL - I want to fix it everywhere else in January when > we'll have to update the copyright year anyway. Makes sense. BTW it says "of the Adblock Plus" which is "of Adblock Plus" in the other files. http://codereview.adblockplus.org/6427347985104896/diff/5685265389584384/ext/... File ext/common.js (right): http://codereview.adblockplus.org/6427347985104896/diff/5685265389584384/ext/... ext/common.js:55: var selectedLocale = navigator.language; Change to `global.navigator.language` for consistency with lines 56 and 62. http://codereview.adblockplus.org/6427347985104896/diff/5685265389584384/ext/... ext/common.js:56: var match = /[?&]locale=([\w\-]+)/.exec(window.location.search); Change to `global.location.search` http://codereview.adblockplus.org/6427347985104896/diff/5685265389584384/ext/... ext/common.js:62: var catalogFile = window.location.pathname.replace(/.*\//, "").replace(/\..*/, "") + ".json"; Change to `global.location.pathname` http://codereview.adblockplus.org/6427347985104896/diff/5685265389584384/test... File test_server.py (right): http://codereview.adblockplus.org/6427347985104896/diff/5685265389584384/test... test_server.py:1: #!/usr/bin/env python I noticed that you mention the dependecy on Flask in README.md at http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/READ... Couldn't we just use Python's built-in HTTP server using `python -m SimpleHTTPServer 5000` to avoid Flask altogether? The only dynamic part of firstRun.html has been removed by this review anyway.
http://codereview.adblockplus.org/6427347985104896/diff/5685265389584384/ext/... File ext/common.js (right): http://codereview.adblockplus.org/6427347985104896/diff/5685265389584384/ext/... ext/common.js:55: var selectedLocale = navigator.language; On 2014/12/17 13:43:40, Thomas Greiner wrote: > Change to `global.navigator.language` for consistency with lines 56 and 62. navigator isn't a property of generic global objects, only that of the window object. Same with the comments below. If anything, it should say window.navigator.locale explicitly (which I prefer, but Sebastian preferred not to change when this code was originally reviewed). http://codereview.adblockplus.org/6427347985104896/diff/5685265389584384/test... File test_server.py (right): http://codereview.adblockplus.org/6427347985104896/diff/5685265389584384/test... test_server.py:1: #!/usr/bin/env python On 2014/12/17 13:43:40, Thomas Greiner wrote: > Couldn't we just use Python's built-in HTTP server using `python -m > SimpleHTTPServer 5000` to avoid Flask altogether? The only dynamic part of > firstRun.html has been removed by this review anyway. Nice catch, I didn't know this module. It seems to be good enough for what we need here.
http://codereview.adblockplus.org/6427347985104896/diff/5685265389584384/ext/... File ext/common.js (right): http://codereview.adblockplus.org/6427347985104896/diff/5685265389584384/ext/... ext/common.js:55: var selectedLocale = navigator.language; On 2014/12/17 14:19:45, Wladimir Palant wrote: > On 2014/12/17 13:43:40, Thomas Greiner wrote: > > Change to `global.navigator.language` for consistency with lines 56 and 62. > > navigator isn't a property of generic global objects, only that of the window > object. Same with the comments below. If anything, it should say > window.navigator.locale explicitly (which I prefer, but Sebastian preferred not > to change when this code was originally reviewed). Do you have a relevant scenario in mind in which we have direct global access to the `window` object but where it isn't the global object?
http://codereview.adblockplus.org/6427347985104896/diff/5685265389584384/ext/... File ext/common.js (right): http://codereview.adblockplus.org/6427347985104896/diff/5685265389584384/ext/... ext/common.js:55: var selectedLocale = navigator.language; On 2014/12/17 15:13:52, Thomas Greiner wrote: > Do you have a relevant scenario in mind in which we have direct global access to > the `window` object but where it isn't the global object? Not here of course - but the implementation in Firefox should use the same approach to wrap code, and there it won't be running in a window context (it won't access the window object of course).
LGTM Note that synchronous XHR was deprecated by Firefox (see https://bugzilla.mozilla.org/show_bug.cgi?id=969671). Not an issue for now because I don't expect them to remove that anytime soon (if ever) but still worth noting.
On 2014/12/17 16:21:44, Thomas Greiner wrote: > Note that synchronous XHR was deprecated by Firefox (see > https://bugzilla.mozilla.org/show_bug.cgi?id=969671). Not an issue for now > because I don't expect them to remove that anytime soon (if ever) but still > worth noting. Yes, I've seen the warnings. But having asynchronous behavior here would really complicate things, so as long as this is our test environment only - not really an issue. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
