Convert now component to use GCM rather than pushMessaging.

Convert now component to use GCM rather than pushMessaging.

BUG=447455
Committed: https://crrev.com/af1b985990b34d81640417006d9f9e71ebd2e9c5
Cr-Commit-Position: refs/heads/master@{#317450}

[skare_, 2015-02-17 23:29:53]

skare@chromium.org changed reviewers:
+ robliao@chromium.org

[skare_, 2015-02-17 23:34:15]

need to fix style today and/or tomorrow, and getting the server portion set up,
but here's the part of the client that gets the token if you have comments.

[robliao, 2015-02-18 00:04:25]

Thanks for this CL!

https://codereview.chromium.org/844503005/diff/80001/chrome/browser/resources...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/844503005/diff/80001/chrome/browser/resources...
chrome/browser/resources/google_now/background.js:1345: var subchannel =
message.data.subchannelId;
So we still need subchannel ID? I thought subchannel Ids were an artifact of
Chrome Push Messaging and aren't needed for Android style GCM. Maybe we can
transform this into some sort of tag instead.

If we still need this, keep this as 'var subchannelId' for consistency.

https://codereview.chromium.org/844503005/diff/80001/chrome/browser/resources...
chrome/browser/resources/google_now/background.js:1355: if
(items.lastPollNowPayloads[message.subchannelId] != payload) {
message.subchannelId -> subchannelId
Two more instances below.

https://codereview.chromium.org/844503005/diff/80001/chrome/browser/resources...
File chrome/browser/resources/google_now/utility.js (right):

https://codereview.chromium.org/844503005/diff/80001/chrome/browser/resources...
chrome/browser/resources/google_now/utility.js:1098: reject();
I think this can be written as...

fillFromChromeLocalStorage({gcmNotificationKey: undefined})
    .then(function(items) {
      if (items.gcmNotificationKey) {
        console.log('Reused GCM Key from Storage');
        return Promise.resolve(items.gcmNotificationKey);
      }

      return requestNewGcmNotificationKey();
    }

https://codereview.chromium.org/844503005/diff/80001/chrome/browser/resources...
chrome/browser/resources/google_now/utility.js:1109: function getUsername() {
Would it make sense to have the authentication manager handle this and return
the login?

https://codereview.chromium.org/844503005/diff/80001/chrome/browser/resources...
chrome/browser/resources/google_now/utility.js:1167: .then(function(items) {
OPTIONAL, since it doesn't really buy that much: Similar restructuring here,
except moving the Promise to capture the async call.

.then(function(items) {
  if (items.gcmRegistrationId) {
    console.log('Reused GCM Registration ID from storage.');
    return Promise.resolve(items.gcmRegistrationId);
  }

  return new Promise(function(resolve, reject) {
    chrome.gcm.register([GCM_PROJECT_ID], function(registrationId) {
      console.log('gcm.register(): ' + registrationId);
      if (registrationId) {
        chrome.storage.local.set({gcmRegistrationId: registrationId});
        resolve(registrationId);
      } else {
        reject();
      }
    });
  });
}

[skare_, 2015-02-19 16:46:01]

Patchset #9 (id:160001) has been deleted

[skare_, 2015-02-19 16:46:12]

Patchset #6 (id:100001) has been deleted

[skare_, 2015-02-19 16:48:10]

moved changes from utility into background since utility didn't need these
items.

still updating tests. just sending a checkpoint to address comments due to the
move.

https://codereview.chromium.org/844503005/diff/80001/chrome/browser/resources...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/844503005/diff/80001/chrome/browser/resources...
chrome/browser/resources/google_now/background.js:1345: var subchannel =
message.data.subchannelId;
On 2015/02/18 00:04:25, robliao wrote:
> So we still need subchannel ID? I thought subchannel Ids were an artifact of
> Chrome Push Messaging and aren't needed for Android style GCM. Maybe we can
> transform this into some sort of tag instead.
> 
> If we still need this, keep this as 'var subchannelId' for consistency.

made this 'tag' - I also don't think we'll need it, could remove if desired. let
me know.

https://codereview.chromium.org/844503005/diff/80001/chrome/browser/resources...
chrome/browser/resources/google_now/background.js:1355: if
(items.lastPollNowPayloads[message.subchannelId] != payload) {
On 2015/02/18 00:04:25, robliao wrote:
> message.subchannelId -> subchannelId
> Two more instances below.

Done.

https://codereview.chromium.org/844503005/diff/80001/chrome/browser/resources...
File chrome/browser/resources/google_now/utility.js (right):

https://codereview.chromium.org/844503005/diff/80001/chrome/browser/resources...
chrome/browser/resources/google_now/utility.js:1098: reject();
On 2015/02/18 00:04:25, robliao wrote:
> I think this can be written as...
> 
> fillFromChromeLocalStorage({gcmNotificationKey: undefined})
>     .then(function(items) {
>       if (items.gcmNotificationKey) {
>         console.log('Reused GCM Key from Storage');
>         return Promise.resolve(items.gcmNotificationKey);
>       }
> 
>       return requestNewGcmNotificationKey();
>     }

yes it can. thanks, done.

https://codereview.chromium.org/844503005/diff/80001/chrome/browser/resources...
chrome/browser/resources/google_now/utility.js:1109: function getUsername() {
On 2015/02/18 00:04:25, robliao wrote:
> Would it make sense to have the authentication manager handle this and return
> the login?

Done.

https://codereview.chromium.org/844503005/diff/80001/chrome/browser/resources...
chrome/browser/resources/google_now/utility.js:1167: .then(function(items) {
On 2015/02/18 00:04:25, robliao wrote:
> OPTIONAL, since it doesn't really buy that much: Similar restructuring here,
> except moving the Promise to capture the async call.
> 
> .then(function(items) {
>   if (items.gcmRegistrationId) {
>     console.log('Reused GCM Registration ID from storage.');
>     return Promise.resolve(items.gcmRegistrationId);
>   }
> 
>   return new Promise(function(resolve, reject) {
>     chrome.gcm.register([GCM_PROJECT_ID], function(registrationId) {
>       console.log('gcm.register(): ' + registrationId);
>       if (registrationId) {
>         chrome.storage.local.set({gcmRegistrationId: registrationId});
>         resolve(registrationId);
>       } else {
>         reject();
>       }
>     });
>   });
> }

Done. Gets rid of an else and some spaces which is nice.

[robliao, 2015-02-19 18:29:21]

Overall lgtm. Just a few nits and a question.

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

https://codereview.chromium.org/844503005/diff/180001/chrome/browser/resource...
chrome/browser/resources/google_now/background.js:1212:
getGcmNotificationKey().then(function(unusedKey) {});
The then portion should be unnecessary. EMCAScript promises call the promise's
function on initialization.

See step 8.

25.4.3.1.1
The abstract operation InitialisePromise initialises a newly allocated promise
object using an executor function.
1. Assert: promise has a have a [[PromiseStatus]] internal slot and it’s value
is undefined.
2. Assert: IsCallable(executor) is true.
3. Set promise's [[PromiseStatus]] internal slot to "unresolved".
4. Set promise's [[PromiseResolveReactions]] internal slot to a new empty List.
5. Set promise's [[PromiseRejectReactions]] internal slot to a new empty List.
6. Let resolve be CreateResolveFunction(promise).
7. Let reject be CreateRejectFunction(promise).
8. Let completion be the result of calling the [[Call]] internal method of
executor with undefined as thisArgument and (resolve, reject) as argumentsList.
9. If completion is an abrupt completion, then
a. Let status be the result of calling the [[Call]] internal method of reject
with undefined as thisArgument and (completion.[[value]]) as argumentsList.
b. ReturnIfAbrupt(status).
10. Return promise.

https://codereview.chromium.org/844503005/diff/180001/chrome/browser/resource...
chrome/browser/resources/google_now/background.js:1224: console.log('reused gcm
key from storage.');
Nit: Capitalize reused.

https://codereview.chromium.org/844503005/diff/180001/chrome/browser/resource...
chrome/browser/resources/google_now/background.js:1242: xhr.responseType =
'text';
Does application/json not work for the responseType?

https://codereview.chromium.org/844503005/diff/180001/chrome/browser/resource...
chrome/browser/resources/google_now/background.js:1283: console.log('reused gcm
registration id from storage.');
Nit: Capitalize reused.

[skare_, 2015-02-20 20:53:47]

Patchset #8 (id:180001) has been deleted

[skare_, 2015-02-20 21:00:47]

New patchsets have been uploaded after l-g-t-m from robliao@chromium.org

[skare_, 2015-02-20 21:53:47]

I moved some things from utility.js to background.js and modified test. The
responseType= text=>json is outstanding. Will look into it but sending this out
again since some things changed.

On 2015/02/19 18:29:21, robliao wrote:
> Overall lgtm. Just a few nits and a question.
> 
>
https://codereview.chromium.org/844503005/diff/180001/chrome/browser/resource...
> File chrome/browser/resources/google_now/background.js (right):
> 
>
https://codereview.chromium.org/844503005/diff/180001/chrome/browser/resource...
> chrome/browser/resources/google_now/background.js:1212:
> getGcmNotificationKey().then(function(unusedKey) {});
> The then portion should be unnecessary. EMCAScript promises call the promise's
> function on initialization.
> 
> See step 8.
> 
> 25.4.3.1.1
> The abstract operation InitialisePromise initialises a newly allocated promise
> object using an executor function.
> 1. Assert: promise has a have a [[PromiseStatus]] internal slot and it’s value
> is undefined.
> 2. Assert: IsCallable(executor) is true.
> 3. Set promise's [[PromiseStatus]] internal slot to "unresolved".
> 4. Set promise's [[PromiseResolveReactions]] internal slot to a new empty
List.
> 5. Set promise's [[PromiseRejectReactions]] internal slot to a new empty List.
> 6. Let resolve be CreateResolveFunction(promise).
> 7. Let reject be CreateRejectFunction(promise).
> 8. Let completion be the result of calling the [[Call]] internal method of
> executor with undefined as thisArgument and (resolve, reject) as
argumentsList.
> 9. If completion is an abrupt completion, then
> a. Let status be the result of calling the [[Call]] internal method of reject
> with undefined as thisArgument and (completion.[[value]]) as argumentsList.
> b. ReturnIfAbrupt(status).
> 10. Return promise.
done, thanks!
> 
>
https://codereview.chromium.org/844503005/diff/180001/chrome/browser/resource...
> chrome/browser/resources/google_now/background.js:1224: console.log('reused
gcm
> key from storage.');
> Nit: Capitalize reused.
done
> 
>
https://codereview.chromium.org/844503005/diff/180001/chrome/browser/resource...
> chrome/browser/resources/google_now/background.js:1242: xhr.responseType =
> 'text';
> Does application/json not work for the responseType?
discussed -- will try. 
> 
>
https://codereview.chromium.org/844503005/diff/180001/chrome/browser/resource...
> chrome/browser/resources/google_now/background.js:1283: console.log('reused
gcm
> registration id from storage.');
> Nit: Capitalize reused.
done

[robliao, 2015-02-20 22:04:23]

lgtm

[skare_, 2015-02-20 23:09:50]

New patchsets have been uploaded after l-g-t-m from robliao@chromium.org

[skare_, 2015-02-20 23:12:13]

The CQ bit was checked by skare@chromium.org

[robliao, 2015-02-20 23:12:21]

Still good.

[commit-bot: I haz the power, 2015-02-20 23:13:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/844503005/240001

[commit-bot: I haz the power, 2015-02-21 00:44:00]

Committed patchset #10 (id:240001)

[commit-bot: I haz the power, 2015-02-21 00:44:39]

Patchset 10 (id:??) landed as
https://crrev.com/af1b985990b34d81640417006d9f9e71ebd2e9c5
Cr-Commit-Position: refs/heads/master@{#317450}