Issue 2040 - Replaced localStorage with chrome.storage.local

Issue 2040 - Replaced localStorage with chrome.storage.local

[kzar, 2015-03-09 15:10:29]

http://codereview.adblockplus.org/5693109165883392/diff/5741031244955648/chro...
File chrome/ext/background.js (right):

http://codereview.adblockplus.org/5693109165883392/diff/5741031244955648/chro...
chrome/ext/background.js:511: items["pref:" + key] = JSON.parse(value);
Nit: Shouldn't this make use of the const keyPrefix?

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

http://codereview.adblockplus.org/5693109165883392/diff/5741031244955648/lib/...
lib/storage/io.js:36: safari.extension.settings[path] = contents;
Perhaps I'm missing something here but how come you are using
safari.extension.settings in this file? If this is all Safari specific code
shouldn't it live in safari/ext somewhere?

http://codereview.adblockplus.org/5693109165883392/diff/5741031244955648/meta...
File metadata.chrome (right):

http://codereview.adblockplus.org/5693109165883392/diff/5741031244955648/meta...
metadata.chrome:13: storage
Obviously you understand what you're doing here but I figure I should double
check. Wvladimir are you OK with us adding an extra required permission to the
manifest?

http://codereview.adblockplus.org/5693109165883392/diff/5741031244955648/safa...
File safari/ext/background.js (right):

http://codereview.adblockplus.org/5693109165883392/diff/5741031244955648/safa...
safari/ext/background.js:702: settings["pref:" + key] =
JSON.parse(settings[key]);
Nit: Again what about the constant?

[Sebastian Noack, 2015-03-09 15:21:36]

http://codereview.adblockplus.org/5693109165883392/diff/5741031244955648/chro...
File chrome/ext/background.js (right):

http://codereview.adblockplus.org/5693109165883392/diff/5741031244955648/chro...
chrome/ext/background.js:511: items["pref:" + key] = JSON.parse(value);
On 2015/03/09 15:10:29, kzar wrote:
> Nit: Shouldn't this make use of the const keyPrefix?

It's not available here, and IMO not worth beeing exported just for the
temporary migration code.

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

http://codereview.adblockplus.org/5693109165883392/diff/5741031244955648/lib/...
lib/storage/io.js:36: safari.extension.settings[path] = contents;
On 2015/03/09 15:10:29, kzar wrote:
> Perhaps I'm missing something here but how come you are using
> safari.extension.settings in this file? If this is all Safari specific code
> shouldn't it live in safari/ext somewhere?

This is just a temporary hack. This code will be removed with the other patch,
which unifies the IO module, to use the new asynchronous ext.storage on all
platforms. Initially this file were used for Opera as well, but now it's only
Safari.

http://codereview.adblockplus.org/5693109165883392/diff/5741031244955648/meta...
File metadata.chrome (right):

http://codereview.adblockplus.org/5693109165883392/diff/5741031244955648/meta...
metadata.chrome:13: storage
On 2015/03/09 15:10:29, kzar wrote:
> Obviously you understand what you're doing here but I figure I should double
> check. Wvladimir are you OK with us adding an extra required permission to the
> manifest?

There is no way around, if we want to use chrome.storage.

http://codereview.adblockplus.org/5693109165883392/diff/5741031244955648/safa...
File safari/ext/background.js (right):

http://codereview.adblockplus.org/5693109165883392/diff/5741031244955648/safa...
safari/ext/background.js:702: settings["pref:" + key] =
JSON.parse(settings[key]);
On 2015/03/09 15:10:29, kzar wrote:
> Nit: Again what about the constant?

It's not available here, and IMO not worth beeing exported just for the
temporary migration code.

[kzar, 2015-03-09 15:24:28]

http://codereview.adblockplus.org/5693109165883392/diff/5741031244955648/chro...
File chrome/ext/background.js (right):

http://codereview.adblockplus.org/5693109165883392/diff/5741031244955648/chro...
chrome/ext/background.js:511: items["pref:" + key] = JSON.parse(value);
On 2015/03/09 15:21:37, Sebastian Noack wrote:
> On 2015/03/09 15:10:29, kzar wrote:
> > Nit: Shouldn't this make use of the const keyPrefix?
> 
> It's not available here, and IMO not worth beeing exported just for the
> temporary migration code.

