Issue 5562 - Move Edge storage to IndexedDB

This is intended to be published under the edge bookmark.

[Oleksandr, 2017-08-24 11:48:46]

https://codereview.adblockplus.org/29526591/diff/29526592/metadata.edge
File metadata.edge (right):

https://codereview.adblockplus.org/29526591/diff/29526592/metadata.edge#newco...
metadata.edge:12: backgroundScripts = lib/localforage.min.js
We need to include localforage first, that's why we duplicate the list of
scripts here.

[Sebastian Noack, 2017-08-24 12:11:47]

https://codereview.adblockplus.org/29526591/diff/29526592/lib/io.js
File lib/io.js (right):

https://codereview.adblockplus.org/29526591/diff/29526592/lib/io.js#newcode33
lib/io.js:33: // Make sure we do not have subscriptions in localStorage from
older
We use IndexedDB for quite a while, now. So I guess this migration code can be
removed now?

https://codereview.adblockplus.org/29526591/diff/29526592/lib/io.js#newcode53
lib/io.js:53: if (err || !value)
Any particular reason, we check for `!value`? It there is an entry for that key,
but the value is empty, wouldn't it be more correct to not report an error here,
but to just resolve with an empty result?

https://codereview.adblockplus.org/29526591/diff/29526592/lib/io.js#newcode100
lib/io.js:100: if ("content" in entry)
Why is this check necessary?

https://codereview.adblockplus.org/29526591/diff/29526592/metadata.edge
File metadata.edge (right):

https://codereview.adblockplus.org/29526591/diff/29526592/metadata.edge#newco...
metadata.edge:12: backgroundScripts = lib/localforage.min.js
On 2017/08/24 11:48:46, Oleksandr wrote:
> We need to include localforage first, that's why we duplicate the list of
> scripts here.

Can't you just append it like this:

backgroundScripts += lib/localforage.min.js

[Oleksandr, 2017-08-24 14:43:59]

https://codereview.adblockplus.org/29526591/diff/29526592/lib/io.js
File lib/io.js (right):

https://codereview.adblockplus.org/29526591/diff/29526592/lib/io.js#newcode33
lib/io.js:33: // Make sure we do not have subscriptions in localStorage from
older
On 2017/08/24 12:11:46, Sebastian Noack wrote:
> We use IndexedDB for quite a while, now. So I guess this migration code can be
> removed now?

Fair enough.

https://codereview.adblockplus.org/29526591/diff/29526592/lib/io.js#newcode53
lib/io.js:53: if (err || !value)
On 2017/08/24 12:11:46, Sebastian Noack wrote:
> Any particular reason, we check for `!value`? It there is an entry for that
key,
> but the value is empty, wouldn't it be more correct to not report an error
here,
> but to just resolve with an empty result?

Based on https://localforage.github.io/localForage/ 'If the key does not exist,
getItem() will return null.'. I am not sure when 'err' will be set, but this
seems like a safe way.

https://codereview.adblockplus.org/29526591/diff/29526592/lib/io.js#newcode100
lib/io.js:100: if ("content" in entry)
On 2017/08/24 12:11:46, Sebastian Noack wrote:
> Why is this check necessary?

If content is undefined (for whatever reason) than next line will throw. This is
just in case there's a different storage format from a different plugin version
or something like that.

https://codereview.adblockplus.org/29526591/diff/29526592/metadata.edge
File metadata.edge (right):

https://codereview.adblockplus.org/29526591/diff/29526592/metadata.edge#newco...
metadata.edge:12: backgroundScripts = lib/localforage.min.js
On 2017/08/24 12:11:46, Sebastian Noack wrote:
> On 2017/08/24 11:48:46, Oleksandr wrote:
> > We need to include localforage first, that's why we duplicate the list of
> > scripts here.
> 
> Can't you just append it like this:
> 
> backgroundScripts += lib/localforage.min.js

No, appending adds to the end of list. We need to prepend here.

[Sebastian Noack, 2017-08-24 14:56:27]

https://codereview.adblockplus.org/29526591/diff/29526592/lib/io.js
File lib/io.js (right):

https://codereview.adblockplus.org/29526591/diff/29526592/lib/io.js#newcode53
lib/io.js:53: if (err || !value)
On 2017/08/24 14:43:58, Oleksandr wrote:
> On 2017/08/24 12:11:46, Sebastian Noack wrote:
> > Any particular reason, we check for `!value`? It there is an entry for that
> key,
> > but the value is empty, wouldn't it be more correct to not report an error
> here,
> > but to just resolve with an empty result?
> 
> Based on https://localforage.github.io/localForage/ 'If the key does not
exist,
> getItem() will return null.'. I am not sure when 'err' will be set, but this
> seems like a safe way.

Based on that information we might want to explicitly check for `value == null`,
since `!value` would also trigger on 0 and "".

