Changed identity API to use promises instead of callbacks.

Changed identity API to use promises instead of callbacks.

BUG=

Committed: https://crrev.com/cbc052cd81ae44e6adafb77e21f7fe98ef417c90
Cr-Commit-Position: refs/heads/master@{#318134}

[John Williams, 2015-02-18 00:45:54]

jrw@chromium.org changed reviewers:
+ jamiewalch@chromium.org, rmsousa@chromium.org, sergeyu@chromium.org

[John Williams, 2015-02-18 00:45:54]

My attempt at properly converting the remoting identity API to use promises. 
The main file to look at is identity.js; all the other changes are a consequence
of what I did there.

[Jamie, 2015-02-18 01:03:03]

jamiewalch@chromium.org changed reviewers:
+ kelvinp@chromium.org

[Jamie, 2015-02-18 01:03:03]

When specifying multiple reviewers, please be explicit about who you're asking
to review each component. In this case, I think Kelvin is actually the best
candidate; Kelvin, can you take a look?

[kelvinp, 2015-02-18 01:13:01]

On 2015/02/18 01:03:03, Jamie wrote:
> When specifying multiple reviewers, please be explicit about who you're asking
> to review each component. In this case, I think Kelvin is actually the best
> candidate; Kelvin, can you take a look?

Sure, I can take a look

[kelvinp, 2015-02-18 19:45:25]

Thank you for doing the change.
Mostly look good, just a suggestion on using base.Deferred instead

https://codereview.chromium.org/937593002/diff/1/remoting/webapp/crd/js/ident...
File remoting/webapp/crd/js/identity.js (right):

https://codereview.chromium.org/937593002/diff/1/remoting/webapp/crd/js/ident...
remoting/webapp/crd/js/identity.js:64: this.pendingPromise_ = new
Promise(function(
I think base.Deferred will work better as you want to return a promise in one
function but resolves/rejects it in other. 

var that = this;
if (!this.getAuthTokenDeferred_) {
  this.getAuthTokenDeferred_ = new base.Deferred();
  chrome.identity.getAuthToken({ interactive: false },
                               that.onAuthComplete.bind(that, false)) 
}

return this.getAuthTokenDeferred_.promise();
... 

In onAuthComplete
getAuthTokenDeferred_.resolves(token) or
getAuthTokenDeferred_.reject(error)

https://codereview.chromium.org/937593002/diff/1/remoting/webapp/crd/js/ident...
remoting/webapp/crd/js/identity.js:65: /** function(string) */ resolve,
Is this a new syntax of annotating parameters? I like it as it seems to be
shorter. However, we use @param elsewhere.  Let's talk about it and we should be
consistent throughout the codebase.

https://codereview.chromium.org/937593002/diff/1/remoting/webapp/crd/js/ident...
remoting/webapp/crd/js/identity.js:76: * Get a fresh access token.
Nit: Gets

https://codereview.chromium.org/937593002/diff/1/remoting/webapp/crd/js/ident...
remoting/webapp/crd/js/identity.js:78: * @return {!Promise<string>} A promise
resolved the an access token or
s/resolved the an/resolved with an/

https://codereview.chromium.org/937593002/diff/1/remoting/webapp/crd/js/ident...
remoting/webapp/crd/js/identity.js:122: * @return {!Promise<{email: string,
name: string}>} Promise
no space between : and type name

https://codereview.chromium.org/937593002/diff/1/remoting/webapp/crd/js/ident...
remoting/webapp/crd/js/identity.js:201: resolve, reject, interactive, token) {
As commented earlier, use base.Deferred instead

https://codereview.chromium.org/937593002/diff/1/remoting/webapp/crd/js/ident...
remoting/webapp/crd/js/identity.js:228: that, resolve, reject, true));
indentation

https://codereview.chromium.org/937593002/diff/1/remoting/webapp/crd/js/oauth...
File remoting/webapp/crd/js/oauth2.js (right):

https://codereview.chromium.org/937593002/diff/1/remoting/webapp/crd/js/oauth...
remoting/webapp/crd/js/oauth2.js:394: return new Promise(function(resolve,
reject) {
Call into getUserInfo instead.

[Jamie, 2015-02-18 19:55:57]

On 2015/02/18 00:45:54, John Williams wrote:
> My attempt at properly converting the remoting identity API to use promises. 
> The main file to look at is identity.js; all the other changes are a
consequence
> of what I did there.

Can I add a request not to land this until after the branch point? It's very
close and this release is already going to involve major churn so I'd prefer not
to introduce any more unknowns at this stage.

[John Williams, 2015-02-18 20:28:04]

On 2015/02/18 19:55:57, Jamie wrote:
> On 2015/02/18 00:45:54, John Williams wrote:
> > My attempt at properly converting the remoting identity API to use promises.

> > The main file to look at is identity.js; all the other changes are a
> consequence
> > of what I did there.
> 
> Can I add a request not to land this until after the branch point? It's very
> close and this release is already going to involve major churn so I'd prefer
not
> to introduce any more unknowns at this stage.

Fine with me.

[John Williams, 2015-02-18 20:28:22]

https://codereview.chromium.org/937593002/diff/1/remoting/webapp/crd/js/ident...
File remoting/webapp/crd/js/identity.js (right):

https://codereview.chromium.org/937593002/diff/1/remoting/webapp/crd/js/ident...
remoting/webapp/crd/js/identity.js:64: this.pendingPromise_ = new
Promise(function(
On 2015/02/18 19:45:24, kelvinp wrote:
> I think base.Deferred will work better as you want to return a promise in one
> function but resolves/rejects it in other. 
> 
> var that = this;
> if (!this.getAuthTokenDeferred_) {
>   this.getAuthTokenDeferred_ = new base.Deferred();
>   chrome.identity.getAuthToken({ interactive: false },
>                                that.onAuthComplete.bind(that, false)) 
> }
> 
> return this.getAuthTokenDeferred_.promise();
> ... 
> 
> In onAuthComplete
> getAuthTokenDeferred_.resolves(token) or
> getAuthTokenDeferred_.reject(error)

Done.

https://codereview.chromium.org/937593002/diff/1/remoting/webapp/crd/js/ident...
remoting/webapp/crd/js/identity.js:65: /** function(string) */ resolve,
On 2015/02/18 19:45:24, kelvinp wrote:
> Is this a new syntax of annotating parameters? I like it as it seems to be
> shorter. However, we use @param elsewhere.  Let's talk about it and we should
be
> consistent throughout the codebase.

It's a recent addition to the syntax.  I like it for documenting parameter types
for functions that are nested inside another expression because the @param
syntax is very heavy and easy to get wrong.  The syntax used in a lot of the
codebase is actually wrong because when the doc comment doesn't immediately
precede the "function" keyword, the version of jscompiler in git just ignores it
silently.  Newer versions of jscompiler actually report this case as an error,
so when you use the correct syntax and indent according to standard rules, you
get this mess:

  new Promise(
      /**
       * @param {function(string)} resolve
       * @param {function(remoting.Error)} reject
       */
      function(resolve, reject) {
        ...
      });

Since the new syntax is shorter by about 2 lines, and less indented by 4
columns, I greatly prefer it.

As for consistency, I think the new syntax should be preferred when it avoids
excessive indentation, but the old form should be used elsewhere.  I don't think
it's worth changing existing uses except the ones that are actually broken.

https://codereview.chromium.org/937593002/diff/1/remoting/webapp/crd/js/ident...
remoting/webapp/crd/js/identity.js:76: * Get a fresh access token.
On 2015/02/18 19:45:24, kelvinp wrote:
> Nit: Gets

Done.

https://codereview.chromium.org/937593002/diff/1/remoting/webapp/crd/js/ident...
remoting/webapp/crd/js/identity.js:122: * @return {!Promise<{email: string,
name: string}>} Promise
On 2015/02/18 19:45:24, kelvinp wrote:
> no space between : and type name

Done.

https://codereview.chromium.org/937593002/diff/1/remoting/webapp/crd/js/ident...
remoting/webapp/crd/js/identity.js:201: resolve, reject, interactive, token) {
On 2015/02/18 19:45:24, kelvinp wrote:
> As commented earlier, use base.Deferred instead

Done.

https://codereview.chromium.org/937593002/diff/1/remoting/webapp/crd/js/ident...
remoting/webapp/crd/js/identity.js:228: that, resolve, reject, true));
On 2015/02/18 19:45:24, kelvinp wrote:
> indentation

Done.

[kelvinp, 2015-02-18 21:33:27]

lgtm

[John Williams, 2015-02-24 01:24:58]

The CQ bit was checked by jrw@chromium.org

[commit-bot: I haz the power, 2015-02-24 01:27:06]

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

[commit-bot: I haz the power, 2015-02-24 01:34:05]

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

[commit-bot: I haz the power, 2015-02-24 01:34:06]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[John Williams, 2015-02-24 03:45:28]

The CQ bit was checked by jrw@chromium.org

[John Williams, 2015-02-24 03:45:28]

The patchset sent to the CQ was uploaded after l-g-t-m from kelvinp@chromium.org
Link to the patchset: https://codereview.chromium.org/937593002/#ps40001 (title:
"Sync to head.")

[commit-bot: I haz the power, 2015-02-24 03:45:54]

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

[commit-bot: I haz the power, 2015-02-24 05:04:42]

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

[commit-bot: I haz the power, 2015-02-24 05:04:43]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[John Williams, 2015-02-25 21:50:30]

The CQ bit was checked by jrw@chromium.org

[John Williams, 2015-02-25 21:50:31]

The patchset sent to the CQ was uploaded after l-g-t-m from kelvinp@chromium.org
Link to the patchset: https://codereview.chromium.org/937593002/#ps60001 (title:
"fixed bad merge")

[John Williams, 2015-02-25 21:50:46]

The CQ bit was unchecked by jrw@chromium.org

[John Williams, 2015-02-25 21:50:47]

The CQ bit was checked by jrw@chromium.org

[commit-bot: I haz the power, 2015-02-25 21:50:51]

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

[commit-bot: I haz the power, 2015-02-25 22:57:08]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-25 22:58:06]

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