Fair enough, yea I agree it doesn't really matter.

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

http://codereview.adblockplus.org/5693109165883392/diff/5741031244955648/lib/...
lib/storage/io.js:36: safari.extension.settings[path] = contents;
On 2015/03/09 15:21:37, Sebastian Noack wrote:
> On 2015/03/09 15:10:29, kzar wrote:
> > Perhaps I'm missing something here but how come you are using
> > safari.extension.settings in this file? If this is all Safari specific code
> > shouldn't it live in safari/ext somewhere?
> 
> This is just a temporary hack. This code will be removed with the other patch,
> which unifies the IO module, to use the new asynchronous ext.storage on all
> platforms. Initially this file were used for Opera as well, but now it's only
> Safari.

Fair enough but in that case there should be a quick comment in this file to
explain that is the case IMO.

[Sebastian Noack, 2015-03-09 15:27:49]

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

http://codereview.adblockplus.org/5693109165883392/diff/5741031244955648/lib/...
lib/storage/io.js:36: safari.extension.settings[path] = contents;
On 2015/03/09 15:24:28, kzar wrote:
> On 2015/03/09 15:21:37, Sebastian Noack wrote:
> > On 2015/03/09 15:10:29, kzar wrote:
> > > Perhaps I'm missing something here but how come you are using
> > > safari.extension.settings in this file? If this is all Safari specific
code
> > > shouldn't it live in safari/ext somewhere?
> > 
> > This is just a temporary hack. This code will be removed with the other
patch,
> > which unifies the IO module, to use the new asynchronous ext.storage on all
> > platforms. Initially this file were used for Opera as well, but now it's
only
> > Safari.
> 
> Fair enough but in that case there should be a quick comment in this file to
> explain that is the case IMO.

Note that I'm going to push both patches together once they are reviewed. I
merely split the change, introducing this hack here, to make it easier to
review.

[kzar, 2015-03-09 15:32:54]

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

http://codereview.adblockplus.org/5693109165883392/diff/5741031244955648/lib/...
lib/storage/io.js:36: safari.extension.settings[path] = contents;
On 2015/03/09 15:27:49, Sebastian Noack wrote:
> On 2015/03/09 15:24:28, kzar wrote:
> > On 2015/03/09 15:21:37, Sebastian Noack wrote:
> > > On 2015/03/09 15:10:29, kzar wrote:
> > > > Perhaps I'm missing something here but how come you are using
> > > > safari.extension.settings in this file? If this is all Safari specific
> code
> > > > shouldn't it live in safari/ext somewhere?
> > > 
> > > This is just a temporary hack. This code will be removed with the other
> patch,
> > > which unifies the IO module, to use the new asynchronous ext.storage on
all
> > > platforms. Initially this file were used for Opera as well, but now it's
> only
> > > Safari.
> > 
> > Fair enough but in that case there should be a quick comment in this file to
> > explain that is the case IMO.
> 
> Note that I'm going to push both patches together once they are reviewed. I
> merely split the change, introducing this hack here, to make it easier to
> review.

OK that makes more sense then, looking at the other review now.

[kzar, 2015-03-09 16:55:19]

Same as review for 2021, pretty much LGTM but I really think Wladimir should
check it as well.

http://codereview.adblockplus.org/5693109165883392/diff/5741031244955648/meta...
File metadata.chrome (right):

http://codereview.adblockplus.org/5693109165883392/diff/5741031244955648/meta...
metadata.chrome:13: storage
On 2015/03/09 15:21:37, Sebastian Noack wrote:
> On 2015/03/09 15:10:29, kzar wrote:
> > Obviously you understand what you're doing here but I figure I should double
> > check. Wvladimir are you OK with us adding an extra required permission to
the
> > manifest?
> 
> There is no way around, if we want to use chrome.storage.

I realise that.

[Wladimir Palant, 2015-03-12 23:05:37]

http://codereview.adblockplus.org/5693109165883392/diff/5741031244955648/lib/...
File lib/prefs.js (right):