https://codereview.adblockplus.org/29526591/diff/29526592/lib/io.js#newcode100
lib/io.js:100: if ("content" in entry)
On 2017/08/24 14:43:58, Oleksandr wrote:
> On 2017/08/24 12:11:46, Sebastian Noack wrote:
> > Why is this check necessary?
> 
> If content is undefined (for whatever reason) than next line will throw. This
is
> just in case there's a different storage format from a different plugin
version
> or something like that.

This theoretical scenario isn't handled in upstream either, neither does it seem
to be specific to Microsoft Edge.

https://codereview.adblockplus.org/29526591/diff/29526592/metadata.edge
File metadata.edge (right):

https://codereview.adblockplus.org/29526591/diff/29526592/metadata.edge#newco...
metadata.edge:12: backgroundScripts = lib/localforage.min.js
On 2017/08/24 14:43:58, Oleksandr wrote:
> On 2017/08/24 12:11:46, Sebastian Noack wrote:
> > On 2017/08/24 11:48:46, Oleksandr wrote:
> > > We need to include localforage first, that's why we duplicate the list of
> > > scripts here.
> > 
> > Can't you just append it like this:
> > 
> > backgroundScripts += lib/localforage.min.js
> 
> No, appending adds to the end of list. We need to prepend here. 

Are you sure? Modules are lazily evaluated on first usage. So even if a module
depends on a module listed below, this should still work. We changed this at
some point, so the order might have been mattered before, but not anymore?

[Oleksandr, 2017-08-24 16:53:29]

https://codereview.adblockplus.org/29526591/diff/29526592/lib/io.js
File lib/io.js (right):

https://codereview.adblockplus.org/29526591/diff/29526592/lib/io.js#newcode53
lib/io.js:53: if (err || !value)
On 2017/08/24 14:56:27, Sebastian Noack wrote:
> On 2017/08/24 14:43:58, Oleksandr wrote:
> > On 2017/08/24 12:11:46, Sebastian Noack wrote:
> > > Any particular reason, we check for `!value`? It there is an entry for
that
> > key,
> > > but the value is empty, wouldn't it be more correct to not report an error
> > here,
> > > but to just resolve with an empty result?
> > 
> > Based on https://localforage.github.io/localForage/ 'If the key does not
> exist,
> > getItem() will return null.'. I am not sure when 'err' will be set, but this
> > seems like a safe way.
> 
> Based on that information we might want to explicitly check for `value ==
null`,
> since `!value` would also trigger on 0 and "".

Done.

https://codereview.adblockplus.org/29526591/diff/29526592/lib/io.js#newcode100
lib/io.js:100: if ("content" in entry)
On 2017/08/24 14:56:27, Sebastian Noack wrote:
> On 2017/08/24 14:43:58, Oleksandr wrote:
> > On 2017/08/24 12:11:46, Sebastian Noack wrote:
> > > Why is this check necessary?
> > 
> > If content is undefined (for whatever reason) than next line will throw.
This
> is
> > just in case there's a different storage format from a different plugin
> version
> > or something like that.
> 
> This theoretical scenario isn't handled in upstream either, neither does it
seem
> to be specific to Microsoft Edge.

Removed the check.

https://codereview.adblockplus.org/29526591/diff/29526592/metadata.edge
File metadata.edge (right):

https://codereview.adblockplus.org/29526591/diff/29526592/metadata.edge#newco...
metadata.edge:12: backgroundScripts = lib/localforage.min.js
On 2017/08/24 14:56:27, Sebastian Noack wrote:
> On 2017/08/24 14:43:58, Oleksandr wrote:
> > On 2017/08/24 12:11:46, Sebastian Noack wrote:
> > > On 2017/08/24 11:48:46, Oleksandr wrote:
> > > > We need to include localforage first, that's why we duplicate the list
of
> > > > scripts here.
> > > 
> > > Can't you just append it like this:
> > > 
> > > backgroundScripts += lib/localforage.min.js
> > 
> > No, appending adds to the end of list. We need to prepend here. 
> 
> Are you sure? Modules are lazily evaluated on first usage. So even if a module
> depends on a module listed below, this should still work. We changed this at
> some point, so the order might have been mattered before, but not anymore?

Checked again. We need localforage.min.js before adblockplus.js, but it isn't
available there on startup.

[Sebastian Noack, 2017-08-24 17:11:01]

https://codereview.adblockplus.org/29526591/diff/29526666/lib/io.js
File lib/io.js (right):

https://codereview.adblockplus.org/29526591/diff/29526666/lib/io.js#newcode31
lib/io.js:31: console.trace();
Did you forget to remove this?

[Oleksandr, 2017-08-24 17:17:04]

https://codereview.adblockplus.org/29526591/diff/29526666/lib/io.js
File lib/io.js (right):

https://codereview.adblockplus.org/29526591/diff/29526666/lib/io.js#newcode31
lib/io.js:31: console.trace();
On 2017/08/24 17:11:00, Sebastian Noack wrote:
> Did you forget to remove this?
Whoops. Yes, sorry. Some of the refreshes before commit didn't work apparently.

[Sebastian Noack, 2017-08-24 17:24:25]

LGTM