Issue 29350042: Issue 4023 - Move storage of subscription lists to localStorage (Closed)
|
Created:
Aug. 22, 2016, 9:24 a.m. by Oleksandr Modified:
Sept. 13, 2016, 1:19 p.m. Visibility:
Public. |
DescriptionIssue 4023 - Move storage of subscription lists to localStorage
Patch Set 1 #
Total comments: 37
Patch Set 2 : Cleanup #
Total comments: 17
Patch Set 3 : Fix formatting #
Total comments: 1
Patch Set 4 : Add full stop after a comment and fix formatting #
Total comments: 3
Patch Set 5 : Make sure entry["compressed"] is truthful #
MessagesTotal messages: 13
Dumb question but are these changes (and the other review) for https://bitbucket.org/sebastian_noack/adblockplusedge or the main adblockpluschrome repository?
https://codereview.adblockplus.org/29350042/diff/29350043/lib/io.js File lib/io.js (left): https://codereview.adblockplus.org/29350042/diff/29350043/lib/io.js#oldcode78 lib/io.js:78: copyFile: function(fromFile, toFile, callback) copyFile and renameFile have been removed? https://codereview.adblockplus.org/29350042/diff/29350043/lib/io.js File lib/io.js (right): https://codereview.adblockplus.org/29350042/diff/29350043/lib/io.js#newcode44 lib/io.js:44: entry = null; Nit: `entry = null` seems redundant since we already know entry is falsey here. https://codereview.adblockplus.org/29350042/diff/29350043/lib/io.js#newcode50 lib/io.js:50: { Nit: Please remove the braces for the else clause. https://codereview.adblockplus.org/29350042/diff/29350043/lib/io.js#newcode84 lib/io.js:84: var key = fileToKey(file); You seem to use let and var inconsistently throughout. https://codereview.adblockplus.org/29350042/diff/29350043/lib/io.js#newcode88 lib/io.js:88: if (typeof browser == "undefined") Since this code is for the adblockplusedge repository I wonder why we bother do the `typeof browser == "undefined"` check at all. We already know this is Edge right? https://codereview.adblockplus.org/29350042/diff/29350043/lib/io.js#newcode97 lib/io.js:97: var dataString = JSON.stringify(data); Perhaps do the JSON serialization and compression in one step? That would be consistent with how you do the reverse process and I guess would avoid keeping a reference to both. (Not sure that keeping a reference to both would matter, but since they're both large strings maybe it would.) https://codereview.adblockplus.org/29350042/diff/29350043/lib/io.js#newcode99 lib/io.js:99: chrome.storage.local.remove(key); Did you mean to use chrome.storage instead of ext.storage here? https://codereview.adblockplus.org/29350042/diff/29350043/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29350042/diff/29350043/lib/prefs.js#newcod... lib/prefs.js:260: ext.storage.set(items); I wonder if Edge supports (and JS Hydra doesn't mess up) the new syntax for computed Object properties[1]? If so I think we could do something like this: `ext.storage.set({[prefToKey(pref)]: value});` [1] - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/O... https://codereview.adblockplus.org/29350042/diff/29350043/qunit/tests/io.js File qunit/tests/io.js (right): https://codereview.adblockplus.org/29350042/diff/29350043/qunit/tests/io.js#n... qunit/tests/io.js:19: var IO = require("io").IO; You're requiring the same module twice here, I don't see why. https://codereview.adblockplus.org/29350042/diff/29350043/qunit/tests/io.js#n... qunit/tests/io.js:23: test("Test writing entry larger than 1Mb but smaller than 5Mb", function(assert) Nit: If we're just supporting Edge anyway I guess we could use fancy stuff like arrow notation here? (Would also avoid the line being too long that way.) https://codereview.adblockplus.org/29350042/diff/29350043/qunit/tests/io.js#n... qunit/tests/io.js:32: var testDataOver5Mb = new Array(300000); Indentation seems wrong, should be two spaces. https://codereview.adblockplus.org/29350042/diff/29350043/qunit/tests/io.js#n... qunit/tests/io.js:43: readData: new Array(), Why not array literal? (Same elsewhere.) https://codereview.adblockplus.org/29350042/diff/29350043/qunit/tests/io.js#n... qunit/tests/io.js:49: function testReadWrite(data, fileName, assert) Please put the testReadWrite function above where it's first used. https://codereview.adblockplus.org/29350042/diff/29350043/qunit/tests/io.js#n... qunit/tests/io.js:51: // For debugging purposes clear the storage Will this trash the user's saved filters? Also this won't be called at the end of the last test so I guess junk will be left in storage after running the tests? Is it possible to use setUp / tearDown functions instead of putting that stuff here? https://codereview.adblockplus.org/29350042/diff/29350043/qunit/tests/io.js#n... qunit/tests/io.js:65: IO.readFromFile(IO.resolveFilePath(fileName), Nit: Mind indenting this like so? IO.readFromFile( IO.resolveFilePath(fileName), function() { ... } ); That way you'll avoid having some lines that are too long. https://codereview.adblockplus.org/29350042/diff/29350043/safari/ext/backgrou... File safari/ext/background.js (right): https://codereview.adblockplus.org/29350042/diff/29350043/safari/ext/backgrou... safari/ext/background.js:543: set: function(items, callback) I guess it's good not to break stuff, but if these changes are for the adblockplusedge repository surely we can just delete the whole safari directory and everything that references it?
https://codereview.adblockplus.org/29350042/diff/29350043/lib/io.js File lib/io.js (right): https://codereview.adblockplus.org/29350042/diff/29350043/lib/io.js#newcode88 lib/io.js:88: if (typeof browser == "undefined") On 2016/08/22 15:38:32, kzar wrote: > Since this code is for the adblockplusedge repository I wonder why we bother do > the `typeof browser == "undefined"` check at all. We already know this is Edge > right? As discussed on IRC, this should go into a bookmark in adblockpluschrome rather than into a separate repo. Moreover, at some point we will merge those changes upstream, using this code on Edge, Chrome, and Safari. https://codereview.adblockplus.org/29350042/diff/29350043/lib/lz-string.js File lib/lz-string.js (right): https://codereview.adblockplus.org/29350042/diff/29350043/lib/lz-string.js#ne... lib/lz-string.js:3: // under the terms of the WTFPL, Version 2 While this license should theoretically work for us, since it seems to be compatible with GPLv3, while also allowing to re-license the code if needed, it would be better to use a library under a more serious and approved license (e.g. MIT, CC0). But at the very least, please check with Judith from legal, so that she knows about this third-party code and can approve the license.
https://codereview.adblockplus.org/29350042/diff/29350043/lib/lz-string.js File lib/lz-string.js (right): https://codereview.adblockplus.org/29350042/diff/29350043/lib/lz-string.js#ne... lib/lz-string.js:3: // under the terms of the WTFPL, Version 2 On 2016/08/22 16:49:19, Sebastian Noack wrote: > While this license should theoretically work for us, since it seems to be > compatible with GPLv3, while also allowing to re-license the code if needed, it > would be better to use a library under a more serious and approved license (e.g. > MIT, CC0). But at the very least, please check with Judith from legal, so that > she knows about this third-party code and can approve the license. Done. Judith is fine with this. https://codereview.adblockplus.org/29350042/diff/29350043/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29350042/diff/29350043/lib/prefs.js#newcod... lib/prefs.js:260: ext.storage.set(items); On 2016/08/22 15:38:32, kzar wrote: > I wonder if Edge supports (and JS Hydra doesn't mess up) the new syntax for > computed Object properties[1]? If so I think we could do something like this: > `ext.storage.set({[prefToKey(pref)]: value});` > > [1] - > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/O... JS Hydra does not let this through.
Looking much better, mostly nits left in the code now. A couple of general things for code reviews however: 1. When publishing a new patch set please start the message with the patch set name like "Patch Set 2 : Cleanup". 2. Please respond to all inline comments. If you've done what was asked just click to add a reply of "Done.". If a question was asked or you've decided not to make the change asked please reply with a more detailed message. (Please can you go back and respond to all my existing inline comments in the review as well.) Those make it much easier for the reviewers, sorry I know it's kind of a pain. https://codereview.adblockplus.org/29350042/diff/29350193/lib/io.js File lib/io.js (right): https://codereview.adblockplus.org/29350042/diff/29350193/lib/io.js#newcode81 lib/io.js:81: // Edge cannot write files larger than 1Mb (500k string in UTF-16). I think this comment should be inside the else block, since that's the Edge related code. https://codereview.adblockplus.org/29350042/diff/29350193/lib/io.js#newcode85 lib/io.js:85: entry[key] = {lastModified: Date.now(), Nit: No need to wrap this into two lines if it will fit on one. https://codereview.adblockplus.org/29350042/diff/29350193/lib/io.js#newcode94 lib/io.js:94: entry[key] = {lastModified: Date.now(), Nit: This could fit on two lines instead of three. https://codereview.adblockplus.org/29350042/diff/29350193/qunit/tests/io.js File qunit/tests/io.js (right): https://codereview.adblockplus.org/29350042/diff/29350193/qunit/tests/io.js#n... qunit/tests/io.js:20: module("IO validation", Nit: We would normally have the opening bracket on this line like so: module("IO validation", { https://codereview.adblockplus.org/29350042/diff/29350193/qunit/tests/io.js#n... qunit/tests/io.js:24: if (typeof browser != "undefined") Nit: Mind checking for `== "undefined"` and switching these around? https://codereview.adblockplus.org/29350042/diff/29350193/qunit/tests/io.js#n... qunit/tests/io.js:49: { Nit: No braces required here. https://codereview.adblockplus.org/29350042/diff/29350193/qunit/tests/io.js#n... qunit/tests/io.js:65: equal(dummyParser.readData.length, data.length, Nit: This is indented incorrectly. Please could you do it something like this? equal( dummyParser.readData.length, data.length, "Check if read entry is the same size as written" ); Or equally something like this: equal(dummyParser.readData.length, data.length, "Check if read entry is the same size as written"); (Same below.) https://codereview.adblockplus.org/29350042/diff/29350193/qunit/tests/io.js#n... qunit/tests/io.js:75: test("Test writing entry larger than 1Mb but smaller than 5Mb", (assert) => Nit: The functions with one arguments using this syntax don't need the parentheses. `assert => { ... }` is the same as `(assert) => { ... }`. Same elsewhere.
Patch set 3 : fix formatting. https://codereview.adblockplus.org/29350042/diff/29350043/lib/io.js File lib/io.js (left): https://codereview.adblockplus.org/29350042/diff/29350043/lib/io.js#oldcode78 lib/io.js:78: copyFile: function(fromFile, toFile, callback) On 2016/08/22 15:38:32, kzar wrote: > copyFile and renameFile have been removed? Yes. https://codereview.adblockplus.org/29350042/diff/29350043/lib/io.js File lib/io.js (right): https://codereview.adblockplus.org/29350042/diff/29350043/lib/io.js#newcode44 lib/io.js:44: entry = null; On 2016/08/22 15:38:32, kzar wrote: > Nit: `entry = null` seems redundant since we already know entry is falsey here. Done. https://codereview.adblockplus.org/29350042/diff/29350043/lib/io.js#newcode50 lib/io.js:50: { On 2016/08/22 15:38:32, kzar wrote: > Nit: Please remove the braces for the else clause. Done. https://codereview.adblockplus.org/29350042/diff/29350043/lib/io.js#newcode84 lib/io.js:84: var key = fileToKey(file); On 2016/08/22 15:38:32, kzar wrote: > You seem to use let and var inconsistently throughout. Done. https://codereview.adblockplus.org/29350042/diff/29350043/lib/io.js#newcode88 lib/io.js:88: if (typeof browser == "undefined") On 2016/08/22 16:49:19, Sebastian Noack wrote: > On 2016/08/22 15:38:32, kzar wrote: > > Since this code is for the adblockplusedge repository I wonder why we bother > do > > the `typeof browser == "undefined"` check at all. We already know this is Edge > > right? > > As discussed on IRC, this should go into a bookmark in adblockpluschrome rather > than into a separate repo. Moreover, at some point we will merge those changes > upstream, using this code on Edge, Chrome, and Safari. Acknowledged. https://codereview.adblockplus.org/29350042/diff/29350043/lib/io.js#newcode97 lib/io.js:97: var dataString = JSON.stringify(data); On 2016/08/22 15:38:32, kzar wrote: > Perhaps do the JSON serialization and compression in one step? That would be > consistent with how you do the reverse process and I guess would avoid keeping a > reference to both. (Not sure that keeping a reference to both would matter, but > since they're both large strings maybe it would.) Done. https://codereview.adblockplus.org/29350042/diff/29350043/lib/io.js#newcode99 lib/io.js:99: chrome.storage.local.remove(key); On 2016/08/22 15:38:32, kzar wrote: > Did you mean to use chrome.storage instead of ext.storage here? Done. https://codereview.adblockplus.org/29350042/diff/29350043/qunit/tests/io.js File qunit/tests/io.js (right): https://codereview.adblockplus.org/29350042/diff/29350043/qunit/tests/io.js#n... qunit/tests/io.js:19: var IO = require("io").IO; On 2016/08/22 15:38:33, kzar wrote: > You're requiring the same module twice here, I don't see why. Done. https://codereview.adblockplus.org/29350042/diff/29350043/qunit/tests/io.js#n... qunit/tests/io.js:23: test("Test writing entry larger than 1Mb but smaller than 5Mb", function(assert) On 2016/08/22 15:38:33, kzar wrote: > Nit: If we're just supporting Edge anyway I guess we could use fancy stuff like > arrow notation here? (Would also avoid the line being too long that way.) Done. https://codereview.adblockplus.org/29350042/diff/29350043/qunit/tests/io.js#n... qunit/tests/io.js:32: var testDataOver5Mb = new Array(300000); On 2016/08/22 15:38:33, kzar wrote: > Indentation seems wrong, should be two spaces. Done. https://codereview.adblockplus.org/29350042/diff/29350043/qunit/tests/io.js#n... qunit/tests/io.js:43: readData: new Array(), On 2016/08/22 15:38:33, kzar wrote: > Why not array literal? (Same elsewhere.) Done. https://codereview.adblockplus.org/29350042/diff/29350043/qunit/tests/io.js#n... qunit/tests/io.js:51: // For debugging purposes clear the storage On 2016/08/22 15:38:32, kzar wrote: > Will this trash the user's saved filters? Also this won't be called at the end > of the last test so I guess junk will be left in storage after running the > tests? Is it possible to use setUp / tearDown functions instead of putting that > stuff here? This was just a comment I forgot to remove. The code for clearing storage wasn't here. I have added afterEach method per your suggestion. https://codereview.adblockplus.org/29350042/diff/29350043/qunit/tests/io.js#n... qunit/tests/io.js:65: IO.readFromFile(IO.resolveFilePath(fileName), On 2016/08/22 15:38:33, kzar wrote: > Nit: Mind indenting this like so? > > IO.readFromFile( > IO.resolveFilePath(fileName), > function() > { > ... > } > ); > > That way you'll avoid having some lines that are too long. Done. https://codereview.adblockplus.org/29350042/diff/29350043/safari/ext/backgrou... File safari/ext/background.js (right): https://codereview.adblockplus.org/29350042/diff/29350043/safari/ext/backgrou... safari/ext/background.js:543: set: function(items, callback) On 2016/08/22 15:38:33, kzar wrote: > I guess it's good not to break stuff, but if these changes are for the > adblockplusedge repository surely we can just delete the whole safari directory > and everything that references it? The idea is to commit with a bookmark to adblockpluschrome. https://codereview.adblockplus.org/29350042/diff/29350193/lib/io.js File lib/io.js (right): https://codereview.adblockplus.org/29350042/diff/29350193/lib/io.js#newcode81 lib/io.js:81: // Edge cannot write files larger than 1Mb (500k string in UTF-16). On 2016/08/30 14:13:02, kzar wrote: > I think this comment should be inside the else block, since that's the Edge > related code. Done. https://codereview.adblockplus.org/29350042/diff/29350193/lib/io.js#newcode85 lib/io.js:85: entry[key] = {lastModified: Date.now(), On 2016/08/30 14:13:03, kzar wrote: > Nit: No need to wrap this into two lines if it will fit on one. Done. https://codereview.adblockplus.org/29350042/diff/29350193/lib/io.js#newcode94 lib/io.js:94: entry[key] = {lastModified: Date.now(), On 2016/08/30 14:13:02, kzar wrote: > Nit: This could fit on two lines instead of three. Done. However it looked better as it was, IMHO. https://codereview.adblockplus.org/29350042/diff/29350193/qunit/tests/io.js File qunit/tests/io.js (right): https://codereview.adblockplus.org/29350042/diff/29350193/qunit/tests/io.js#n... qunit/tests/io.js:20: module("IO validation", On 2016/08/30 14:13:03, kzar wrote: > Nit: We would normally have the opening bracket on this line like so: > > module("IO validation", { Done. https://codereview.adblockplus.org/29350042/diff/29350193/qunit/tests/io.js#n... qunit/tests/io.js:24: if (typeof browser != "undefined") On 2016/08/30 14:13:03, kzar wrote: > Nit: Mind checking for `== "undefined"` and switching these around? Done. https://codereview.adblockplus.org/29350042/diff/29350193/qunit/tests/io.js#n... qunit/tests/io.js:49: { On 2016/08/30 14:13:03, kzar wrote: > Nit: No braces required here. Done. https://codereview.adblockplus.org/29350042/diff/29350193/qunit/tests/io.js#n... qunit/tests/io.js:65: equal(dummyParser.readData.length, data.length, On 2016/08/30 14:13:03, kzar wrote: > Nit: This is indented incorrectly. Please could you do it something like this? > > equal( > dummyParser.readData.length, data.length, > "Check if read entry is the same size as written" > ); > > Or equally something like this: > > equal(dummyParser.readData.length, data.length, > "Check if read entry is the same size as written"); > > (Same below.) Done. https://codereview.adblockplus.org/29350042/diff/29350193/qunit/tests/io.js#n... qunit/tests/io.js:75: test("Test writing entry larger than 1Mb but smaller than 5Mb", (assert) => On 2016/08/30 14:13:03, kzar wrote: > Nit: The functions with one arguments using this syntax don't need the > parentheses. `assert => { ... }` is the same as `(assert) => { ... }`. Same > elsewhere. Done.
https://codereview.adblockplus.org/29350042/diff/29350043/lib/io.js File lib/io.js (left): https://codereview.adblockplus.org/29350042/diff/29350043/lib/io.js#oldcode78 lib/io.js:78: copyFile: function(fromFile, toFile, callback) On 2016/08/31 21:54:35, Oleksandr wrote: > On 2016/08/22 15:38:32, kzar wrote: > > copyFile and renameFile have been removed? > > Yes. But aren't they needed by adblockpluscore/lib/filerStorage.js? https://codereview.adblockplus.org/29350042/diff/29350193/lib/io.js File lib/io.js (right): https://codereview.adblockplus.org/29350042/diff/29350193/lib/io.js#newcode94 lib/io.js:94: entry[key] = {lastModified: Date.now(), On 2016/08/31 21:54:37, Oleksandr wrote: > On 2016/08/30 14:13:02, kzar wrote: > > Nit: This could fit on two lines instead of three. > > Done. However it looked better as it was, IMHO. I can't see the change? https://codereview.adblockplus.org/29350042/diff/29350368/lib/io.js File lib/io.js (right): https://codereview.adblockplus.org/29350042/diff/29350368/lib/io.js#newcode90 lib/io.js:90: // to do so. The solution is to compress and fallback to localStorage Nit: Mind adding a fullstop at the end of this comment?
Patch Set 4 : Add full stop after a comment and fix formatting https://codereview.adblockplus.org/29350042/diff/29350043/lib/io.js File lib/io.js (left): https://codereview.adblockplus.org/29350042/diff/29350043/lib/io.js#oldcode78 lib/io.js:78: copyFile: function(fromFile, toFile, callback) On 2016/09/01 12:57:37, kzar wrote: > On 2016/08/31 21:54:35, Oleksandr wrote: > > On 2016/08/22 15:38:32, kzar wrote: > > > copyFile and renameFile have been removed? > > > > Yes. > > But aren't they needed by adblockpluscore/lib/filerStorage.js? I don't think they are. Looks like we are not doing backups anymore, for which those were used. At least I wasn't able to invoke that code. I have actually removed it because of Sebastian's comment on this code review: https://codereview.adblockplus.org/29339112/diff/29339113/lib/io.js#newcode35
LGTM. Sebastian do you want to review this one too? (You missed replying to a couple of inline comments there at the end. Otherwise the review is looking a lot better now, thanks.) https://codereview.adblockplus.org/29350042/diff/29350043/lib/io.js File lib/io.js (left): https://codereview.adblockplus.org/29350042/diff/29350043/lib/io.js#oldcode78 lib/io.js:78: copyFile: function(fromFile, toFile, callback) On 2016/09/02 04:12:26, Oleksandr wrote: > On 2016/09/01 12:57:37, kzar wrote: > > On 2016/08/31 21:54:35, Oleksandr wrote: > > > On 2016/08/22 15:38:32, kzar wrote: > > > > copyFile and renameFile have been removed? > > > > > > Yes. > > > > But aren't they needed by adblockpluscore/lib/filerStorage.js? > > I don't think they are. Looks like we are not doing backups anymore, for which > those were used. At least I wasn't able to invoke that code. I have actually > removed it because of Sebastian's comment on this code review: > https://codereview.adblockplus.org/29339112/diff/29339113/lib/io.js#newcode35 Ah Ok, fair enough.
https://codereview.adblockplus.org/29350042/diff/29350407/lib/io.js File lib/io.js (right): https://codereview.adblockplus.org/29350042/diff/29350407/lib/io.js#newcode65 lib/io.js:65: if ("compressed" in entry) Please check for entry["compressed"], otherwise, if it is set to false, we'd still try to decompress it. I know that we currently don't set it to false, but still that way the logic is more accurrate. https://codereview.adblockplus.org/29350042/diff/29350407/lib/io.js#newcode91 lib/io.js:91: let processedData = LZString.compressToUTF16(JSON.stringify(data)); (Why) do we need to call JSON.stringifiy() here? Isn't data always a string here?
Patch Set: Make sure entry["compressed"] is truthful https://codereview.adblockplus.org/29350042/diff/29350407/lib/io.js File lib/io.js (right): https://codereview.adblockplus.org/29350042/diff/29350407/lib/io.js#newcode91 lib/io.js:91: let processedData = LZString.compressToUTF16(JSON.stringify(data)); On 2016/09/08 15:27:22, Sebastian Noack wrote: > (Why) do we need to call JSON.stringifiy() here? Isn't data always a string > here? 'data' here is an array of strings, which are lines from the easylist file.
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