http://codereview.adblockplus.org/5693109165883392/diff/5741031244955648/lib/...
lib/prefs.js:91: }
The flow here is rather convoluted, a change to prefs.foo does through several
instances and prefs[pref] is set twice. The point seems to be considering
external changes (is there even a UI for that?). However, if we do that more
effort is required:

* What if an external change changes preference type? We probably want to log an
error and fall back to default.

* What if an external change stores the default value in storage? We probably
want prefs[pref] to be undefined then.

Did you find code paths that rely on Prefs.foo value changing synchronously?

http://codereview.adblockplus.org/5693109165883392/diff/5639274879778816/chro...
File chrome/ext/background.js (right):

http://codereview.adblockplus.org/5693109165883392/diff/5639274879778816/chro...
chrome/ext/background.js:543: }
This logic doesn't really belong in the generic abstraction layer. If you want
to keep it here, migratePrefs() should get a function as parameter that will map
old names to new ones (or return null if the key is unknown). This way the logic
here will stay generic.

http://codereview.adblockplus.org/5693109165883392/diff/5639274879778816/safa...
File safari/ext/background.js (right):

http://codereview.adblockplus.org/5693109165883392/diff/5639274879778816/safa...
safari/ext/background.js:663: setTimeout(callback, 0, items);
Did you verify that this works on all Safari versions we support? MDN doesn't
know when Safari added support for additional setTimeout parameters.

[Sebastian Noack, 2015-03-12 23:35:29]

Looking into the other comments tomorrow.

http://codereview.adblockplus.org/5693109165883392/diff/5741031244955648/lib/...
File lib/prefs.js (right):

http://codereview.adblockplus.org/5693109165883392/diff/5741031244955648/lib/...
lib/prefs.js:91: }
On 2015/03/12 23:05:37, Wladimir Palant wrote:
> The flow here is rather convoluted, a change to prefs.foo does through several
> instances and prefs[pref] is set twice. The point seems to be considering
> external changes (is there even a UI for that?). However, if we do that more
> effort is required:
> 
> * What if an external change changes preference type? We probably want to log
an
> error and fall back to default.
> 
> * What if an external change stores the default value in storage? We probably
> want prefs[pref] to be undefined then.
> 
> Did you find code paths that rely on Prefs.foo value changing synchronously?

Currently there is only one case where we externally change prefs, that is the
migration code. However, in the long-term when we don't import the Prefs object
into the options page anymore, calling ext.storage.local.set will be the easiest
and only cross-platform way for users to set hidden preferences. Moreover, when
we implement chrome.storage.managed it there can be external changes we have to
respond to via chrome.storage.on changed as well. So yes we need to handle
external changes.

Do we need to check the type for those? Not for chrome.storage.managed since it
is strictly typed anyway (we have to provide a manifest listing the possible
keys and the type of their values). For the migration code it can be ignored
IMO. The prefs imported there were matter of the same type checks before. So
they should be valid unless somebody messed with localStorage directly, a case
that also wasn't handled before. For the case that some user manually calls
chrhrome.storage.set this might make sense, but when messing with the JS console
there are so many ways to screw up ABP that it isnt probably worth handling this
particular one.

http://codereview.adblockplus.org/5693109165883392/diff/5639274879778816/chro...
File chrome/ext/background.js (right):

http://codereview.adblockplus.org/5693109165883392/diff/5639274879778816/chro...
chrome/ext/background.js:543: }
On 2015/03/12 23:05:37, Wladimir Palant wrote:
> This logic doesn't really belong in the generic abstraction layer. If you want
> to keep it here, migratePrefs() should get a function as parameter that will
map
> old names to new ones (or return null if the key is unknown). This way the
logic
> here will stay generic.

Well, where doese code with a different implementation for both platforms belong
if not in the abstraction layer? But I'm fine with passing a map func instead an
object.

http://codereview.adblockplus.org/5693109165883392/diff/5639274879778816/safa...
File safari/ext/background.js (right):

http://codereview.adblockplus.org/5693109165883392/diff/5639274879778816/safa...
safari/ext/background.js:663: setTimeout(callback, 0, items);
On 2015/03/12 23:05:37, Wladimir Palant wrote:
> Did you verify that this works on all Safari versions we support? MDN doesn't
> know when Safari added support for additional setTimeout parameters.

