Issue 29339112: Issue 3716 - Split up files stored in storage.local (Closed)
|
Created:
March 30, 2016, 4:50 a.m. by Oleksandr Modified:
Sept. 2, 2016, 3:46 a.m. Reviewers:
Sebastian Noack CC:
kzar Visibility:
Public. |
DescriptionIssue 3716 - Split up files stored in storage.local
Patch Set 1 #
Total comments: 7
Patch Set 2 : Remove setMultiple. Use suggested code. #
Total comments: 7
Patch Set 3 : Modify Safari storage. Refactor chunk name creation. Add comments. #
Total comments: 3
Patch Set 4 : Make sure we don't leak chunks. Edit comments. Simplify safari storage. #
Total comments: 2
Patch Set 5 : Use Promise instead of onChanged listener #
Total comments: 3
Patch Set 6 : Fix the promise logic. Remove whitespace changes. #
Total comments: 2
Patch Set 7 : Remove the trailing comma. #
MessagesTotal messages: 15
https://codereview.adblockplus.org/29339112/diff/29339113/chrome/ext/backgrou... File chrome/ext/background.js (right): https://codereview.adblockplus.org/29339112/diff/29339113/chrome/ext/backgrou... chrome/ext/background.js:551: setMultiple: function(items, callback) Rather than adding yet another method here, I would prefer to simply make ext.storage.set() always take an object. https://codereview.adblockplus.org/29339112/diff/29339113/lib/io.js File lib/io.js (right): https://codereview.adblockplus.org/29339112/diff/29339113/lib/io.js#newcode35 lib/io.js:35: if (typeof entry.content.multipartKeys != "undefined") Please leave loadFile() untouched. There is no reason to load the chunks if statFile() was called for example. Instead, please adapt readFile() like below, and simply remove copyFile(), renameFile() and removeFile() these aren't used anymore. readFromFile: function(file, listener, callback) { function onLoaded(entry) { if ("content" in entry) { for (let line of entry.content) listener.process(line); listener.process(null); callback(null); } else { let keys = []; for (let i = 0; i < entry.chunks; i++) keys.push(fileToKey(file) + ":" + i); ext.storage.get(keys, items => { for (let key of keys) for (let line of items[key]) listener.process(line); listener.process(null); callback(null); }); } } loadFile(file, onLoaded, callback); } https://codereview.adblockplus.org/29339112/diff/29339113/lib/io.js#newcode65 lib/io.js:65: var chunkSize = 104856; They told me that each character is two bytes. Moreover we need to reserve some addional bytes for the keys and overhead of the array. So I guess the quota should rather be like that: 1024 * 1024 / 2 - 1000 = 523288 https://codereview.adblockplus.org/29339112/diff/29339113/lib/io.js#newcode71 lib/io.js:71: storageList[key] = data.slice(dataSize, dataSize + chunkSize); Note that data is an array of strings, where each item represents a line in the filter list. So the logic here has to be a little more complex: let key = fileToKey(file); let items = {}; items[key] = {lastModified: Date.now()}; if (typeof browser == "object") { let quota = 1024 * 1024 / 2 - 1000; let chunks = []; let chunk = []; let chunkSize = 0; for (let line of data) { if (line.length + chunkSize > quota) { chunks.push(chunk); chunk = []; chunkSize = 0; } chunk.push(line); chunkSize += line.length; } chunks.push(chunk); if (chunks.length > 1) { for (let i = 0; i < chunks.length; i++) items[key + ":" + i] = chunks[i]; items[key].chunks = chunks.length; ext.storage.set(items, callback); return; } } items[key].content = data; ext.storage.set(items, callback); https://codereview.adblockplus.org/29339112/diff/29339113/lib/io.js#newcode74 lib/io.js:74: content: { multipartKeys: Object.keys(storageList) }, We can easily reconstruct the keys for the chunks. So instead of a list of keys, we can just store the number of chunks, as in the suggested code above.
https://codereview.adblockplus.org/29339112/diff/29339113/lib/io.js File lib/io.js (right): https://codereview.adblockplus.org/29339112/diff/29339113/lib/io.js#newcode65 lib/io.js:65: var chunkSize = 104856; On 2016/03/30 12:06:37, Sebastian Noack wrote: > They told me that each character is two bytes. Moreover we need to reserve some > addional bytes for the keys and overhead of the array. So I guess the quota > should rather be like that: > > 1024 * 1024 / 2 - 1000 = 523288 Yes, strings are UTF-16 in Edge, unlike in Chrome where they are UTF-8, it seems. I was trying to be more generic here and foresee storing Arrays that are not strings. I don't think there's a need for that now, you're right. https://codereview.adblockplus.org/29339112/diff/29339113/lib/io.js#newcode71 lib/io.js:71: storageList[key] = data.slice(dataSize, dataSize + chunkSize); On 2016/03/30 12:06:38, Sebastian Noack wrote: > Note that data is an array of strings, where each item represents a line in the > filter list. So the logic here has to be a little more complex: > > let key = fileToKey(file); > let items = {}; > items[key] = {lastModified: Date.now()}; > > if (typeof browser == "object") > { > let quota = 1024 * 1024 / 2 - 1000; > let chunks = []; > let chunk = []; > let chunkSize = 0; > > for (let line of data) > { > if (line.length + chunkSize > quota) > { > chunks.push(chunk); > chunk = []; > chunkSize = 0; > } > > chunk.push(line); > chunkSize += line.length; > } > chunks.push(chunk); > > if (chunks.length > 1) > { > for (let i = 0; i < chunks.length; i++) > items[key + ":" + i] = chunks[i]; > items[key].chunks = chunks.length; > ext.storage.set(items, callback); > return; > } > } > > items[key].content = data; > ext.storage.set(items, callback); Yep. For some reason I did not notice the data comes in lines, thanks!
https://codereview.adblockplus.org/29339112/diff/29339188/chrome/ext/backgrou... File chrome/ext/background.js (right): https://codereview.adblockplus.org/29339112/diff/29339188/chrome/ext/backgrou... chrome/ext/background.js:545: set: function(items, callback) You have to adapt safari/ext/background.js as well. https://codereview.adblockplus.org/29339112/diff/29339188/lib/io.js File lib/io.js (left): https://codereview.adblockplus.org/29339112/diff/29339188/lib/io.js#oldcode37 lib/io.js:37: }); While changing most of the code in here, this is a good opportunity to enable strict mode for this file. https://codereview.adblockplus.org/29339112/diff/29339188/lib/io.js#oldcode40 lib/io.js:40: function saveFile(file, data, callback) Since this function is only used by writeToFile() now, mind moving the code there, and getting rid of saveFile()? https://codereview.adblockplus.org/29339112/diff/29339188/lib/io.js File lib/io.js (right): https://codereview.adblockplus.org/29339112/diff/29339188/lib/io.js#newcode47 lib/io.js:47: if (typeof browser == "object") A comment that this accounts for Edge and how the quota was determined would make sense here. https://codereview.adblockplus.org/29339112/diff/29339188/lib/io.js#newcode105 lib/io.js:105: keys.push(fileToKey(file) + ":" + i); How about moving the logic to create those keys into fileToKey()? function fileToKey(file, chunk) { let key = keyPrefix; if (file instanceof FakeFile) key += file.path; else key += file.spec; if (typeof chunk != "undefined") key += ":" + chunk; return key; } https://codereview.adblockplus.org/29339112/diff/29339188/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29339112/diff/29339188/lib/prefs.js#newcod... lib/prefs.js:217: let storageItems = {}; Nit: Just call this variable items for consistence with the other code changed here and with the Chrome API documentation.
https://codereview.adblockplus.org/29339112/diff/29339188/lib/io.js File lib/io.js (right): https://codereview.adblockplus.org/29339112/diff/29339188/lib/io.js#newcode47 lib/io.js:47: if (typeof browser == "object") I just noticed that we potentially leak chunks when overriding the file with a smaller one. So we should remove previous chunks that doesn't get overridden: let items = {}; let entry = items[fileToKey(file)] = {lastModified: Date.now()}; if (typeof browser == "object") { loadFile(file).catch(() => null) .then(oldEntry => { let quota = 1024 * 1024 / 2 - 1000; let chunks = []; let chunk = []; let chunkSize = 0; for (let line of data) { if (line.length + chunkSize > quota) { chunks.push(chunk); chunk = []; chunkSize = 0; } chunk.push(line); chunkSize += line.length; } chunks.push(chunk); for (let i = 0; i < chunks.length; i++) items[fileToKey(file, i)] = chunks[i]; entry.chunks = chunks.length; ext.storage.set(items, callback); if (oldEntry && "chunks" in oldEntry) for (let i = entry.chunks; i < oldEntry.chunks; i++) ext.storage.remove(fileToKey(file, i)); }); } else { entry.content = data; ext.storage.set(items, callback); } Note that in order to simplify the code above, I implied to make loadFile() return a Promise: function loadFile(file) { return new Promise((resolve, reject) => { let key = fileToKey(file); ext.storage.get([key], items => { let entry = items[key]; if (entry) resolve(entry); else reject(new Error("File doesn't exist")); }); }); } https://codereview.adblockplus.org/29339112/diff/29339206/lib/io.js File lib/io.js (right): https://codereview.adblockplus.org/29339112/diff/29339206/lib/io.js#newcode92 lib/io.js:92: // Write to file, splitting into chunks, if required. Nit: This comment is kinda redundant with the comment below. https://codereview.adblockplus.org/29339112/diff/29339206/lib/io.js#newcode104 lib/io.js:104: // In Edge strings are stored in UTF-16 format, so 2 bytes per char How about mentioning that we reserve another 1000 bytes for keys and the array overhead? https://codereview.adblockplus.org/29339112/diff/29339206/safari/ext/backgrou... File safari/ext/background.js (right): https://codereview.adblockplus.org/29339112/diff/29339206/safari/ext/backgrou... safari/ext/background.js:712: if (items.hasOwnProperty(key)) I think it's safe to just skip this check. We never call ext.storage.set with anything other than a plain object without custom prototype.
I think maybe promises is over complicating things a little here. How about the approach in the changeset 4? I'm using storage.onChanged for that.
On 2016/03/31 18:32:55, Oleksandr wrote: > I think maybe promises is over complicating things a little here. How about the > approach in the changeset 4? I'm using storage.onChanged for that. I don't really see how using a promise makes anything more complicated here. If we would have already used promises back then when I wrote that code I would have have made loadFile() return a promise from the beginning. However, using an onChanged listener makes things rather complicated as you spread logic and move it out of the regular code flow. Moreover, ext.storage.onChanged is not only called for files.
https://codereview.adblockplus.org/29339112/diff/29339243/safari/ext/backgrou... File safari/ext/background.js (left): https://codereview.adblockplus.org/29339112/diff/29339243/safari/ext/backgrou... safari/ext/background.js:713: setTimeout(callback, 0); Nit: While changing the code here anyway, note that the second argument of setTimeout() defaults to 0 and therefore can be removed. https://codereview.adblockplus.org/29339112/diff/29339243/safari/ext/backgrou... File safari/ext/background.js (right): https://codereview.adblockplus.org/29339112/diff/29339243/safari/ext/backgrou... safari/ext/background.js:711: { Nit: the braces here are unnecessary.
Decoupling cleanup from the write logic is actually a good idea here, IMO. onChanged event would have been as sort of a destructor, making sure we cleanup the storage even if the file gets modified in any other way other then 'writeFile'. I don't feel too strong about this, however, so patchset 5 uses Promises to cleanup.
https://codereview.adblockplus.org/29339112/diff/29339257/lib/io.js File lib/io.js (right): https://codereview.adblockplus.org/29339112/diff/29339257/lib/io.js#newcode62 lib/io.js:62: loadFile(file).catch(() => null) The logic here is wrong. In the case of readFromFile() and statFile() we have to simply call the callback with the error, if the promise is rejected: loadFile(file).then(onLoaded, callback); https://codereview.adblockplus.org/29339112/diff/29339257/lib/io.js#newcode93 lib/io.js:93: Nit: You added trailing whitespaces to this line. https://codereview.adblockplus.org/29339112/diff/29339257/lib/io.js#newcode136 lib/io.js:136: ext.storage.set(items, callback); Nit: You added trailing whitespaces to this line.
Fingers crossed now! https://codereview.adblockplus.org/29339112/diff/29339267/lib/io.js File lib/io.js (right): https://codereview.adblockplus.org/29339112/diff/29339267/lib/io.js#newcode98 lib/io.js:98: if (typeof browser == "object") I was using this to test in Chrome. Just noticed that it wasn't reverted before upload.
https://codereview.adblockplus.org/29339112/diff/29339267/lib/io.js File lib/io.js (right): https://codereview.adblockplus.org/29339112/diff/29339267/lib/io.js#newcode150 lib/io.js:150: }, Nit: You added a redundant comma here. There should be no comma after the last property in object literals.
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
