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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 3 months ago by Sebastian Noack
Modified:
5 years, 3 months ago
Reviewers:
Wladimir Palant, kzar
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
5 years, 3 months ago (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 ...
5 years, 3 months ago (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: ...
5 years, 3 months ago (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 ...
5 years, 3 months ago (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: ...
5 years, 3 months ago (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 ...
5 years, 3 months ago (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): > ...
5 years, 3 months ago (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 ...
5 years, 3 months ago (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 ...
5 years, 3 months ago (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 ...
5 years, 3 months ago (2014-10-30 23:52:43 UTC) #10
Sebastian Noack
5 years, 3 months ago (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.
Sign in to reply to this message.

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