Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(955)

Issue 6427347985104896: Issue 1663 - Making first-run page testable in the adblockplusui repository (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 11 months ago by Wladimir Palant
Modified:
4 years, 11 months ago
Reviewers:
Thomas Greiner
Visibility:
Public.

Description

A 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+173 lines, -3 lines) Patch
A ext/common.js View 1 2 1 chunk +143 lines, -0 lines 0 comments Download
M firstRun.html View 1 chunk +1 line, -3 lines 0 comments Download
A test_server.py View 1 2 1 chunk +29 lines, -0 lines 0 comments Download

Messages

Total messages: 9
Wladimir Palant
4 years, 11 months ago (2014-12-16 14:05:07 UTC) #1
Thomas Greiner
http://codereview.adblockplus.org/6427347985104896/diff/5629499534213120/ext/common.js File ext/common.js (right): http://codereview.adblockplus.org/6427347985104896/diff/5629499534213120/ext/common.js#newcode18 ext/common.js:18: this.ext = function(ext) Why do you use `this.ext` here ...
4 years, 11 months ago (2014-12-17 11:24:44 UTC) #2
Wladimir Palant
http://codereview.adblockplus.org/6427347985104896/diff/5629499534213120/ext/common.js File ext/common.js (right): http://codereview.adblockplus.org/6427347985104896/diff/5629499534213120/ext/common.js#newcode18 ext/common.js:18: this.ext = function(ext) On 2014/12/17 11:24:44, Thomas Greiner wrote: ...
4 years, 11 months ago (2014-12-17 13:00:38 UTC) #3
Thomas Greiner
http://codereview.adblockplus.org/6427347985104896/diff/5629499534213120/test_server.py File test_server.py (right): http://codereview.adblockplus.org/6427347985104896/diff/5629499534213120/test_server.py#newcode3 test_server.py:3: # This file is part of the Adblock Plus ...
4 years, 11 months ago (2014-12-17 13:43:40 UTC) #4
Wladimir Palant
http://codereview.adblockplus.org/6427347985104896/diff/5685265389584384/ext/common.js File ext/common.js (right): http://codereview.adblockplus.org/6427347985104896/diff/5685265389584384/ext/common.js#newcode55 ext/common.js:55: var selectedLocale = navigator.language; On 2014/12/17 13:43:40, Thomas Greiner ...
4 years, 11 months ago (2014-12-17 14:19:44 UTC) #5
Thomas Greiner
http://codereview.adblockplus.org/6427347985104896/diff/5685265389584384/ext/common.js File ext/common.js (right): http://codereview.adblockplus.org/6427347985104896/diff/5685265389584384/ext/common.js#newcode55 ext/common.js:55: var selectedLocale = navigator.language; On 2014/12/17 14:19:45, Wladimir Palant ...
4 years, 11 months ago (2014-12-17 15:13:52 UTC) #6
Wladimir Palant
http://codereview.adblockplus.org/6427347985104896/diff/5685265389584384/ext/common.js File ext/common.js (right): http://codereview.adblockplus.org/6427347985104896/diff/5685265389584384/ext/common.js#newcode55 ext/common.js:55: var selectedLocale = navigator.language; On 2014/12/17 15:13:52, Thomas Greiner ...
4 years, 11 months ago (2014-12-17 15:25:54 UTC) #7
Thomas Greiner
LGTM Note that synchronous XHR was deprecated by Firefox (see https://bugzilla.mozilla.org/show_bug.cgi?id=969671). Not an issue for ...
4 years, 11 months ago (2014-12-17 16:21:44 UTC) #8
Wladimir Palant
4 years, 11 months ago (2014-12-17 16:24:21 UTC) #9
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5