Convert Google Now's use of chrome.local.storage.get to use Promises

Convert Google Now's use of chrome.storage.local.get to use Promises

BUG=164227
R=rgustafson@chromium.org, skare@chromium.org, vadimt@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=254542

[skare_, 2014-02-20 00:17:12]

https://codereview.chromium.org/166033010/diff/1/chrome/browser/resources/goo...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/166033010/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/background.js:1085: console.log(results);
temporary?

https://codereview.chromium.org/166033010/diff/1/chrome/browser/resources/goo...
File chrome/browser/resources/google_now/utility.js (right):

https://codereview.chromium.org/166033010/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/utility.js:465: /*
nit: /* -> /** and in :472 
(sorry!)

https://codereview.chromium.org/166033010/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/utility.js:470: var NO_PROMISE_REJECTION =
false;
(no action required)
this works; could make an @enum {boolean} too

https://codereview.chromium.org/166033010/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/utility.js:480: * Provides the promise
equivalent of instrumented.chrome.local.get.
instrumented.chrome.storage.local?

https://codereview.chromium.org/166033010/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/utility.js:485: * @return {Promise} A
promise that fills the default storage object. On
just as a first-read opinion, "fills" is slightly confusing but that might just
be me. It's basically "get from" or "read from" right? (those aren't great
suggestions because they might make a couple things wordy elsewhere)

https://codereview.chromium.org/166033010/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/utility.js:486: *     failure, if promise
rejection is a allowed, the promise is rejected,
nit: is a allowed -> is allowed

[robliao, 2014-02-21 21:25:45]

https://codereview.chromium.org/166033010/diff/1/chrome/browser/resources/goo...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/166033010/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/background.js:1085: console.log(results);
Yup! Done.
On 2014/02/20 00:17:12, Travis Skare wrote:
> temporary?

https://codereview.chromium.org/166033010/diff/1/chrome/browser/resources/goo...
File chrome/browser/resources/google_now/utility.js (right):

https://codereview.chromium.org/166033010/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/utility.js:465: /*
On 2014/02/20 00:17:12, Travis Skare wrote:
> nit: /* -> /** and in :472 
> (sorry!)

Done.

https://codereview.chromium.org/166033010/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/utility.js:470: var NO_PROMISE_REJECTION =
false;
On 2014/02/20 00:17:12, Travis Skare wrote:
> (no action required)
> this works; could make an @enum {boolean} too

Done.

https://codereview.chromium.org/166033010/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/utility.js:480: * Provides the promise
equivalent of instrumented.chrome.local.get.
On 2014/02/20 00:17:12, Travis Skare wrote:
> instrumented.chrome.storage.local?

Done.

https://codereview.chromium.org/166033010/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/utility.js:485: * @return {Promise} A
promise that fills the default storage object. On
I'm open to other suggestions.

I chose fill because we are filling the object in, replacing default values with
the stored values.

get didn't work because we return the default object if anything failed.
On 2014/02/20 00:17:12, Travis Skare wrote:
> just as a first-read opinion, "fills" is slightly confusing but that might
just
> be me. It's basically "get from" or "read from" right? (those aren't great
> suggestions because they might make a couple things wordy elsewhere)

https://codereview.chromium.org/166033010/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/utility.js:486: *     failure, if promise
rejection is a allowed, the promise is rejected,
On 2014/02/20 00:17:12, Travis Skare wrote:
> nit: is a allowed -> is allowed

Done.

[robliao, 2014-02-24 18:31:42]

Rebase to r252941

[skare_, 2014-02-27 18:46:13]

lgtm

[robliao, 2014-02-27 18:46:53]

vadimt: Please provide owner approval of this CL.

[vadimt, 2014-02-27 19:07:24]

On 2014/02/27 18:46:53, robliao wrote:
> vadimt: Please provide owner approval of this CL.

Will rgustafson@ review this? If so, I'll better wait.

[robliao, 2014-02-27 19:08:09]

On 2014/02/27 19:07:24, vadimt wrote:
> On 2014/02/27 18:46:53, robliao wrote:
> > vadimt: Please provide owner approval of this CL.
> 
> Will rgustafson@ review this? If so, I'll better wait.

We don't need to wait. She has other server work to do as it is.

[robliao, 2014-02-27 19:50:07]

Adding NOPRESUBMIT due to http://crbug.com/338301

** Presubmit Warnings **
Found JavaScript style violations in
chrome\browser\resources\google_now\background.js:
  line 275: E0002: Missing space before "("
  }).catch(function() {
          ^

[robliao, 2014-02-27 19:50:11]

The CQ bit was checked by robliao@chromium.org

[commit-bot: I haz the power, 2014-02-27 19:50:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-27 19:50:50]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[vadimt, 2014-02-27 20:20:36]

https://codereview.chromium.org/166033010/diff/380001/chrome/browser/resource...
File chrome/browser/resources/google_now/utility.js (right):

https://codereview.chromium.org/166033010/diff/380001/chrome/browser/resource...
chrome/browser/resources/google_now/utility.js:652: } else if
(opt_allowPromiseRejection === PromiseRejection.ALLOW) {
Why not "if (opt_allowPromiseRejection)"?

https://codereview.chromium.org/166033010/diff/380001/chrome/browser/resource...
chrome/browser/resources/google_now/utility.js:903:
request[currentDelayStorageKey] = maximumDelaySeconds;
This changes the semantics. If items retrieved, but don't contains
currentDelayStorageKey, the old semantics was to leave it undefined, and the new
one is to assign maximumDelaySeconds.
Is this the intent?

[rgustafson, 2014-02-27 21:32:08]

https://codereview.chromium.org/166033010/diff/250001/chrome/browser/resource...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/166033010/diff/250001/chrome/browser/resource...
chrome/browser/resources/google_now/background.js:878: /** @type
{NotificationDataEntry} */
Object.<ChromeNotificationId or string,NotificationDataEntry>? 

