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. Visibility:
Public. |
DescriptionIssue 1502 - Cache translation catalogs on Safari
Patch Set 1 #
Total comments: 8
Patch Set 2 : Renamed "source" variable to avoid confusion #
Total comments: 6
MessagesTotal messages: 11
I'm not sure I'm the best person to review this changeset as I'm not so familiar with the codebase. That said I'll give it a go, I've made some comments in line. http://codereview.adblockplus.org/6326608939974656/diff/5629499534213120/safa... File safari/ext/common.js (right): http://codereview.adblockplus.org/6326608939974656/diff/5629499534213120/safa... safari/ext/common.js:144: var source = JSON.parse(xhr.responseText); I found it confusing that this variable is called source and then part of it is passed to the parsing function as a parameter called source. Maybe rename the source var here or the source parameter above? http://codereview.adblockplus.org/6326608939974656/diff/5629499534213120/safa... safari/ext/common.js:160: var [text, placeholders] = message; I've never seen this syntax in JS before, sure it's valid? (I tried `var [a,b] = [1,2];` in the console just now and it didn't seem to work.)
http://codereview.adblockplus.org/6326608939974656/diff/5629499534213120/safa... File safari/ext/common.js (right): http://codereview.adblockplus.org/6326608939974656/diff/5629499534213120/safa... safari/ext/common.js:144: var source = JSON.parse(xhr.responseText); On 2014/10/28 19:12:16, kzar wrote: > I found it confusing that this variable is called source and then part of it is > passed to the parsing function as a parameter called source. Maybe rename the > source var here or the source parameter above? I could argue that this is a different context and "source" is a rather generic term, like "i" (short for "iteration"), which is used in for-loops all over the place. However, if you have a better idea how to call this and/or the other variable, I can rename them. http://codereview.adblockplus.org/6326608939974656/diff/5629499534213120/safa... safari/ext/common.js:160: var [text, placeholders] = message; On 2014/10/28 19:12:16, kzar wrote: > I've never seen this syntax in JS before, sure it's valid? (I tried `var [a,b] = > [1,2];` in the console just now and it didn't seem to work.) It's called destructuring assignment, and is a new feature in ES6: http://wiki.ecmascript.org/doku.php?id=harmony:destructuring It's not yet supported by Safari and Chrome. However, this code is processed by jshydra, which lets us use some new JavaScript syntax like this, and converts that line into following code: var text = message[0]; var placeholders = message[1];
http://codereview.adblockplus.org/6326608939974656/diff/5629499534213120/safa... File safari/ext/common.js (right): http://codereview.adblockplus.org/6326608939974656/diff/5629499534213120/safa... safari/ext/common.js:144: var source = JSON.parse(xhr.responseText); On 2014/10/28 19:35:53, Sebastian Noack wrote: > On 2014/10/28 19:12:16, kzar wrote: > > I found it confusing that this variable is called source and then part of it > is > > passed to the parsing function as a parameter called source. Maybe rename the > > source var here or the source parameter above? > > I could argue that this is a different context and "source" is a rather generic > term, like "i" (short for "iteration"), which is used in for-loops all over the > place. > > However, if you have a better idea how to call this and/or the other variable, I > can rename them. Perhaps the source variable here could be renamed "messages" and the source parameter above could be renamed "message"? http://codereview.adblockplus.org/6326608939974656/diff/5629499534213120/safa... safari/ext/common.js:160: var [text, placeholders] = message; On 2014/10/28 19:35:53, Sebastian Noack wrote: > On 2014/10/28 19:12:16, kzar wrote: > > I've never seen this syntax in JS before, sure it's valid? (I tried `var [a,b] > = > > [1,2];` in the console just now and it didn't seem to work.) > > It's called destructuring assignment, and is a new feature in ES6: > http://wiki.ecmascript.org/doku.php?id=harmony:destructuring > > It's not yet supported by Safari and Chrome. However, this code is processed by > jshydra, which lets us use some new JavaScript syntax like this, and converts > that line into following code: > > var text = message[0]; > var placeholders = message[1]; I guessed you were somehow right :p (Understand about destructuring from other languages but hadn't realised it was included in ES6!)
http://codereview.adblockplus.org/6326608939974656/diff/5629499534213120/safa... File safari/ext/common.js (right): http://codereview.adblockplus.org/6326608939974656/diff/5629499534213120/safa... safari/ext/common.js:144: var source = JSON.parse(xhr.responseText); On 2014/10/28 22:05:29, kzar wrote: > On 2014/10/28 19:35:53, Sebastian Noack wrote: > > On 2014/10/28 19:12:16, kzar wrote: > > > I found it confusing that this variable is called source and then part of it > > is > > > passed to the parsing function as a parameter called source. Maybe rename > the > > > source var here or the source parameter above? > > > > I could argue that this is a different context and "source" is a rather > generic > > term, like "i" (short for "iteration"), which is used in for-loops all over > the > > place. > > > > However, if you have a better idea how to call this and/or the other variable, > I > > can rename them. > > Perhaps the source variable here could be renamed "messages" and the source > parameter above could be renamed "message"? Not really consistent. Since the variable storing the parsed message is already called just "message", one might assume that "messages" would be a collection of them. However, I renamed this variable to "rawMessage" (since its parsed counterpart is called "message") and the other variable to "rawCatalog" (since its parsed counterpart is called "catalog").
LGTM http://codereview.adblockplus.org/6326608939974656/diff/5629499534213120/safa... File safari/ext/common.js (right): http://codereview.adblockplus.org/6326608939974656/diff/5629499534213120/safa... safari/ext/common.js:144: var source = JSON.parse(xhr.responseText); On 2014/10/29 08:08:54, Sebastian Noack wrote: > On 2014/10/28 22:05:29, kzar wrote: > > On 2014/10/28 19:35:53, Sebastian Noack wrote: > > > On 2014/10/28 19:12:16, kzar wrote: > > > > I found it confusing that this variable is called source and then part of > it > > > is > > > > passed to the parsing function as a parameter called source. Maybe rename > > the > > > > source var here or the source parameter above? > > > > > > I could argue that this is a different context and "source" is a rather > > generic > > > term, like "i" (short for "iteration"), which is used in for-loops all over > > the > > > place. > > > > > > However, if you have a better idea how to call this and/or the other > variable, > > I > > > can rename them. > > > > Perhaps the source variable here could be renamed "messages" and the source > > parameter above could be renamed "message"? > > Not really consistent. Since the variable storing the parsed message is already > called just "message", one might assume that "messages" would be a collection of > them. > > However, I renamed this variable to "rawMessage" (since its parsed counterpart > is called "message") and the other variable to "rawCatalog" (since its parsed > counterpart is called "catalog"). Cool that makes more sense to me now.
On 2014/10/30 12:40:15, kzar wrote: > LGTM > > http://codereview.adblockplus.org/6326608939974656/diff/5629499534213120/safa... > File safari/ext/common.js (right): > > http://codereview.adblockplus.org/6326608939974656/diff/5629499534213120/safa... > safari/ext/common.js:144: var source = JSON.parse(xhr.responseText); > On 2014/10/29 08:08:54, Sebastian Noack wrote: > > On 2014/10/28 22:05:29, kzar wrote: > > > On 2014/10/28 19:35:53, Sebastian Noack wrote: > > > > On 2014/10/28 19:12:16, kzar wrote: > > > > > I found it confusing that this variable is called source and then part > of > > it > > > > is > > > > > passed to the parsing function as a parameter called source. Maybe > rename > > > the > > > > > source var here or the source parameter above? > > > > > > > > I could argue that this is a different context and "source" is a rather > > > generic > > > > term, like "i" (short for "iteration"), which is used in for-loops all > over > > > the > > > > place. > > > > > > > > However, if you have a better idea how to call this and/or the other > > variable, > > > I > > > > can rename them. > > > > > > Perhaps the source variable here could be renamed "messages" and the source > > > parameter above could be renamed "message"? > > > > Not really consistent. Since the variable storing the parsed message is > already > > called just "message", one might assume that "messages" would be a collection > of > > them. > > > > However, I renamed this variable to "rawMessage" (since its parsed counterpart > > is called "message") and the other variable to "rawCatalog" (since its parsed > > counterpart is called "catalog"). > > Cool that makes more sense to me now. @Wladimir: Are you still going to do a full review, or are you fine with me pushing the changes?
On 2014/10/30 12:44:25, Sebastian Noack wrote: > @Wladimir: Are you still going to do a full review, or are you fine with me > pushing the changes? For reference, Wladimir just told me on IRC that he is fine with pushing this change, without having him review it first. So I did.
Note that I don't consider mixing in various unrelated changes into an otherwise simple change a good approach. I had to go through the entire code, figure out what moved where and where you actually changed something. The fact that you didn't describe your changes didn't help - this review is *not* about caching catalogs. Please don't waste reviewer's time... 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 || ""); Why not keep the more obvious text.replace(...)? And - no, I don't care which one is a split millisecond faster. http://codereview.adblockplus.org/6326608939974656/diff/5757334940811264/safa... safari/ext/common.js:113: for (var placeholder in rawMessage.placeholders) I didn't expect it but apparently one can iterate over null :/ http://codereview.adblockplus.org/6326608939974656/diff/5757334940811264/safa... safari/ext/common.js:118: placeholders[parseInt(content.substr(1)) - 1] = placeholder; Doing both regular expressions and string manipulation is unnecessary here and obfuscates the meaning. How about: var match = /^\$(\d+)$/.exec(content); if (match) placeholders[parseInt(match[1], 10)] = placeholder; Note that the second parameter to parseInt() is important. I'm not sure whether all our supported Safari versions will have parseInt("017") == 17.
On 2014/10/30 23:00:44, Wladimir Palant wrote: > Note that I don't consider mixing in various unrelated changes into an otherwise > simple change a good approach. I had to go through the entire code, figure out > what moved where and where you actually changed something. The fact that you > didn't describe your changes didn't help - this review is *not* about caching > catalogs. I disagree. This change is about caching the "parsed" catalogs. So I separated the parsing from the lookups, which not only makes sense for performance reasons, but also made the code easier to understand.
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. 
|
