Finished storage polyfill.

Finished storage polyfill.

Included in this CL are all the other functions of the storage polyfill
that were split out of the last storage polyfill CL, including set,
clear, and remove, as well as a function exposed only for testing.

R=raymes@chromium.org

Committed: c1ac20bcf0bea9c8b0a4229e8cf7c4ae6f8733c3

[Matthew Alger, 2016-01-15 04:21:26]

alger@google.com changed reviewers:
+ raymes@chromium.org

[Matthew Alger, 2016-01-15 04:21:27]

Second half of the storage polyfill CL.

[raymes, 2016-01-18 00:25:20]

https://codereview.chromium.org/1593443003/diff/1/src/js/polyfills/storage.po...
File src/js/polyfills/storage.polyfill.js (right):

https://codereview.chromium.org/1593443003/diff/1/src/js/polyfills/storage.po...
src/js/polyfills/storage.polyfill.js:147: *  list of keys to get, or a
dictionary specifying default values. Empty
nit: 4 space indent (here and below)

https://codereview.chromium.org/1593443003/diff/1/src/js/polyfills/storage.po...
src/js/polyfills/storage.polyfill.js:191: opt_callback();
what if opt_callback is undefined?

https://codereview.chromium.org/1593443003/diff/1/src/js/polyfills/storage.po...
src/js/polyfills/storage.polyfill.js:205: opt_callback();
what if opt_callback is undefined?

https://codereview.chromium.org/1593443003/diff/1/src/js/polyfills/storage.po...
src/js/polyfills/storage.polyfill.js:209: if (Array.isArray(keys)) {
If keys is a string, you could turn it into an array and just have a single
codepath here. This would remove some code duplication. 

You could do this to remove one codepath in get() as well (optional).

https://codereview.chromium.org/1593443003/diff/1/src/js/polyfills/storage.po...
src/js/polyfills/storage.polyfill.js:218: opt_callback();
what if opt_callback is undefined?

https://codereview.chromium.org/1593443003/diff/1/src/js/polyfills/storage.po...
src/js/polyfills/storage.polyfill.js:248: localforage.clear(opt_callback);
I think it's probably better to clear things before the callback is called
because the callbacks might rely on this. You should do this in the callback for
clear()

https://codereview.chromium.org/1593443003/diff/1/src/js/polyfills/storage.po...
src/js/polyfills/storage.polyfill.js:252: if (opt_callback)
This is inconsistent with other places where you have if (opt_callback !=
undefined). I would suggest using this notation everywhere since it's clearer
and there's no real downsides (e.g. if opt_callback did not evaluate to true
then we would not want to try to call it anyway).

https://codereview.chromium.org/1593443003/diff/1/tests/js/polyfills/storage....
File tests/js/polyfills/storage.polyfill.test.js (right):

https://codereview.chromium.org/1593443003/diff/1/tests/js/polyfills/storage....
tests/js/polyfills/storage.polyfill.test.js:336: assert.expect(0);
What's this for?

