Issue 2021 - Replaced FileSystem API with chrome.storage.local

Issue 2021 - Replaced FileSystem API with chrome.storage.local

[kzar, 2015-03-09 16:16:07]

http://codereview.adblockplus.org/5768064935133184/diff/5741031244955648/lib/...
File lib/io.js (right):

http://codereview.adblockplus.org/5768064935133184/diff/5741031244955648/lib/...
lib/io.js:29: ext.storage.get([key], function(items)
I guess they key here shouldn't be wrapped in an array here either. (I noticed a
similar comment in the review for the tests.)

http://codereview.adblockplus.org/5768064935133184/diff/5741031244955648/lib/...
lib/io.js:138: // Migrate files for users updating from old versions.
I think this code should be above the exports.IO section.

[Sebastian Noack, 2015-03-09 16:24:43]

http://codereview.adblockplus.org/5768064935133184/diff/5741031244955648/lib/...
File lib/io.js (right):

http://codereview.adblockplus.org/5768064935133184/diff/5741031244955648/lib/...
lib/io.js:29: ext.storage.get([key], function(items)
On 2015/03/09 16:16:07, kzar wrote:
> I guess they key here shouldn't be wrapped in an array here either. (I noticed
a
> similar comment in the review for the tests.)

In the tests we use the native chrome.storage.local.get API, to verify the data
where written correctly. And yes that API can be called either with a string or
an array of strings. However, ext.storage.get requires an array at least on
Safari, where I didn't mind to implement the silly shorthand form for
simplicity.

http://codereview.adblockplus.org/5768064935133184/diff/5741031244955648/lib/...
lib/io.js:138: // Migrate files for users updating from old versions.
On 2015/03/09 16:16:07, kzar wrote:
> I think this code should be above the exports.IO section.

Strictly the globals and variable definitions should be at the top, and the code
running the migration at the bottom, but I think it makes sense to keep all the
migration code together.

[kzar, 2015-03-09 16:35:34]

http://codereview.adblockplus.org/5768064935133184/diff/5741031244955648/lib/...
File lib/io.js (right):

http://codereview.adblockplus.org/5768064935133184/diff/5741031244955648/lib/...
lib/io.js:29: ext.storage.get([key], function(items)
On 2015/03/09 16:24:43, Sebastian Noack wrote:
> On 2015/03/09 16:16:07, kzar wrote:
> > I guess they key here shouldn't be wrapped in an array here either. (I
noticed
> a
> > similar comment in the review for the tests.)
> 
> In the tests we use the native chrome.storage.local.get API, to verify the
data
> where written correctly. And yes that API can be called either with a string
or
> an array of strings. However, ext.storage.get requires an array at least on
> Safari, where I didn't mind to implement the silly shorthand form for
> simplicity.

OK, fair enough.

http://codereview.adblockplus.org/5768064935133184/diff/5741031244955648/lib/...
lib/io.js:138: // Migrate files for users updating from old versions.
On 2015/03/09 16:24:43, Sebastian Noack wrote:
> On 2015/03/09 16:16:07, kzar wrote:
> > I think this code should be above the exports.IO section.
> 
> Strictly the globals and variable definitions should be at the top, and the
code
> running the migration at the bottom, but I think it makes sense to keep all
the
> migration code together.

Well I actually meant this whole section, the variables and the functions when I
said that I think this code should be at the top.

I don't think it's hugely important, just makes the exports.IO code a bit easier
to read IMO.

[Sebastian Noack, 2015-03-09 16:43:17]

http://codereview.adblockplus.org/5768064935133184/diff/5741031244955648/lib/...
File lib/io.js (right):

http://codereview.adblockplus.org/5768064935133184/diff/5741031244955648/lib/...
lib/io.js:138: // Migrate files for users updating from old versions.
On 2015/03/09 16:35:34, kzar wrote:
> On 2015/03/09 16:24:43, Sebastian Noack wrote:
> > On 2015/03/09 16:16:07, kzar wrote:
> > > I think this code should be above the exports.IO section.
> > 
> > Strictly the globals and variable definitions should be at the top, and the
> code
> > running the migration at the bottom, but I think it makes sense to keep all
> the
> > migration code together.
> 
> Well I actually meant this whole section, the variables and the functions when
I
> said that I think this code should be at the top.
> 
> I don't think it's hugely important, just makes the exports.IO code a bit
easier
> to read IMO.

IMO the opposite is the case.

1. When you come to this file you are most likely interested in the
implementation of the IO.* APIs, and not in the migration code that runs only
once, and will be removed sooner or later.
2. The way the code is currently structured it's obvious on first sight that
everyting below the comment is migration code. But when moving it at the top,
you have to figure out where the migration code actually ends.

[kzar, 2015-03-09 16:53:33]

A lot of the migration code is pretty ugly but at least it's temporary I suppose
and I can't see a much better way to do any of it.

LGTM but I'm not very familiar with a lot of this code so I think Wladimir
should definitely have a look as well.

[Wladimir Palant, 2015-04-20 12:09:07]

LGTM