There seems to be a disconnect between where ChromeNotificationId and string are
used. I'd prefer to use ChromeNotificationId where the chromeNotificationId is
actually used so there's less confusion between that and notificationId. (I know
this wasn't introduced by this change, but it highlighted it)

https://codereview.chromium.org/166033010/diff/380001/chrome/browser/resource...
File chrome/browser/resources/google_now/utility.js (right):

https://codereview.chromium.org/166033010/diff/380001/chrome/browser/resource...
chrome/browser/resources/google_now/utility.js:638: * @param {boolean}
opt_allowPromiseRejection If PromiseRejection.ALLOW,
document param as optional

[robliao, 2014-02-27 23:52:36]

Updates and it turns out NOPRESUBMIT doesn't work...

https://codereview.chromium.org/166033010/diff/250001/chrome/browser/resource...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/166033010/diff/250001/chrome/browser/resource...
chrome/browser/resources/google_now/background.js:878: /** @type
{NotificationDataEntry} */
Noted, and that clarification should really be done in another change instead of
hidden in this one.

Updated the type.

On 2014/02/27 21:32:09, rgustafson wrote:
> Object.<ChromeNotificationId or string,NotificationDataEntry>? 
> 
> There seems to be a disconnect between where ChromeNotificationId and string
are
> used. I'd prefer to use ChromeNotificationId where the chromeNotificationId is
> actually used so there's less confusion between that and notificationId. (I
know
> this wasn't introduced by this change, but it highlighted it)

https://codereview.chromium.org/166033010/diff/380001/chrome/browser/resource...
File chrome/browser/resources/google_now/utility.js (right):

https://codereview.chromium.org/166033010/diff/380001/chrome/browser/resource...
chrome/browser/resources/google_now/utility.js:638: * @param {boolean}
opt_allowPromiseRejection If PromiseRejection.ALLOW,
On 2014/02/27 21:32:10, rgustafson wrote:
> document param as optional

Done.

