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

Issue 6432313504169984: Issue 1663 - Various Firefox-related changes of the first-run page (Closed)

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

Description

A bunch of issues only came up when testing in Firefox. 1) Platform-specific utils.js is pointless. 2) Social links should also be updated after subscription contents were downloaded. 3) i18n.js includes lots of Firefox-specific functionality, this one should move into the abstraction layer now that we have one for Firefox. 4) messageResponder.js shouldn't assume that all classes are already defined in the global context - it should require() them explicitly instead. 5) |global| parameter not used consistently in a few places. 6) Synchronous approach to getting application locale doesn't work in Firefox, messaging has to be used there.

Patch Set 1 #

Patch Set 2 : Updated fake background page #

Total comments: 12

Patch Set 3 : Addressed comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+157 lines, -196 lines) Patch
M README.md View 1 chunk +0 lines, -1 line 0 comments Download
M background.js View 1 1 chunk +108 lines, -96 lines 0 comments Download
M ext/common.js View 1 2 1 chunk +1 line, -1 line 0 comments Download
M firstRun.html View 1 chunk +0 lines, -1 line 0 comments Download
M firstRun.js View 2 chunks +6 lines, -1 line 0 comments Download
M i18n.js View 1 2 3 chunks +16 lines, -68 lines 0 comments Download
M messageResponder.js View 1 2 3 chunks +26 lines, -4 lines 0 comments Download
R utils.js View 1 chunk +0 lines, -24 lines 0 comments Download

Messages

Total messages: 11
Wladimir Palant
4 years, 10 months ago (2015-01-06 22:19:07 UTC) #1
Wladimir Palant
Realized that I forgot to update the fake background page, done that as well (lots ...
4 years, 10 months ago (2015-01-06 22:35:20 UTC) #2
Sebastian Noack
LGTM
4 years, 10 months ago (2015-01-07 09:57:33 UTC) #3
Thomas Greiner
http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/i18n.js File i18n.js (right): http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/i18n.js#newcode22 i18n.js:22: if (ext.i18n.noSpecialMessages) Nit: Shouldn't we avoid negatives like that? ...
4 years, 10 months ago (2015-01-07 12:19:54 UTC) #4
Sebastian Noack
http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/i18n.js File i18n.js (right): http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/i18n.js#newcode22 i18n.js:22: if (ext.i18n.noSpecialMessages) On 2015/01/07 12:19:55, Thomas Greiner wrote: > ...
4 years, 10 months ago (2015-01-07 12:22:17 UTC) #5
Sebastian Noack
http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/messageResponder.js File messageResponder.js (right): http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/messageResponder.js#newcode20 messageResponder.js:20: if (typeof ext == "undefined") On 2015/01/07 12:19:55, Thomas ...
4 years, 10 months ago (2015-01-07 12:23:21 UTC) #6
Wladimir Palant
I addressed the comments with the exception of adding a nested switch statement. http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/i18n.js File ...
4 years, 10 months ago (2015-01-07 16:33:10 UTC) #7
Sebastian Noack
LGTM http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/i18n.js File i18n.js (right): http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/i18n.js#newcode22 i18n.js:22: if (ext.i18n.noSpecialMessages) On 2015/01/07 16:33:10, Wladimir Palant wrote: ...
4 years, 10 months ago (2015-01-07 16:40:55 UTC) #8
Wladimir Palant
http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/i18n.js File i18n.js (right): http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/i18n.js#newcode22 i18n.js:22: if (ext.i18n.noSpecialMessages) On 2015/01/07 16:40:56, Sebastian Noack wrote: > ...
4 years, 10 months ago (2015-01-07 16:52:40 UTC) #9
Thomas Greiner
LGTM http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/messageResponder.js File messageResponder.js (right): http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/messageResponder.js#newcode97 messageResponder.js:97: if (message.what == "issues") On 2015/01/07 16:33:10, Wladimir ...
4 years, 10 months ago (2015-01-07 17:31:38 UTC) #10
Wladimir Palant
4 years, 10 months ago (2015-01-07 19:03:39 UTC) #11
http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/mess...
File messageResponder.js (right):

http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/mess...
messageResponder.js:97: if (message.what == "issues")
On 2015/01/07 17:31:38, Thomas Greiner wrote:
> Generally, how about introducing brackets for switch-statements to our coding
> style to make them more legible. It would also eliminate the need to consider
> the exception that's specified in Mozilla's coding style: "When you need to
> declare a variable in a switch, put the block in braces" (see
>
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style...)

While I am inclined to agree, this will unfortunately make switch statements
even more verbose than they already are. And it's still going to require two
indentation levels.
Sign in to reply to this message.

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