Issue 29433625: Issue 5220 - Update the IO API (Closed)
|
Created:
May 8, 2017, 2:14 p.m. by hub Modified:
May 23, 2017, 1:15 p.m. Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
DescriptionIssue 5220 - Update the IO API
Patch Set 1 #
Total comments: 20
Patch Set 2 : Fix the IO implementation based on feedback. #
Total comments: 10
Patch Set 3 : Fixes to the promise flow and review comments. #MessagesTotal messages: 9
I added Sebastian to CC - it's his module, he should be aware of this review. https://codereview.adblockplus.org/29433625/diff/29433626/lib/io.js File lib/io.js (right): https://codereview.adblockplus.org/29433625/diff/29433626/lib/io.js#newcode26 lib/io.js:26: return keyPrefix + (file instanceof FakeFile ? file.path : file.spec); Does this even work? The parameters passed to the methods here are no longer File objects, they are plain string paths. So this should simply return `keyPrefix + path`. I don't really remember what file.spec is about - what was passing URLs to this API? Maybe Sebastian remembers. I strongly suspect that this code is long gone. The only usage of I/O APIs I can see in adblockpluschrome is FilterStorage. https://codereview.adblockplus.org/29433625/diff/29433626/lib/io.js#newcode36 lib/io.js:36: { This should really be checking chrome.runtime.lastError, not really trivial given the current ext API however... Not with this commit I guess. https://codereview.adblockplus.org/29433625/diff/29433626/lib/io.js#newcode49 lib/io.js:49: return new Promise((resolve) => I don't think that it is a good idea to omit the reject parameter merely because you aren't currently using it. But that's my personal opinion, feel free to wait for Sebastian's if you disagree. https://codereview.adblockplus.org/29433625/diff/29433626/lib/io.js#newcode64 lib/io.js:64: resolveFilePath(path) { return new FakeFile(path); }, This method should be removed, along with the FakeFile class. https://codereview.adblockplus.org/29433625/diff/29433626/lib/io.js#newcode66 lib/io.js:66: readFromFile(file, parser) I suggest that you copy JSDoc comments from https://hg.adblockplus.org/adblockplus/file/09f13537a2b0/lib/io.js. It might also be a good idea to keep parameter names consistent - I'm not sure whether the function parameter here is better described as a parser than as a read listener. https://codereview.adblockplus.org/29433625/diff/29433626/lib/io.js#newcode73 lib/io.js:73: parser(null); Please remove this call, it is no longer necessary to call the listener with null in the end - resolving the promise is sufficient. https://codereview.adblockplus.org/29433625/diff/29433626/lib/io.js#newcode90 lib/io.js:90: }); You don't need to create the promise explicitly. This code is equivalent to: return loadFile(fromFile).then(entry => { return saveFile(toFile, entry.content); }); See also https://hg.adblockplus.org/adblockplus/file/09f13537a2b0/webextension/io.js#l72 - this implementation should be identical to the one in adblockpluschrome in most respects. https://codereview.adblockplus.org/29433625/diff/29433626/lib/io.js#newcode104 lib/io.js:104: }); It's better to create the promises where they are actually required: return loadFile(fromFile).then(entry => { return new Promise((resolve, reject) => { ext.storage.set(fileToKey(newName), entry, resolve); }); }).then(() => { return new Promise((resolve, reject) => { ext.storage.remove(fileToKey(fromFile), resolve); }); }); Two aspects to note: since all parameters are paths now, fileToKey() should be used on newName as well rather than using keyPrefix here explicitly. Also, it is a good idea to remove the existing entry only after the new one was created - I know that the code currently doesn't check for errors, but that's a limitation of the current ext.storage implementation. In https://hg.adblockplus.org/adblockplus/file/09f13537a2b0/webextension/io.js#l52 I actually created a removeFile() helper function because the same code is reused below. https://codereview.adblockplus.org/29433625/diff/29433626/lib/io.js#newcode117 lib/io.js:117: return loadFile(file).then((entry) => Nit: you are somewhat inconsistent, are you putting single arrow function parameters in parentheses or not? Our coding style doesn't enforce it either way but maybe you should decide on that for yourself. https://codereview.adblockplus.org/29433625/diff/29433626/lib/io.js#newcode123 lib/io.js:123: }); This promise will currently be rejected if the entry doesn't exist, it should rather return a result (the version in ABP for Firefox doesn't do that because the error is processed further up). E.g. something along the lines of: return loadFile(file).then(entry => { ... }).catch(error => { if (error.type == "NoSuchFile") return {exists: false}; throw error; }); And making sure to set the type property on the error thrown in loadFile().
Update patch to fix these. https://codereview.adblockplus.org/29433625/diff/29433626/lib/io.js File lib/io.js (right): https://codereview.adblockplus.org/29433625/diff/29433626/lib/io.js#newcode26 lib/io.js:26: return keyPrefix + (file instanceof FakeFile ? file.path : file.spec); On 2017/05/09 09:12:27, Wladimir Palant wrote: > Does this even work? The parameters passed to the methods here are no longer > File objects, they are plain string paths. So this should simply return > `keyPrefix + path`. > > I don't really remember what file.spec is about - what was passing URLs to this > API? Maybe Sebastian remembers. I strongly suspect that this code is long gone. > The only usage of I/O APIs I can see in adblockpluschrome is FilterStorage. Looking at the code it seems that "file" is a fileName (string). I'll remove fakeFile anyway and change this to match. https://codereview.adblockplus.org/29433625/diff/29433626/lib/io.js#newcode49 lib/io.js:49: return new Promise((resolve) => On 2017/05/09 09:12:27, Wladimir Palant wrote: > I don't think that it is a good idea to omit the reject parameter merely because > you aren't currently using it. But that's my personal opinion, feel free to wait > for Sebastian's if you disagree. I'll put it back wherever. I it probably a better idea to leave it. https://codereview.adblockplus.org/29433625/diff/29433626/lib/io.js#newcode64 lib/io.js:64: resolveFilePath(path) { return new FakeFile(path); }, On 2017/05/09 09:12:27, Wladimir Palant wrote: > This method should be removed, along with the FakeFile class. Done. https://codereview.adblockplus.org/29433625/diff/29433626/lib/io.js#newcode66 lib/io.js:66: readFromFile(file, parser) On 2017/05/09 09:12:27, Wladimir Palant wrote: > I suggest that you copy JSDoc comments from > https://hg.adblockplus.org/adblockplus/file/09f13537a2b0/lib/io.js. It might > also be a good idea to keep parameter names consistent - I'm not sure whether > the function parameter here is better described as a parser than as a read > listener. I'll change the parameters to match. And this resolve the question for `fileToKey` above. https://codereview.adblockplus.org/29433625/diff/29433626/lib/io.js#newcode73 lib/io.js:73: parser(null); On 2017/05/09 09:12:27, Wladimir Palant wrote: > Please remove this call, it is no longer necessary to call the listener with > null in the end - resolving the promise is sufficient. Done. https://codereview.adblockplus.org/29433625/diff/29433626/lib/io.js#newcode90 lib/io.js:90: }); On 2017/05/09 09:12:27, Wladimir Palant wrote: > You don't need to create the promise explicitly. This code is equivalent to: > > return loadFile(fromFile).then(entry => > { > return saveFile(toFile, entry.content); > }); > > See also > https://hg.adblockplus.org/adblockplus/file/09f13537a2b0/webextension/io.js#l72 > - this implementation should be identical to the one in adblockpluschrome in > most respects. Acknowledged. https://codereview.adblockplus.org/29433625/diff/29433626/lib/io.js#newcode104 lib/io.js:104: }); On 2017/05/09 09:12:27, Wladimir Palant wrote: > It's better to create the promises where they are actually required: > > return loadFile(fromFile).then(entry => > { > return new Promise((resolve, reject) => > { > ext.storage.set(fileToKey(newName), entry, resolve); > }); > }).then(() => > { > return new Promise((resolve, reject) => > { > ext.storage.remove(fileToKey(fromFile), resolve); > }); > }); > > Two aspects to note: since all parameters are paths now, fileToKey() should be > used on newName as well rather than using keyPrefix here explicitly. Also, it is > a good idea to remove the existing entry only after the new one was created - I > know that the code currently doesn't check for errors, but that's a limitation > of the current ext.storage implementation. > > In > https://hg.adblockplus.org/adblockplus/file/09f13537a2b0/webextension/io.js#l52 > I actually created a removeFile() helper function because the same code is > reused below. Make sense. https://codereview.adblockplus.org/29433625/diff/29433626/lib/io.js#newcode117 lib/io.js:117: return loadFile(file).then((entry) => On 2017/05/09 09:12:27, Wladimir Palant wrote: > Nit: you are somewhat inconsistent, are you putting single arrow function > parameters in parentheses or not? Our coding style doesn't enforce it either way > but maybe you should decide on that for yourself. Acknowledged. https://codereview.adblockplus.org/29433625/diff/29433626/lib/io.js#newcode123 lib/io.js:123: }); On 2017/05/09 09:12:27, Wladimir Palant wrote: > This promise will currently be rejected if the entry doesn't exist, it should > rather return a result (the version in ABP for Firefox doesn't do that because > the error is processed further up). E.g. something along the lines of: > > return loadFile(file).then(entry => > { > ... > }).catch(error => > { > if (error.type == "NoSuchFile") > return {exists: false}; > throw error; > }); > > And making sure to set the type property on the error thrown in loadFile(). Done.
https://codereview.adblockplus.org/29433625/diff/29433626/lib/io.js File lib/io.js (right): https://codereview.adblockplus.org/29433625/diff/29433626/lib/io.js#newcode49 lib/io.js:49: return new Promise((resolve) => On 2017/05/10 16:08:51, hub wrote: > On 2017/05/09 09:12:27, Wladimir Palant wrote: > > I don't think that it is a good idea to omit the reject parameter merely > because > > you aren't currently using it. But that's my personal opinion, feel free to > wait > > for Sebastian's if you disagree. > > I'll put it back wherever. I it probably a better idea to leave it. Personally, I think I would have omitted it too, but I don't mind either way. https://codereview.adblockplus.org/29433625/diff/29435642/lib/io.js File lib/io.js (right): https://codereview.adblockplus.org/29433625/diff/29435642/lib/io.js#newcode73 lib/io.js:73: * Name of the file to be read Nit: This is inconsistent with how we wrap documentation everywhere else: * @param {type} argname beginning of the description goes here subsequent lines are aligned with the above https://codereview.adblockplus.org/29433625/diff/29435642/lib/io.js#newcode84 lib/io.js:84: listener(line); Nit: Please use 2 (not 4) space indentation. However, is it even necessary to wrap this line? https://codereview.adblockplus.org/29433625/diff/29435642/lib/io.js#newcode135 lib/io.js:135: { The parentheses + return is redundant here.
FakeFile implementation hasn't been removed with the current patch. https://codereview.adblockplus.org/29433625/diff/29435642/lib/io.js File lib/io.js (right): https://codereview.adblockplus.org/29433625/diff/29435642/lib/io.js#newcode73 lib/io.js:73: * Name of the file to be read On 2017/05/22 13:36:30, Sebastian Noack wrote: > Nit: This is inconsistent with how we wrap documentation everywhere else: > > * @param {type} argname beginning of the description goes here > subsequent lines are aligned with the above Yes, I am aware of that - and wrapping it this way is really awkward once you get lengthy type names, there simply isn't much space left for the text. https://codereview.adblockplus.org/29433625/diff/29435642/lib/io.js#newcode84 lib/io.js:84: listener(line); On 2017/05/22 13:36:30, Sebastian Noack wrote: > Nit: Please use 2 (not 4) space indentation. However, is it even necessary to > wrap this line? This comment doesn't appear to match the line it has been added to...
https://codereview.adblockplus.org/29433625/diff/29435642/lib/io.js File lib/io.js (right): https://codereview.adblockplus.org/29433625/diff/29435642/lib/io.js#newcode73 lib/io.js:73: * Name of the file to be read On 2017/05/22 13:46:31, Wladimir Palant wrote: > On 2017/05/22 13:36:30, Sebastian Noack wrote: > > Nit: This is inconsistent with how we wrap documentation everywhere else: > > > > * @param {type} argname beginning of the description goes here > > subsequent lines are aligned with the above > > Yes, I am aware of that - and wrapping it this way is really awkward once you > get lengthy type names, there simply isn't much space left for the text. Adding Dave for a third opinion. https://codereview.adblockplus.org/29433625/diff/29435642/lib/io.js#newcode84 lib/io.js:84: listener(line); On 2017/05/22 13:46:31, Wladimir Palant wrote: > On 2017/05/22 13:36:30, Sebastian Noack wrote: > > Nit: Please use 2 (not 4) space indentation. However, is it even necessary to > > wrap this line? > > This comment doesn't appear to match the line it has been added to... Sorry, I misread the code. For some reason I didn't notice the line above. Please ignore this comment.
https://codereview.adblockplus.org/29433625/diff/29435642/lib/io.js File lib/io.js (right): https://codereview.adblockplus.org/29433625/diff/29435642/lib/io.js#newcode73 lib/io.js:73: * Name of the file to be read On 2017/05/22 13:49:52, Sebastian Noack wrote: > On 2017/05/22 13:46:31, Wladimir Palant wrote: > > On 2017/05/22 13:36:30, Sebastian Noack wrote: > > > Nit: This is inconsistent with how we wrap documentation everywhere else: > > > > > > * @param {type} argname beginning of the description goes here > > > subsequent lines are aligned with the above > > > > Yes, I am aware of that - and wrapping it this way is really awkward once you > > get lengthy type names, there simply isn't much space left for the text. > > Adding Dave for a third opinion. Well IIRC I've done it both ways depending on the repository and what the reviewer asked me to do. I don't care too much either way to be honest, I find both pretty clear.
Updated patch, addressed comments, fixed some Promise code that was wrong. https://codereview.adblockplus.org/29433625/diff/29435642/lib/io.js File lib/io.js (right): https://codereview.adblockplus.org/29433625/diff/29435642/lib/io.js#newcode73 lib/io.js:73: * Name of the file to be read I copied (and proofed) these documentation comment from adblockplus' lib/io.js. Including the formatting. https://codereview.adblockplus.org/29433625/diff/29435642/lib/io.js#newcode135 lib/io.js:135: { On 2017/05/22 13:36:30, Sebastian Noack wrote: > The parentheses + return is redundant here. Done.
LGTM 
|
