Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(96)

Issue 29355962: Issue 4023 - Use localforage (IndexedDB) instead of localStorage (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 4 months ago by Oleksandr
Modified:
3 years, 4 months ago
Reviewers:
kzar, Sebastian Noack
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 ...
3 years, 4 months ago (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. ...
3 years, 4 months ago (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 ...
3 years, 4 months ago (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 = ...
3 years, 4 months ago (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 ...
3 years, 4 months ago (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 = ...
3 years, 4 months ago (2016-10-05 13:32:40 UTC) #6
kzar
(That's a kind of weird name for a patch set "Patch Set 2 : Patch ...
3 years, 4 months ago (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): ...
3 years, 4 months ago (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 ...
3 years, 4 months ago (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: ...
3 years, 4 months ago (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 = ...
3 years, 4 months ago (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 ...
3 years, 4 months ago (2016-10-05 21:53:30 UTC) #12
Oleksandr
Patch Set 5 : Get back to async loadFile. Don't return a promise. Check localStorage. ...
3 years, 4 months ago (2016-10-05 22:57:36 UTC) #13
kzar
LGTM
3 years, 4 months ago (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. ...
3 years, 4 months ago (2016-10-06 12:07:36 UTC) #15
Oleksandr
Patch Set 6 : Make sure we always return asynchronously from loadFile. Add try-catch block. ...
3 years, 4 months ago (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 ...
3 years, 4 months ago (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: ...
3 years, 4 months ago (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 ...
3 years, 4 months ago (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 ...
3 years, 4 months ago (2016-10-07 17:21:07 UTC) #20
Oleksandr
3 years, 4 months ago (2016-10-10 09:50:47 UTC) #21
kzar
LGTM
3 years, 4 months ago (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 ...
3 years, 4 months ago (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 ...
3 years, 4 months ago (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 ...
3 years, 4 months ago (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 ...
3 years, 4 months ago (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 ...
3 years, 4 months ago (2016-10-11 17:15:15 UTC) #27
kzar
LGTM
3 years, 4 months ago (2016-10-11 18:25:59 UTC) #28
Oleksandr
3 years, 4 months ago (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).
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5