I saw this one coming. I checked MDN and made the same observation as you did.
So I tried on Safari 5.1 (the oldest version ABP runs on) and Safari 8 (the
newest one). And also to my surprise that is actually supported. :D

[Sebastian Noack, 2015-03-13 10:30:39]

http://codereview.adblockplus.org/5693109165883392/diff/5639274879778816/chro...
File chrome/ext/background.js (right):

http://codereview.adblockplus.org/5693109165883392/diff/5639274879778816/chro...
chrome/ext/background.js:543: }
On 2015/03/12 23:35:29, Sebastian Noack wrote:
> On 2015/03/12 23:05:37, Wladimir Palant wrote:
> > If you want
> > to keep it here, migratePrefs() should get a function as parameter that will
> map
> > old names to new ones (or return null if the key is unknown). This way the
> logic
> > here will stay generic.
> 
> But I'm fine with passing a map func instead an object.

Done.

[Wladimir Palant, 2015-03-16 12:20:26]

http://codereview.adblockplus.org/5693109165883392/diff/5681717746597888/safa...
File safari/ext/background.js (right):

http://codereview.adblockplus.org/5693109165883392/diff/5681717746597888/safa...
safari/ext/background.js:713: if (item)
Add |&& item.key != key| here? I think we will keep "migrating" currentVersion
until the end of times otherwise. Should have a comment explaining this of
course.

[Sebastian Noack, 2015-03-16 12:54:34]

http://codereview.adblockplus.org/5693109165883392/diff/5681717746597888/safa...
File safari/ext/background.js (right):

http://codereview.adblockplus.org/5693109165883392/diff/5681717746597888/safa...
safari/ext/background.js:713: if (item)
On 2015/03/16 12:20:26, Wladimir Palant wrote:
> Add |&& item.key != key| here? I think we will keep "migrating" currentVersion
> until the end of times otherwise. Should have a comment explaining this of
> course.

Done.

[Wladimir Palant, 2015-03-16 13:27:36]

LGTM

[Sebastian Noack, 2015-03-20 13:22:26]

I have merged some changes from
http://codereview.adblockplus.org/5251132066627584 into this patch, since they
make sense for chrome.storage.local as well.

[Wladimir Palant, 2015-04-09 19:20:15]

http://codereview.adblockplus.org/5693109165883392/diff/5740423507083264/back...
File background.js (left):

http://codereview.adblockplus.org/5693109165883392/diff/5740423507083264/back...
background.js:62: var canUseChromeNotifications = require("info").platform ==
"chromium"
This variable is also used in showNotification() function.

[Sebastian Noack, 2015-04-09 21:35:54]

http://codereview.adblockplus.org/5693109165883392/diff/5740423507083264/back...
File background.js (left):

http://codereview.adblockplus.org/5693109165883392/diff/5740423507083264/back...
background.js:62: var canUseChromeNotifications = require("info").platform ==
"chromium"
On 2015/04/09 19:20:15, Wladimir Palant wrote:
> This variable is also used in showNotification() function.

Oh, you are right, I move it back here. But if you don't mind I keep the new
logic using Service.vc.compare().

[Wladimir Palant, 2015-04-09 21:57:18]

LGTM

[kzar, 2015-04-13 10:26:39]

Otherwise LGTM

http://codereview.adblockplus.org/5693109165883392/diff/5635415851663360/chro...
File chrome/ext/background.js (right):

http://codereview.adblockplus.org/5693109165883392/diff/5635415851663360/chro...
chrome/ext/background.js:520: // ignoring unkown and inavlid preferences.
Nit: Typo in this comment "inavlid".

[Sebastian Noack, 2015-04-13 10:30:24]

http://codereview.adblockplus.org/5693109165883392/diff/5635415851663360/chro...
File chrome/ext/background.js (right):

http://codereview.adblockplus.org/5693109165883392/diff/5635415851663360/chro...
chrome/ext/background.js:520: // ignoring unkown and inavlid preferences.
On 2015/04/13 10:26:39, kzar wrote:
> Nit: Typo in this comment "inavlid".

Done.

[kzar, 2015-04-13 10:31:10]

LGTM

[Wladimir Palant, 2015-04-13 10:53:03]

Still LGTM