https://codereview.chromium.org/166033010/diff/380001/chrome/browser/resource...
chrome/browser/resources/google_now/utility.js:652: } else if
(opt_allowPromiseRejection === PromiseRejection.ALLOW) {
We're comparing against an enum, so we should do an equality comparison.
On 2014/02/27 20:20:36, vadimt wrote:
> Why not "if (opt_allowPromiseRejection)"?

https://codereview.chromium.org/166033010/diff/380001/chrome/browser/resource...
chrome/browser/resources/google_now/utility.js:903:
request[currentDelayStorageKey] = maximumDelaySeconds;
Gotcha. Fixed.
On 2014/02/27 20:20:36, vadimt wrote:
> This changes the semantics. If items retrieved, but don't contains
> currentDelayStorageKey, the old semantics was to leave it undefined, and the
new
> one is to assign maximumDelaySeconds.
> Is this the intent?

[vadimt, 2014-02-28 20:21:04]

https://codereview.chromium.org/166033010/diff/380001/chrome/browser/resource...
File chrome/browser/resources/google_now/utility.js (right):

https://codereview.chromium.org/166033010/diff/380001/chrome/browser/resource...
chrome/browser/resources/google_now/utility.js:652: } else if
(opt_allowPromiseRejection === PromiseRejection.ALLOW) {
On 2014/02/27 23:52:36, robliao wrote:
> We're comparing against an enum, so we should do an equality comparison.
> On 2014/02/27 20:20:36, vadimt wrote:
> > Why not "if (opt_allowPromiseRejection)"?
> 

You don't use the fact that enum values are booleans, which is confusing. I
believe you should either:
1. Define the constants as 0 and 1
OR
2. if (opt_allowPromiseRejection)

Your choice.

https://codereview.chromium.org/166033010/diff/400001/chrome/browser/resource...
File chrome/browser/resources/google_now/utility.js (right):

https://codereview.chromium.org/166033010/diff/400001/chrome/browser/resource...
chrome/browser/resources/google_now/utility.js:638: * @param {boolean=}
opt_allowPromiseRejection If PromiseRejection.ALLOW,
Should be {PromiseRejection=}

[robliao, 2014-02-28 20:39:48]

https://codereview.chromium.org/166033010/diff/380001/chrome/browser/resource...
File chrome/browser/resources/google_now/utility.js (right):

https://codereview.chromium.org/166033010/diff/380001/chrome/browser/resource...
chrome/browser/resources/google_now/utility.js:652: } else if
(opt_allowPromiseRejection === PromiseRejection.ALLOW) {
We can use the number, but for enums it shouldn't matter what type they are.
They are enums and comparisons should be done against them to respect the
abstraction.
On 2014/02/28 20:21:05, vadimt wrote:
> On 2014/02/27 23:52:36, robliao wrote:
> > We're comparing against an enum, so we should do an equality comparison.
> > On 2014/02/27 20:20:36, vadimt wrote:
> > > Why not "if (opt_allowPromiseRejection)"?
> > 
> 
> You don't use the fact that enum values are booleans, which is confusing. I
> believe you should either:
> 1. Define the constants as 0 and 1
> OR
> 2. if (opt_allowPromiseRejection)
> 
> Your choice.

https://codereview.chromium.org/166033010/diff/400001/chrome/browser/resource...
File chrome/browser/resources/google_now/utility.js (right):

https://codereview.chromium.org/166033010/diff/400001/chrome/browser/resource...
chrome/browser/resources/google_now/utility.js:638: * @param {boolean=}
opt_allowPromiseRejection If PromiseRejection.ALLOW,
On 2014/02/28 20:21:05, vadimt wrote:
> Should be {PromiseRejection=}

Done.

[vadimt, 2014-02-28 21:39:08]

lgtm

[rgustafson, 2014-02-28 22:15:57]

lgtm

[robliao, 2014-03-03 19:59:11]

Committed patchset #6 manually as r254542 (presubmit successful).