https://codereview.chromium.org/1593443003/diff/1/tests/js/polyfills/storage....
tests/js/polyfills/storage.polyfill.test.js:413:
chrome.storage.onChanged.addListener(function(changes) {
You could probably merge all the multiple handler tests into the single handler
ones - testing multiple would presumably cover the single case

[Matthew Alger, 2016-01-18 00:37:37]

https://codereview.chromium.org/1593443003/diff/1/src/js/polyfills/storage.po...
File src/js/polyfills/storage.polyfill.js (right):

https://codereview.chromium.org/1593443003/diff/1/src/js/polyfills/storage.po...
src/js/polyfills/storage.polyfill.js:147: *  list of keys to get, or a
dictionary specifying default values. Empty
On 2016/01/18 00:25:20, raymes wrote:
> nit: 4 space indent (here and below)

Done.

https://codereview.chromium.org/1593443003/diff/1/src/js/polyfills/storage.po...
src/js/polyfills/storage.polyfill.js:191: opt_callback();
On 2016/01/18 00:25:20, raymes wrote:
> what if opt_callback is undefined?

Done.

https://codereview.chromium.org/1593443003/diff/1/src/js/polyfills/storage.po...
src/js/polyfills/storage.polyfill.js:205: opt_callback();
On 2016/01/18 00:25:20, raymes wrote:
> what if opt_callback is undefined?

Done.

https://codereview.chromium.org/1593443003/diff/1/src/js/polyfills/storage.po...
src/js/polyfills/storage.polyfill.js:209: if (Array.isArray(keys)) {
On 2016/01/18 00:25:20, raymes wrote:
> If keys is a string, you could turn it into an array and just have a single
> codepath here. This would remove some code duplication. 
> 
> You could do this to remove one codepath in get() as well (optional).

Oh, good idea! Done, and done in get too.

https://codereview.chromium.org/1593443003/diff/1/src/js/polyfills/storage.po...
src/js/polyfills/storage.polyfill.js:218: opt_callback();
On 2016/01/18 00:25:20, raymes wrote:
> what if opt_callback is undefined?

Done.

https://codereview.chromium.org/1593443003/diff/1/src/js/polyfills/storage.po...
src/js/polyfills/storage.polyfill.js:248: localforage.clear(opt_callback);
On 2016/01/18 00:25:20, raymes wrote:
> I think it's probably better to clear things before the callback is called
> because the callbacks might rely on this. You should do this in the callback
for
> clear()

Do you mean it's better to clear things before the onChanged event is triggered?
The callback is already called after clear.

I assume you mean the trigger, so I've fixed that.

https://codereview.chromium.org/1593443003/diff/1/src/js/polyfills/storage.po...
src/js/polyfills/storage.polyfill.js:252: if (opt_callback)
On 2016/01/18 00:25:20, raymes wrote:
> This is inconsistent with other places where you have if (opt_callback !=
> undefined). I would suggest using this notation everywhere since it's clearer
> and there's no real downsides (e.g. if opt_callback did not evaluate to true
> then we would not want to try to call it anyway).

Changed everything to just check opt_callback's truthiness.

https://codereview.chromium.org/1593443003/diff/1/tests/js/polyfills/storage....
File tests/js/polyfills/storage.polyfill.test.js (right):

https://codereview.chromium.org/1593443003/diff/1/tests/js/polyfills/storage....
tests/js/polyfills/storage.polyfill.test.js:336: assert.expect(0);
On 2016/01/18 00:25:20, raymes wrote:
> What's this for?

expect tells QUnit how many assertions there will be in the test. This tells
QUnit we don't expect any assertions - we just expect the handlers to be called.

https://codereview.chromium.org/1593443003/diff/1/tests/js/polyfills/storage....
tests/js/polyfills/storage.polyfill.test.js:413:
chrome.storage.onChanged.addListener(function(changes) {
On 2016/01/18 00:25:20, raymes wrote:
> You could probably merge all the multiple handler tests into the single
handler
> ones - testing multiple would presumably cover the single case

I'm not sure that's entirely true. It's easily possible to implement this such
that only one handler can be registered at a time, and having single/multiple
split like this would reveal that.

[raymes, 2016-01-18 02:13:34]

lgtm with a couple of comments

https://codereview.chromium.org/1593443003/diff/1/tests/js/polyfills/storage....
File tests/js/polyfills/storage.polyfill.test.js (right):

https://codereview.chromium.org/1593443003/diff/1/tests/js/polyfills/storage....
tests/js/polyfills/storage.polyfill.test.js:336: assert.expect(0);
On 2016/01/18 00:37:37, Matthew Alger wrote:
> On 2016/01/18 00:25:20, raymes wrote:
> > What's this for?
> 
> expect tells QUnit how many assertions there will be in the test. This tells
> QUnit we don't expect any assertions - we just expect the handlers to be
called.

I see - so do things fail without this? If not then I think you could just
remove it. I could see using this if you wanted to check that some assertion was
called n times.

https://codereview.chromium.org/1593443003/diff/1/tests/js/polyfills/storage....
tests/js/polyfills/storage.polyfill.test.js:413:
chrome.storage.onChanged.addListener(function(changes) {
On 2016/01/18 00:37:37, Matthew Alger wrote:
> On 2016/01/18 00:25:20, raymes wrote:
> > You could probably merge all the multiple handler tests into the single
> handler
> > ones - testing multiple would presumably cover the single case
> 
> I'm not sure that's entirely true. It's easily possible to implement this such
> that only one handler can be registered at a time, and having single/multiple
> split like this would reveal that.

I agree. But having only the multiple test would also catch that. The only thing
it wouldn't catch is if things worked with multiple handlers but not with a
single handler and it would be hard to write things in a way where that was the
case. But since they're already written, I'm ok leaving them.

[Matthew Alger, 2016-01-18 02:14:58]

https://codereview.chromium.org/1593443003/diff/1/tests/js/polyfills/storage....
File tests/js/polyfills/storage.polyfill.test.js (right):

https://codereview.chromium.org/1593443003/diff/1/tests/js/polyfills/storage....
tests/js/polyfills/storage.polyfill.test.js:336: assert.expect(0);
On 2016/01/18 02:13:34, raymes wrote:
> On 2016/01/18 00:37:37, Matthew Alger wrote:
> > On 2016/01/18 00:25:20, raymes wrote:
> > > What's this for?
> > 
> > expect tells QUnit how many assertions there will be in the test. This tells
> > QUnit we don't expect any assertions - we just expect the handlers to be
> called.
> 
> I see - so do things fail without this? If not then I think you could just
> remove it. I could see using this if you wanted to check that some assertion
was
> called n times.

Yes, QUnit will explicitly fail if there are no assertions. expect(0) is
something of a special case.

https://codereview.chromium.org/1593443003/diff/1/tests/js/polyfills/storage....
tests/js/polyfills/storage.polyfill.test.js:413:
chrome.storage.onChanged.addListener(function(changes) {
On 2016/01/18 02:13:34, raymes wrote:
> On 2016/01/18 00:37:37, Matthew Alger wrote:
> > On 2016/01/18 00:25:20, raymes wrote:
> > > You could probably merge all the multiple handler tests into the single
> > handler
> > > ones - testing multiple would presumably cover the single case
> > 
> > I'm not sure that's entirely true. It's easily possible to implement this
such
> > that only one handler can be registered at a time, and having
single/multiple
> > split like this would reveal that.
> 
> I agree. But having only the multiple test would also catch that. The only
thing
> it wouldn't catch is if things worked with multiple handlers but not with a
> single handler and it would be hard to write things in a way where that was
the
> case. But since they're already written, I'm ok leaving them. 

Acknowledged.

[Matthew Alger, 2016-01-18 02:15:38]

Description was changed from

==========
Finished storage polyfill.

Included in this CL are all the other functions of the storage polyfill
that were split out of the last storage polyfill CL, including set,
clear, and remove, as well as a function exposed only for testing.
==========

to

==========
Finished storage polyfill.

Included in this CL are all the other functions of the storage polyfill
that were split out of the last storage polyfill CL, including set,
clear, and remove, as well as a function exposed only for testing.

R=raymes@chromium.org

Committed: c1ac20bcf0bea9c8b0a4229e8cf7c4ae6f8733c3
==========

[Matthew Alger, 2016-01-18 02:15:42]

Committed patchset #3 (id:40001) manually as
c1ac20bcf0bea9c8b0a4229e8cf7c4ae6f8733c3 (presubmit successful).