Issue 29355962: Issue 4023 - Use localforage (IndexedDB) instead of localStorage (Closed)
|
Created:
Oct. 5, 2016, 9:40 a.m. by Oleksandr Modified:
Oct. 11, 2016, 7:45 p.m. Visibility:
Public. |
DescriptionIssue 4023 - Use localforage (IndexedDB) instead of localStorage
Patch Set 1 #
Total comments: 13
Patch Set 2 : Patch set 1 - Address the comments #
Total comments: 6
Patch Set 3 : Simplify the code. Fix the "undefined" bug. #
Total comments: 2
Patch Set 4 : Remove redundant try-catch #Patch Set 5 : Get back to async loadFile. Don't return a promise. Check localStorage. #
Total comments: 5
Patch Set 6 : Make sure we always return asynchronously from loadFile. Add try-catch block. #
Total comments: 4
Patch Set 7 : Remove second parameter in setTimeout #Patch Set 8 : Don't catch exceptions from successCallback #
Total comments: 7
Patch Set 9 : Don't call both successCallback and errorCallback #
MessagesTotal messages: 29
Using IndexedDB directly turned out to complicate code considerably. I have ended up just using Mozilla's localforage project (which we already use for Adblock Browser on Android).
On 2016/10/05 09:42:58, Oleksandr wrote: > Using IndexedDB directly turned out to complicate code considerably. I have > ended up just using Mozilla's localforage project (which we already use for > Adblock Browser on Android). Would you mind updating the issue? I had a look at that expecting to see some more explanation about why localforage was a good idea but didn't see any. Also the issue is already marked as fixed?
On 2016/10/05 11:39:36, kzar wrote: > On 2016/10/05 09:42:58, Oleksandr wrote: > > Using IndexedDB directly turned out to complicate code considerably. I have > > ended up just using Mozilla's localforage project (which we already use for > > Adblock Browser on Android). > > Would you mind updating the issue? I had a look at that expecting to see some > more explanation about why localforage was a good idea but didn't see any. Also > the issue is already marked as fixed? Updated the issue.
Thanks for updating the issue. Some comments: https://codereview.adblockplus.org/29355962/diff/29355963/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29355962/diff/29355963/lib/adblockplus.js#... lib/adblockplus.js:376: entry = localforage.getItem(key, function(err, value) Since apparently `localforage.getItem` returns a promise if called without the callback parameter couldn't we simplify this code? We could return the Promise it gave us (after checking the `err` and `value`, resolving with only `value`) instead of creating a new one from scratch. https://codereview.adblockplus.org/29355962/diff/29355963/lib/adblockplus.js#... lib/adblockplus.js:379: { Nit: No need for braces around if... else... clauses here. https://codereview.adblockplus.org/29355962/diff/29355963/lib/adblockplus.js#... lib/adblockplus.js:398: entry[key] = { Since this review is for changes to adblockplusedgems can't we remove `entry` and simplify some of this logic? https://codereview.adblockplus.org/29355962/diff/29355963/lib/adblockplus.js#... lib/adblockplus.js:406: callback(); Could you explain this change? How come ext.storage.set isn't calling the callback? Also it looks kind of weird here since couldn't the callback be called both here and by ext.storage.set? https://codereview.adblockplus.org/29355962/diff/29355963/lib/adblockplus.js#... lib/adblockplus.js:410: // Use IndexedDB to store data This comment seems kind of pointless.
As far as this is just an experimental hack in adblockplusedgems, fine with me using localforage for now. But if this workaround turns out to be efficient, we should also merge it upstream. But I'd rather not have that localforage thing in there. Yes, using IndexedDB directly requires some boilerplate, but since we only use it in one place, I consider that a better approach than using an addional abstraction layer. After all, the io module is already an abstraction layer.
Patch set 1 - Address the comments. https://codereview.adblockplus.org/29355962/diff/29355963/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29355962/diff/29355963/lib/adblockplus.js#... lib/adblockplus.js:376: entry = localforage.getItem(key, function(err, value) On 2016/10/05 12:14:17, kzar wrote: > Since apparently `localforage.getItem` returns a promise if called without the > callback parameter couldn't we simplify this code? We could return the Promise > it gave us (after checking the `err` and `value`, resolving with only `value`) > instead of creating a new one from scratch. I am not 100% sure I follow your suggestion, but we still need to check the ext.storage first to be able to migrate the current users to IndexedDB. So we still need to create a promise from scratch at least for that. https://codereview.adblockplus.org/29355962/diff/29355963/lib/adblockplus.js#... lib/adblockplus.js:406: callback(); On 2016/10/05 12:14:17, kzar wrote: > Could you explain this change? How come ext.storage.set isn't calling the > callback? Also it looks kind of weird here since couldn't the callback be called > both here and by ext.storage.set? Yeah, that was a bug.
(That's a kind of weird name for a patch set "Patch Set 2 : Patch set 1 - Address the comments".) https://codereview.adblockplus.org/29355962/diff/29355963/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29355962/diff/29355963/lib/adblockplus.js#... lib/adblockplus.js:376: entry = localforage.getItem(key, function(err, value) On 2016/10/05 13:32:40, Oleksandr wrote: > On 2016/10/05 12:14:17, kzar wrote: > > Since apparently `localforage.getItem` returns a promise if called without the > > callback parameter couldn't we simplify this code? We could return the Promise > > it gave us (after checking the `err` and `value`, resolving with only `value`) > > instead of creating a new one from scratch. > > I am not 100% sure I follow your suggestion, but we still need to check the > ext.storage first to be able to migrate the current users to IndexedDB. So we > still need to create a promise from scratch at least for that. I mean for you do to something like this (untested): function loadFile(file) { var key = fileToKey(file); return localforage.getItem(key).then(value => value || Promise.reject(new Error("File doesn't exist"))); } https://codereview.adblockplus.org/29355962/diff/29356014/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29355962/diff/29356014/lib/adblockplus.js#... lib/adblockplus.js:398: localforage.setItem(key, entry, callback); Did you test this? Isn't `key` undefined now? https://codereview.adblockplus.org/29355962/diff/29356014/lib/adblockplus.js#... lib/adblockplus.js:398: localforage.setItem(key, entry, callback); Why not just pass in `{lastModified: Date.now(), content: data}` here directly if you're not using the `entry` variable elsewhere any more?
Patch Set 3 : Simplify the code. Fix the "undefined" bug. https://codereview.adblockplus.org/29355962/diff/29355963/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29355962/diff/29355963/lib/adblockplus.js#... lib/adblockplus.js:376: entry = localforage.getItem(key, function(err, value) On 2016/10/05 13:46:53, kzar wrote: > On 2016/10/05 13:32:40, Oleksandr wrote: > > On 2016/10/05 12:14:17, kzar wrote: > > > Since apparently `localforage.getItem` returns a promise if called without > the > > > callback parameter couldn't we simplify this code? We could return the > Promise > > > it gave us (after checking the `err` and `value`, resolving with only > `value`) > > > instead of creating a new one from scratch. > > > > I am not 100% sure I follow your suggestion, but we still need to check the > > ext.storage first to be able to migrate the current users to IndexedDB. So we > > still need to create a promise from scratch at least for that. > > I mean for you do to something like this (untested): > > function loadFile(file) > { > var key = fileToKey(file); > return localforage.getItem(key).then(value => value || Promise.reject(new > Error("File doesn't exist"))); > } That won't work because like I said above we also need to call ext.storage.get first. Isn't that right? https://codereview.adblockplus.org/29355962/diff/29356014/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29355962/diff/29356014/lib/adblockplus.js#... lib/adblockplus.js:398: localforage.setItem(key, entry, callback); On 2016/10/05 13:46:53, kzar wrote: > Why not just pass in `{lastModified: Date.now(), content: data}` here directly > if you're not using the `entry` variable elsewhere any more? I am not sure this is prettier, but I don't have a strong opinion.
https://codereview.adblockplus.org/29355962/diff/29355963/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29355962/diff/29355963/lib/adblockplus.js#... lib/adblockplus.js:376: entry = localforage.getItem(key, function(err, value) On 2016/10/05 21:07:40, Oleksandr wrote: > On 2016/10/05 13:46:53, kzar wrote: > > On 2016/10/05 13:32:40, Oleksandr wrote: > > > On 2016/10/05 12:14:17, kzar wrote: > > > > Since apparently `localforage.getItem` returns a promise if called without > > the > > > > callback parameter couldn't we simplify this code? We could return the > > Promise > > > > it gave us (after checking the `err` and `value`, resolving with only > > `value`) > > > > instead of creating a new one from scratch. > > > > > > I am not 100% sure I follow your suggestion, but we still need to check the > > > ext.storage first to be able to migrate the current users to IndexedDB. So > we > > > still need to create a promise from scratch at least for that. > > > > I mean for you do to something like this (untested): > > > > function loadFile(file) > > { > > var key = fileToKey(file); > > return localforage.getItem(key).then(value => value || Promise.reject(new > > Error("File doesn't exist"))); > > } > > That won't work because like I said above we also need to call ext.storage.get > first. Isn't that right? Why do we have to call ext.storage.get() first? I don't think we have to. https://codereview.adblockplus.org/29355962/diff/29356014/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29355962/diff/29356014/lib/adblockplus.js#... lib/adblockplus.js:398: localforage.setItem(key, entry, callback); On 2016/10/05 21:07:40, Oleksandr wrote: > On 2016/10/05 13:46:53, kzar wrote: > > Why not just pass in `{lastModified: Date.now(), content: data}` here directly > > if you're not using the `entry` variable elsewhere any more? > > I am not sure this is prettier, but I don't have a strong opinion. Also considering that we'd have to wrap this line if we inline the object literal, I think it is fine as it is. https://codereview.adblockplus.org/29355962/diff/29356030/lib/adblockplus.js File lib/adblockplus.js (left): https://codereview.adblockplus.org/29355962/diff/29356030/lib/adblockplus.js#... lib/adblockplus.js:370: try I think this try-catch block should be removed now. It seems to have been there originally to handle if JSON.parse() fails. But if there is an error now it is passed to the callback rather than being thrown synchronously, right?
https://codereview.adblockplus.org/29355962/diff/29356014/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29355962/diff/29356014/lib/adblockplus.js#... lib/adblockplus.js:398: localforage.setItem(key, entry, callback); On 2016/10/05 21:27:31, Sebastian Noack wrote: > On 2016/10/05 21:07:40, Oleksandr wrote: > > On 2016/10/05 13:46:53, kzar wrote: > > > Why not just pass in `{lastModified: Date.now(), content: data}` here > directly > > > if you're not using the `entry` variable elsewhere any more? > > > > I am not sure this is prettier, but I don't have a strong opinion. > > Also considering that we'd have to wrap this line if we inline the object > literal, I think it is fine as it is. ...as it was, I meant.
Patch Set 4 : Remove redundant try-catch. https://codereview.adblockplus.org/29355962/diff/29355963/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29355962/diff/29355963/lib/adblockplus.js#... lib/adblockplus.js:376: entry = localforage.getItem(key, function(err, value) On 2016/10/05 21:27:31, Sebastian Noack wrote: > On 2016/10/05 21:07:40, Oleksandr wrote: > > On 2016/10/05 13:46:53, kzar wrote: > > > On 2016/10/05 13:32:40, Oleksandr wrote: > > > > On 2016/10/05 12:14:17, kzar wrote: > > > > > Since apparently `localforage.getItem` returns a promise if called > without > > > the > > > > > callback parameter couldn't we simplify this code? We could return the > > > Promise > > > > > it gave us (after checking the `err` and `value`, resolving with only > > > `value`) > > > > > instead of creating a new one from scratch. > > > > > > > > I am not 100% sure I follow your suggestion, but we still need to check > the > > > > ext.storage first to be able to migrate the current users to IndexedDB. So > > we > > > > still need to create a promise from scratch at least for that. > > > > > > I mean for you do to something like this (untested): > > > > > > function loadFile(file) > > > { > > > var key = fileToKey(file); > > > return localforage.getItem(key).then(value => value || Promise.reject(new > > > Error("File doesn't exist"))); > > > } > > > > That won't work because like I said above we also need to call ext.storage.get > > first. Isn't that right? > > Why do we have to call ext.storage.get() first? I don't think we have to. Current ABP for Edge users have their subscriptions stored using ext.storage. It wouldn't be great to just reset their settings. We want to gently migrate those to IndexedDB, no? https://codereview.adblockplus.org/29355962/diff/29356014/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29355962/diff/29356014/lib/adblockplus.js#... lib/adblockplus.js:398: localforage.setItem(key, entry, callback); On 2016/10/05 21:28:38, Sebastian Noack wrote: > On 2016/10/05 21:27:31, Sebastian Noack wrote: > > On 2016/10/05 21:07:40, Oleksandr wrote: > > > On 2016/10/05 13:46:53, kzar wrote: > > > > Why not just pass in `{lastModified: Date.now(), content: data}` here > > directly > > > > if you're not using the `entry` variable elsewhere any more? > > > > > > I am not sure this is prettier, but I don't have a strong opinion. > > > > Also considering that we'd have to wrap this line if we inline the object > > literal, I think it is fine as it is. > > ...as it was, I meant. Done. https://codereview.adblockplus.org/29355962/diff/29356030/lib/adblockplus.js File lib/adblockplus.js (left): https://codereview.adblockplus.org/29355962/diff/29356030/lib/adblockplus.js#... lib/adblockplus.js:370: try On 2016/10/05 21:27:32, Sebastian Noack wrote: > I think this try-catch block should be removed now. It seems to have been there > originally to handle if JSON.parse() fails. But if there is an error now it is > passed to the callback rather than being thrown synchronously, right? Done.
https://codereview.adblockplus.org/29355962/diff/29355963/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29355962/diff/29355963/lib/adblockplus.js#... lib/adblockplus.js:376: entry = localforage.getItem(key, function(err, value) On 2016/10/05 21:49:12, Oleksandr wrote: > On 2016/10/05 21:27:31, Sebastian Noack wrote: > > On 2016/10/05 21:07:40, Oleksandr wrote: > > > On 2016/10/05 13:46:53, kzar wrote: > > > > On 2016/10/05 13:32:40, Oleksandr wrote: > > > > > On 2016/10/05 12:14:17, kzar wrote: > > > > > > Since apparently `localforage.getItem` returns a promise if called > > without > > > > the > > > > > > callback parameter couldn't we simplify this code? We could return the > > > > Promise > > > > > > it gave us (after checking the `err` and `value`, resolving with only > > > > `value`) > > > > > > instead of creating a new one from scratch. > > > > > > > > > > I am not 100% sure I follow your suggestion, but we still need to check > > the > > > > > ext.storage first to be able to migrate the current users to IndexedDB. > So > > > we > > > > > still need to create a promise from scratch at least for that. > > > > > > > > I mean for you do to something like this (untested): > > > > > > > > function loadFile(file) > > > > { > > > > var key = fileToKey(file); > > > > return localforage.getItem(key).then(value => value || > Promise.reject(new > > > > Error("File doesn't exist"))); > > > > } > > > > > > That won't work because like I said above we also need to call > ext.storage.get > > > first. Isn't that right? > > > > Why do we have to call ext.storage.get() first? I don't think we have to. > > Current ABP for Edge users have their subscriptions stored using ext.storage. It > wouldn't be great to just reset their settings. We want to gently migrate those > to IndexedDB, no? Do they? Don't we use localStorage here in the version that is out for a while? But you are right, we have to migrate those data from localStorage.
Patch Set 5 : Get back to async loadFile. Don't return a promise. Check localStorage. https://codereview.adblockplus.org/29355962/diff/29355963/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29355962/diff/29355963/lib/adblockplus.js#... lib/adblockplus.js:376: entry = localforage.getItem(key, function(err, value) On 2016/10/05 21:53:30, Sebastian Noack wrote: > On 2016/10/05 21:49:12, Oleksandr wrote: > > On 2016/10/05 21:27:31, Sebastian Noack wrote: > > > On 2016/10/05 21:07:40, Oleksandr wrote: > > > > On 2016/10/05 13:46:53, kzar wrote: > > > > > On 2016/10/05 13:32:40, Oleksandr wrote: > > > > > > On 2016/10/05 12:14:17, kzar wrote: > > > > > > > Since apparently `localforage.getItem` returns a promise if called > > > without > > > > > the > > > > > > > callback parameter couldn't we simplify this code? We could return > the > > > > > Promise > > > > > > > it gave us (after checking the `err` and `value`, resolving with > only > > > > > `value`) > > > > > > > instead of creating a new one from scratch. > > > > > > > > > > > > I am not 100% sure I follow your suggestion, but we still need to > check > > > the > > > > > > ext.storage first to be able to migrate the current users to > IndexedDB. > > So > > > > we > > > > > > still need to create a promise from scratch at least for that. > > > > > > > > > > I mean for you do to something like this (untested): > > > > > > > > > > function loadFile(file) > > > > > { > > > > > var key = fileToKey(file); > > > > > return localforage.getItem(key).then(value => value || > > Promise.reject(new > > > > > Error("File doesn't exist"))); > > > > > } > > > > > > > > That won't work because like I said above we also need to call > > ext.storage.get > > > > first. Isn't that right? > > > > > > Why do we have to call ext.storage.get() first? I don't think we have to. > > > > Current ABP for Edge users have their subscriptions stored using ext.storage. > It > > wouldn't be great to just reset their settings. We want to gently migrate > those > > to IndexedDB, no? > > Do they? Don't we use localStorage here in the version that is out for a while? > > But you are right, we have to migrate those data from localStorage. Ah, you're right. Version 0.9.6, which is the current public build in Windows Store does use localStorage. Switched to checking localStorage first and made loadFile async again.
LGTM
https://codereview.adblockplus.org/29355962/diff/29356042/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29355962/diff/29356042/lib/adblockplus.js#... lib/adblockplus.js:366: successCallback(JSON.parse(entry)); Don't we have to respond asynchronously here (i.e. wrap it by setTimeout)? https://codereview.adblockplus.org/29355962/diff/29356042/lib/adblockplus.js#... lib/adblockplus.js:370: entry = localforage.getItem(key, function(err, value) It seems unnecessary to assign to "entry" here. It seems unused.
Patch Set 6 : Make sure we always return asynchronously from loadFile. Add try-catch block. https://codereview.adblockplus.org/29355962/diff/29356042/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29355962/diff/29356042/lib/adblockplus.js#... lib/adblockplus.js:366: successCallback(JSON.parse(entry)); On 2016/10/06 12:07:36, Sebastian Noack wrote: > Don't we have to respond asynchronously here (i.e. wrap it by setTimeout)? Indeed, we should. Great catch! For reference, this is discussed here: https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js#.... I wasn't able to test the full upgrade, but a synthetic test did show that this has to return asynchronously. https://codereview.adblockplus.org/29355962/diff/29356042/lib/adblockplus.js#... lib/adblockplus.js:370: entry = localforage.getItem(key, function(err, value) On 2016/10/06 12:07:36, Sebastian Noack wrote: > It seems unnecessary to assign to "entry" here. It seems unused. Done.
https://codereview.adblockplus.org/29355962/diff/29356042/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29355962/diff/29356042/lib/adblockplus.js#... lib/adblockplus.js:366: successCallback(JSON.parse(entry)); On 2016/10/06 23:28:13, Oleksandr wrote: > On 2016/10/06 12:07:36, Sebastian Noack wrote: > > Don't we have to respond asynchronously here (i.e. wrap it by setTimeout)? > > Indeed, we should. Great catch! For reference, this is discussed here: > https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js#.... > I wasn't able to test the full upgrade, but a synthetic test did show that this > has to return asynchronously. Yes, that is what I remember, and why I brought it up here. https://codereview.adblockplus.org/29355962/diff/29356274/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29355962/diff/29356274/lib/adblockplus.js#... lib/adblockplus.js:368: setTimeout(successCallback(JSON.parse(entry)), 0); Not too important but you can call setTimout() without secomd parameter. That is equivalent to passing 0. Not sure if it is worth catching that theoretical error in the first place.
Patch Set 7 : Remove second parameter in setTimeout https://codereview.adblockplus.org/29355962/diff/29356274/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29355962/diff/29356274/lib/adblockplus.js#... lib/adblockplus.js:368: setTimeout(successCallback(JSON.parse(entry)), 0); On 2016/10/06 23:38:35, Sebastian Noack wrote: > Not too important but you can call setTimout() without secomd parameter. That is > equivalent to passing 0. Not sure if it is worth catching that theoretical error > in the first place. Done. But I think it wouldn't hurt catching an error here. I stumbled across this when I tried my synthetic test of upgrade. Who knows what can be found in localStorage, really... Users can edit it manually too, in theory.
https://codereview.adblockplus.org/29355962/diff/29356274/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29355962/diff/29356274/lib/adblockplus.js#... lib/adblockplus.js:368: setTimeout(successCallback(JSON.parse(entry)), 0); On 2016/10/07 16:29:17, Oleksandr wrote: > On 2016/10/06 23:38:35, Sebastian Noack wrote: > > Not too important but you can call setTimout() without secomd parameter. That > is > > equivalent to passing 0. Not sure if it is worth catching that theoretical > error > > in the first place. > > Done. But I think it wouldn't hurt catching an error here. I stumbled across > this when I tried my synthetic test of upgrade. Who knows what can be found in > localStorage, really... Users can edit it manually too, in theory. Wouldn't this also catch an exception thrown by the successCallback function, resulting in both successCallback and errorCallback to be called?
https://codereview.adblockplus.org/29355962/diff/29356274/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29355962/diff/29356274/lib/adblockplus.js#... lib/adblockplus.js:368: setTimeout(successCallback(JSON.parse(entry)), 0); On 2016/10/07 17:00:00, kzar wrote: > On 2016/10/07 16:29:17, Oleksandr wrote: > > On 2016/10/06 23:38:35, Sebastian Noack wrote: > > > Not too important but you can call setTimout() without secomd parameter. > That > > is > > > equivalent to passing 0. Not sure if it is worth catching that theoretical > > error > > > in the first place. > > > > Done. But I think it wouldn't hurt catching an error here. I stumbled across > > this when I tried my synthetic test of upgrade. Who knows what can be found in > > localStorage, really... Users can edit it manually too, in theory. > > Wouldn't this also catch an exception thrown by the successCallback function, > resulting in both successCallback and errorCallback to be called? Yes, but I think that's expected, no?
LGTM
https://codereview.adblockplus.org/29355962/diff/29356368/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29355962/diff/29356368/lib/adblockplus.js#... lib/adblockplus.js:374: setTimeout(successCallback(entry)); So if there is an entry in localStorage but it isn't valid JSON, we first call the error callback and then the success callback? This seems to be wrong. https://codereview.adblockplus.org/29355962/diff/29356368/lib/adblockplus.js#... lib/adblockplus.js:380: if (err || !value) The check for !value seems to be wrong. So if an entry stores an empty string, a use case that is generally supported, you treat this as an error which I think we should not.
https://codereview.adblockplus.org/29355962/diff/29356368/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29355962/diff/29356368/lib/adblockplus.js#... lib/adblockplus.js:374: setTimeout(successCallback(entry)); On 2016/10/10 14:58:06, Sebastian Noack wrote: > So if there is an entry in localStorage but it isn't valid JSON, we first call > the error callback and then the success callback? This seems to be wrong. Whoops you're right, sorry I should have spotted that too. Ollie I think you need to return after the error callback from inside the `catch(err)` block. (As a bonus if you return after the success callback too you could avoid needing the else {...} block for the localforage stuff.) Also perhaps we should check for `typeof == "string"` here explicitly? How about something like this? (untested) // First check localStorage in case the file hasn't been migrated to IndexDB yet var entry = localStorage.getItem(key); if (typeof entry == "string") { try { entry = JSON.parse(entry); } catch(error) { setTimeout(errorCallback(new Error("File is corrupted"))); return; } setTimeout(successCallback(entry)); return; } // Finally check IndexDB localforage.getItem(key).then(successCallback, errorCallback);
Patch Set 9 : Don't call both successCallback and errorCallback https://codereview.adblockplus.org/29355962/diff/29356368/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29355962/diff/29356368/lib/adblockplus.js#... lib/adblockplus.js:380: if (err || !value) On 2016/10/10 14:58:06, Sebastian Noack wrote: > The check for !value seems to be wrong. So if an entry stores an empty string, a > use case that is generally supported, you treat this as an error which I think > we should not. I don't think this is generally supported. This is not a string, but object with `lastModified` and `content` properties. `content` may very well be an empty string, but if a value here is falsey this is very much an error.
https://codereview.adblockplus.org/29355962/diff/29356368/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29355962/diff/29356368/lib/adblockplus.js#... lib/adblockplus.js:374: setTimeout(successCallback(entry)); On 2016/10/11 07:22:41, kzar wrote: > On 2016/10/10 14:58:06, Sebastian Noack wrote: > > So if there is an entry in localStorage but it isn't valid JSON, we first call > > the error callback and then the success callback? This seems to be wrong. > > Whoops you're right, sorry I should have spotted that too. > > Ollie I think you need to return after the error callback from inside the > `catch(err)` block. (As a bonus if you return after the success callback too you > could avoid needing the else {...} block for the localforage stuff.) Also > perhaps we should check for `typeof == "string"` here explicitly? > > How about something like this? (untested) > > // First check localStorage in case the file hasn't been migrated to IndexDB yet > var entry = localStorage.getItem(key); > if (typeof entry == "string") > { > try > { > entry = JSON.parse(entry); > } > catch(error) > { > setTimeout(errorCallback(new Error("File is corrupted"))); > return; > } > setTimeout(successCallback(entry)); > return; > } > > // Finally check IndexDB > localforage.getItem(key).then(successCallback, errorCallback); Done.
LGTM https://codereview.adblockplus.org/29355962/diff/29356368/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29355962/diff/29356368/lib/adblockplus.js#... lib/adblockplus.js:380: if (err || !value) On 2016/10/11 13:52:13, Oleksandr wrote: > On 2016/10/10 14:58:06, Sebastian Noack wrote: > > The check for !value seems to be wrong. So if an entry stores an empty string, > a > > use case that is generally supported, you treat this as an error which I think > > we should not. > > I don't think this is generally supported. This is not a string, but object with > `lastModified` and `content` properties. `content` may very well be an empty > string, but if a value here is falsey this is very much an error. Alright, I missed that the value is an object not the string with the actual data, still I wonder whether this check is necessary, but at least it shouldn't break anything.
LGTM
Message was sent while issue was closed.
https://codereview.adblockplus.org/29355962/diff/29356368/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29355962/diff/29356368/lib/adblockplus.js#... lib/adblockplus.js:380: if (err || !value) On 2016/10/11 17:15:15, Sebastian Noack wrote: > On 2016/10/11 13:52:13, Oleksandr wrote: > > On 2016/10/10 14:58:06, Sebastian Noack wrote: > > > The check for !value seems to be wrong. So if an entry stores an empty > string, > > a > > > use case that is generally supported, you treat this as an error which I > think > > > we should not. > > > > I don't think this is generally supported. This is not a string, but object > with > > `lastModified` and `content` properties. `content` may very well be an empty > > string, but if a value here is falsey this is very much an error. > > Alright, I missed that the value is an object not the string with the actual > data, still I wonder whether this check is necessary, but at least it shouldn't > break anything. It does break things if we remove this check. In case the value is not in IndexedDB database there is no error, but the value is null. We need to map that to error. For example for cases when the code is checking if the patterns-backup exists (which is still does). 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
