Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 6326608939974656: Issue 1502 - Cache translation catalogs on Safari (Closed)

Created:
Oct. 28, 2014, 6:50 p.m. by Sebastian Noack
Modified:
Oct. 30, 2014, 11:53 p.m.
Reviewers:
kzar, Wladimir Palant
Visibility:
Public.

Description

Issue 1502 - Cache translation catalogs on Safari

Patch Set 1 #

Total comments: 8

Patch Set 2 : Renamed "source" variable to avoid confusion #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+51 lines, -55 lines) Patch
M safari/ext/common.js View 1 2 chunks +51 lines, -55 lines 6 comments Download

Messages

Total messages: 11
Sebastian Noack
Oct. 28, 2014, 6:50 p.m. (2014-10-28 18:50:52 UTC) #1
kzar
I'm not sure I'm the best person to review this changeset as I'm not so ...
Oct. 28, 2014, 7:12 p.m. (2014-10-28 19:12:16 UTC) #2
Sebastian Noack
http://codereview.adblockplus.org/6326608939974656/diff/5629499534213120/safari/ext/common.js File safari/ext/common.js (right): http://codereview.adblockplus.org/6326608939974656/diff/5629499534213120/safari/ext/common.js#newcode144 safari/ext/common.js:144: var source = JSON.parse(xhr.responseText); On 2014/10/28 19:12:16, kzar wrote: ...
Oct. 28, 2014, 7:35 p.m. (2014-10-28 19:35:52 UTC) #3
kzar
http://codereview.adblockplus.org/6326608939974656/diff/5629499534213120/safari/ext/common.js File safari/ext/common.js (right): http://codereview.adblockplus.org/6326608939974656/diff/5629499534213120/safari/ext/common.js#newcode144 safari/ext/common.js:144: var source = JSON.parse(xhr.responseText); On 2014/10/28 19:35:53, Sebastian Noack ...
Oct. 28, 2014, 10:05 p.m. (2014-10-28 22:05:28 UTC) #4
Sebastian Noack
http://codereview.adblockplus.org/6326608939974656/diff/5629499534213120/safari/ext/common.js File safari/ext/common.js (right): http://codereview.adblockplus.org/6326608939974656/diff/5629499534213120/safari/ext/common.js#newcode144 safari/ext/common.js:144: var source = JSON.parse(xhr.responseText); On 2014/10/28 22:05:29, kzar wrote: ...
Oct. 29, 2014, 8:08 a.m. (2014-10-29 08:08:53 UTC) #5
kzar
LGTM http://codereview.adblockplus.org/6326608939974656/diff/5629499534213120/safari/ext/common.js File safari/ext/common.js (right): http://codereview.adblockplus.org/6326608939974656/diff/5629499534213120/safari/ext/common.js#newcode144 safari/ext/common.js:144: var source = JSON.parse(xhr.responseText); On 2014/10/29 08:08:54, Sebastian ...
Oct. 30, 2014, 12:40 p.m. (2014-10-30 12:40:15 UTC) #6
Sebastian Noack
On 2014/10/30 12:40:15, kzar wrote: > LGTM > > http://codereview.adblockplus.org/6326608939974656/diff/5629499534213120/safari/ext/common.js > File safari/ext/common.js (right): > ...
Oct. 30, 2014, 12:44 p.m. (2014-10-30 12:44:25 UTC) #7
Sebastian Noack
On 2014/10/30 12:44:25, Sebastian Noack wrote: > @Wladimir: Are you still going to do a ...
Oct. 30, 2014, 1 p.m. (2014-10-30 13:00:44 UTC) #8
Wladimir Palant
Note that I don't consider mixing in various unrelated changes into an otherwise simple change ...
Oct. 30, 2014, 11 p.m. (2014-10-30 23:00:44 UTC) #9
Sebastian Noack
On 2014/10/30 23:00:44, Wladimir Palant wrote: > Note that I don't consider mixing in various ...
Oct. 30, 2014, 11:52 p.m. (2014-10-30 23:52:43 UTC) #10
Sebastian Noack
Oct. 30, 2014, 11:53 p.m. (2014-10-30 23:53:01 UTC) #11
http://codereview.adblockplus.org/6326608939974656/diff/5757334940811264/safa...
File safari/ext/common.js (right):

http://codereview.adblockplus.org/6326608939974656/diff/5757334940811264/safa...
safari/ext/common.js:105: return text.split("$" + placeholder +
"$").join(content || "");
On 2014/10/30 23:00:44, Wladimir Palant wrote:
> Why not keep the more obvious text.replace(...)? And - no, I don't care which
> one is a split millisecond faster.

Because .replace() with a string as first argument only replaces the first
occurrence of that string. I could have used .replace() with a regex using the g
flag as first argument. However, since the string is variable it would require
escaping. So using .split().join() is simplest way to replace all occurrences of
a variable string.

The old code was just broken. So yes, this change is unrelated. However, I had
to restructure that code anyway, and didn't mind fixing it while doing so.

http://codereview.adblockplus.org/6326608939974656/diff/5757334940811264/safa...
safari/ext/common.js:113: for (var placeholder in rawMessage.placeholders)
On 2014/10/30 23:00:44, Wladimir Palant wrote:
> I didn't expect it but apparently one can iterate over null :/

You can. Iterating over null (or undefined) has the same result as iterating
over an empty object. While talking about unrelated changes, note that the old
code also didn't mind checking for null here. ;)

http://codereview.adblockplus.org/6326608939974656/diff/5757334940811264/safa...
safari/ext/common.js:118: placeholders[parseInt(content.substr(1)) - 1] =
placeholder;
On 2014/10/30 23:00:44, Wladimir Palant wrote:
> Note that the second parameter to parseInt() is important. I'm not sure
whether
> all our supported Safari versions will have parseInt("017") == 17.

Neither the old code, nor most other calls to parseInt() in this repository,
specify a radix. I'm going to file a separate issue for that.

Powered by Google App Engine
This is Rietveld