Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

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.
Reviewers:
Sebastian Noack, kzar
Visibility:
Public.

Description

Issue 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+40 lines, -59 lines) Patch
M background.html View 1 chunk +1 line, -1 line 0 comments Download
M lib/adblockplus.js View 1 2 3 4 5 6 7 8 6 chunks +38 lines, -57 lines 0 comments Download
M qunit/index.html View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 29
Oleksandr
Using IndexedDB directly turned out to complicate code considerably. I have ended up just using ...
Oct. 5, 2016, 9:42 a.m. (2016-10-05 09:42:58 UTC) #1
kzar
On 2016/10/05 09:42:58, Oleksandr wrote: > Using IndexedDB directly turned out to complicate code considerably. ...
Oct. 5, 2016, 11:39 a.m. (2016-10-05 11:39:36 UTC) #2
Oleksandr
On 2016/10/05 11:39:36, kzar wrote: > On 2016/10/05 09:42:58, Oleksandr wrote: > > Using IndexedDB ...
Oct. 5, 2016, 11:57 a.m. (2016-10-05 11:57:07 UTC) #3
kzar
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#newcode376 lib/adblockplus.js:376: entry = ...
Oct. 5, 2016, 12:14 p.m. (2016-10-05 12:14:17 UTC) #4
Sebastian Noack
As far as this is just an experimental hack in adblockplusedgems, fine with me using ...
Oct. 5, 2016, 12:32 p.m. (2016-10-05 12:32:31 UTC) #5
Oleksandr
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#newcode376 lib/adblockplus.js:376: entry = ...
Oct. 5, 2016, 1:32 p.m. (2016-10-05 13:32:40 UTC) #6
kzar
(That's a kind of weird name for a patch set "Patch Set 2 : Patch ...
Oct. 5, 2016, 1:46 p.m. (2016-10-05 13:46:54 UTC) #7
Oleksandr
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): ...
Oct. 5, 2016, 9:07 p.m. (2016-10-05 21:07:41 UTC) #8
Sebastian Noack
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#newcode376 lib/adblockplus.js:376: entry = localforage.getItem(key, function(err, value) On 2016/10/05 21:07:40, Oleksandr ...
Oct. 5, 2016, 9:27 p.m. (2016-10-05 21:27:32 UTC) #9
Sebastian Noack
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#newcode398 lib/adblockplus.js:398: localforage.setItem(key, entry, callback); On 2016/10/05 21:27:31, Sebastian Noack wrote: ...
Oct. 5, 2016, 9:28 p.m. (2016-10-05 21:28:38 UTC) #10
Oleksandr
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#newcode376 lib/adblockplus.js:376: entry = ...
Oct. 5, 2016, 9:49 p.m. (2016-10-05 21:49:13 UTC) #11
Sebastian Noack
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#newcode376 lib/adblockplus.js:376: entry = localforage.getItem(key, function(err, value) On 2016/10/05 21:49:12, Oleksandr ...
Oct. 5, 2016, 9:53 p.m. (2016-10-05 21:53:30 UTC) #12
Oleksandr
Patch Set 5 : Get back to async loadFile. Don't return a promise. Check localStorage. ...
Oct. 5, 2016, 10:57 p.m. (2016-10-05 22:57:36 UTC) #13
kzar
LGTM
Oct. 6, 2016, 5:18 a.m. (2016-10-06 05:18:18 UTC) #14
Sebastian Noack
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#newcode366 lib/adblockplus.js:366: successCallback(JSON.parse(entry)); Don't we have to respond asynchronously here (i.e. ...
Oct. 6, 2016, 12:07 p.m. (2016-10-06 12:07:36 UTC) #15
Oleksandr
Patch Set 6 : Make sure we always return asynchronously from loadFile. Add try-catch block. ...
Oct. 6, 2016, 11:28 p.m. (2016-10-06 23:28:14 UTC) #16
Sebastian Noack
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#newcode366 lib/adblockplus.js:366: successCallback(JSON.parse(entry)); On 2016/10/06 23:28:13, Oleksandr wrote: > On 2016/10/06 ...
Oct. 6, 2016, 11:38 p.m. (2016-10-06 23:38:35 UTC) #17
Oleksandr
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#newcode368 lib/adblockplus.js:368: ...
Oct. 7, 2016, 4:29 p.m. (2016-10-07 16:29:18 UTC) #18
kzar
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#newcode368 lib/adblockplus.js:368: setTimeout(successCallback(JSON.parse(entry)), 0); On 2016/10/07 16:29:17, Oleksandr wrote: > On ...
Oct. 7, 2016, 5 p.m. (2016-10-07 17:00:01 UTC) #19
Oleksandr
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#newcode368 lib/adblockplus.js:368: setTimeout(successCallback(JSON.parse(entry)), 0); On 2016/10/07 17:00:00, kzar wrote: > On ...
Oct. 7, 2016, 5:21 p.m. (2016-10-07 17:21:07 UTC) #20
Oleksandr
Oct. 10, 2016, 9:50 a.m. (2016-10-10 09:50:47 UTC) #21
kzar
LGTM
Oct. 10, 2016, 11:11 a.m. (2016-10-10 11:11:32 UTC) #22
Sebastian Noack
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#newcode374 lib/adblockplus.js:374: setTimeout(successCallback(entry)); So if there is an entry in localStorage ...
Oct. 10, 2016, 2:58 p.m. (2016-10-10 14:58:07 UTC) #23
kzar
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#newcode374 lib/adblockplus.js:374: setTimeout(successCallback(entry)); On 2016/10/10 14:58:06, Sebastian Noack wrote: > So ...
Oct. 11, 2016, 7:22 a.m. (2016-10-11 07:22:42 UTC) #24
Oleksandr
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#newcode380 ...
Oct. 11, 2016, 1:52 p.m. (2016-10-11 13:52:13 UTC) #25
Oleksandr
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#newcode374 lib/adblockplus.js:374: setTimeout(successCallback(entry)); On 2016/10/11 07:22:41, kzar wrote: > On 2016/10/10 ...
Oct. 11, 2016, 1:52 p.m. (2016-10-11 13:52:54 UTC) #26
Sebastian Noack
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#newcode380 lib/adblockplus.js:380: if (err || !value) On 2016/10/11 13:52:13, Oleksandr ...
Oct. 11, 2016, 5:15 p.m. (2016-10-11 17:15:15 UTC) #27
kzar
LGTM
Oct. 11, 2016, 6:25 p.m. (2016-10-11 18:25:59 UTC) #28
Oleksandr
Oct. 11, 2016, 7:45 p.m. (2016-10-11 19:45:55 UTC) #29
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).

Powered by Google App Engine
This is Rietveld