Improve HRD first run experience

Improve HRD first run experience

Summary of changes:
1. Implements a rich UI to prompt for the user's permission to
   accept remote assistance from a hangout participant
2. Move getUrlParameter() to base.js
3. Show an interactive auth prompt if the user doesn't have an
   OAuth2 token for starting the It2Me host in v2 app. For v1
   app, do an auth redirect.
4. Tear down the It2MeHelpee channel if the user decline the
   remote assistance.

BUG=451591
Committed: https://crrev.com/e24343ec0281dc35e64dd14d4ef214148658768d
Cr-Commit-Position: refs/heads/master@{#314405}

[kelvinp, 2015-01-31 20:29:24]

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

[kelvinp, 2015-01-31 20:51:21]

Patchset #1 (id:1) has been deleted

[kelvinp, 2015-01-31 21:02:27]

PTAL

[Jamie, 2015-02-02 19:07:41]

https://codereview.chromium.org/888323002/diff/20001/remoting/remoting_webapp...
File remoting/remoting_webapp_files.gypi (right):

https://codereview.chromium.org/888323002/diff/20001/remoting/remoting_webapp...
remoting/remoting_webapp_files.gypi:319:
'webapp/crd/resources/icon_hangout.webp',
I think we need to check this in to third_party. Lambros has just gone through a
similar process for some other icons (and learned that they *didn't* need to go
into third_party) so it might depend on the licensing. He might be able to help
you decide.

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/html...
File remoting/webapp/crd/html/dialog_hangout_consent.css (right):

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/html...
remoting/webapp/crd/html/dialog_hangout_consent.css:41: }
Blank line here, please.

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/html...
remoting/webapp/crd/html/dialog_hangout_consent.css:80: }
And here.

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/html...
remoting/webapp/crd/html/dialog_hangout_consent.css:104: display: none
!important;
Do you need this? I don't see any other rules that affect display in this CSS
file, and if this is needed as a result of a rule in some other CSS file, then I
think it belongs there.

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/html...
File remoting/webapp/crd/html/dialog_hangout_consent.html (right):

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/html...
remoting/webapp/crd/html/dialog_hangout_consent.html:11: <link rel="icon"
type="image/png" href="chromoting16.webp">
Is this the right icon (ie, should this window be branded Chromoting or
Hangounts)?

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/h...
File remoting/webapp/crd/js/hangout_consent_dialog.js (right):

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/hangout_consent_dialog.js:16: var AUTHENTICATED_MARGIN =
100;
Can these constants be computed (eg, using getBoundingClientRect) rather than
being hard-coded? Hard-coding is risky, especially for localizable content
(which I assume this is). For window position, I think it's fine if you center
it assuming US English content (the worst that can happen is it's off-center
vertically by a small amount), but for anything more visually important, you
should do size-based calculations in the dialog itself, where you can measure
things precisely.

If you can get rid of them, you could consider getting rid of the strings as
well, since you're only referring to them once, and I'm not sure there's much
point in making them variables.

If you do want to keep them, they should probably be local variables inside
show(), since that's the only place they're used.

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/hangout_consent_dialog.js:39: * @param {boolean} confirm
whether the user authorize the it2me connection
s/whether/Whether/ (also, capitalize the first word of the parameter description
elsewhere in this file).

s/authorize/authorized/

Full-stop at the end of the sentence, please.

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/hangout_consent_dialog.js:43: if
(this.onConsentResponseDeferred_) {
Under what circumstances can this be null? Would an assert be a better option
here?

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/hangout_consent_dialog.js:55: * @param {boolean}
authenticated if set, the consent dialog will hide the
s/set/true/

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/hangout_consent_dialog.js:56: *  authorization section.
Authentication and authorization have very specific, and different, meanings, so
documenting an |authenticated| parameter as affecting an "authorization" section
is confusing. Pick whichever is more appropriate and be consistent.

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/hangout_consent_dialog.js:58: *  upon |opt_parentBounds|.
s/upon/within/

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/hangout_consent_dialog.js:64: if
(!this.onConsentResponseDeferred_) {
I think this needs to be an assert. Returning the same Promise for two separate
calls will (I think) leave one of them hanging, because the Promise will only
resolve once.

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/hangout_consent_dialog.js:70: outerBounds: {width:
DIALOG_WIDTH, height: height}
Space after '{' and before '}' (the only time we don't do this is in JSDoc type
annotations).

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/hangout_consent_dialog.js:74: DIALOG_URL + '?' +
AUTHENTICATED_PARAM + '=' + String(!!authenticated);
I think Boolean(authenticated) would be clearer than String(!!authenticated).

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/h...
File remoting/webapp/crd/js/hangout_consent_dialog_main.js (right):

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/hangout_consent_dialog_main.js:55:
document.addEventListener("DOMContentLoaded", onDomContentLoaded);
Single-quotes for JS strings, please.

Elsewhere, we have just used "load". I'm not sure whether it makes any
difference here (one potential problem that springs to mind is that if you
switch to measuring the DOM rather than hard-coding the dialog dimensions, there
might be a race-condition if you use DOMContentLoaded, since you might try to
measure the DOM before the CSS is loaded).

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/i...
File remoting/webapp/crd/js/it2me_helpee_channel.js (right):

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/i...
remoting/webapp/crd/js/it2me_helpee_channel.js:253: var bounds = /** @type
{Bounds} */ (message['parentBounds']);
getObjectAttr would be better (it does some minimal type-checking).

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/i...
remoting/webapp/crd/js/it2me_helpee_channel.js:269: /** @type {function(*):void}
*/(this.connectToHost_.bind(this, email))
Why does this call need a cast, but the others don't? Maybe this one can be
removed now that we're using a more modern compiler?

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/i...
remoting/webapp/crd/js/it2me_helpee_channel.js:317: return Promise.reject(new
Error(remoting.Error.NOT_AUTHORIZED));
I think CANCELLED is a better error here. Generally, if the user explicitly
declines something (CANCELLED), then no error should be displayed. If some
operation failed because the user doesn't have permission to do it
(NOT_AUTHORIZED) then we should show an error. Even if this code-path doesn't
touch any common error handling code where such logic might be implemented, it's
better to be consistent.

[kelvinp, 2015-02-03 01:11:55]

Patchset #2 (id:40001) has been deleted

[kelvinp, 2015-02-03 01:12:40]

Patchset #2 (id:40001) has been deleted

[kelvinp, 2015-02-03 01:15:33]

PTAL

https://codereview.chromium.org/888323002/diff/20001/remoting/remoting_webapp...
File remoting/remoting_webapp_files.gypi (right):

https://codereview.chromium.org/888323002/diff/20001/remoting/remoting_webapp...
remoting/remoting_webapp_files.gypi:319:
'webapp/crd/resources/icon_hangout.webp',
On 2015/02/02 19:07:39, Jamie wrote:
> I think we need to check this in to third_party. Lambros has just gone through
a
> similar process for some other icons (and learned that they *didn't* need to
go
> into third_party) so it might depend on the licensing. He might be able to
help
> you decide.

Acknowledged. Using the CRD icon for now.  Will use the correct icon when
Lambros setup the internal resources repository.

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/html...
File remoting/webapp/crd/html/dialog_hangout_consent.css (right):

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/html...
remoting/webapp/crd/html/dialog_hangout_consent.css:41: }
On 2015/02/02 19:07:40, Jamie wrote:
> Blank line here, please.

Done.

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/html...
remoting/webapp/crd/html/dialog_hangout_consent.css:80: }
On 2015/02/02 19:07:40, Jamie wrote:
> And here.

Done.

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/html...
remoting/webapp/crd/html/dialog_hangout_consent.css:104: display: none
!important;
On 2015/02/02 19:07:39, Jamie wrote:
> Do you need this? I don't see any other rules that affect display in this CSS
> file, and if this is needed as a result of a rule in some other CSS file, then
I
> think it belongs there.

This is how the CRD app hides elements using JavaScript.
In JavaScript, we set element.hidden = true, this css rule picks up the
attribute change and set display:none on the element.

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/html...
File remoting/webapp/crd/html/dialog_hangout_consent.html (right):

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/html...
remoting/webapp/crd/html/dialog_hangout_consent.html:11: <link rel="icon"
type="image/png" href="chromoting16.webp">
On 2015/02/02 19:07:40, Jamie wrote:
> Is this the right icon (ie, should this window be branded Chromoting or
> Hangounts)?

Removed as we create the window with frame:none.  The icon won't be shown to the
users anyways.

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/h...
File remoting/webapp/crd/js/hangout_consent_dialog.js (right):

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/hangout_consent_dialog.js:16: var AUTHENTICATED_MARGIN =
100;
On 2015/02/02 19:07:40, Jamie wrote:
> Can these constants be computed (eg, using getBoundingClientRect) rather than
> being hard-coded? Hard-coding is risky, especially for localizable content
> (which I assume this is). For window position, I think it's fine if you center
> it assuming US English content (the worst that can happen is it's off-center
> vertically by a small amount), but for anything more visually important, you
> should do size-based calculations in the dialog itself, where you can measure
> things precisely.
> 
> If you can get rid of them, you could consider getting rid of the strings as
> well, since you're only referring to them once, and I'm not sure there's much
> point in making them variables.
> 
> If you do want to keep them, they should probably be local variables inside
> show(), since that's the only place they're used.

Good point. In the new iteration, I added a measuring pass when the page is
loaded to ensure the height is big enough such that contents won't be clipped.

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/hangout_consent_dialog.js:39: * @param {boolean} confirm
whether the user authorize the it2me connection
On 2015/02/02 19:07:40, Jamie wrote:
> s/whether/Whether/ (also, capitalize the first word of the parameter
description
> elsewhere in this file).
> 
> s/authorize/authorized/
> 
> Full-stop at the end of the sentence, please.

Done.

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/hangout_consent_dialog.js:43: if
(this.onConsentResponseDeferred_) {
On 2015/02/02 19:07:40, Jamie wrote:
> Under what circumstances can this be null? Would an assert be a better option
> here?

I am trying guard against invalid IPC's.  I think an Xsrf token is probably a
better solution.

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/hangout_consent_dialog.js:55: * @param {boolean}
authenticated if set, the consent dialog will hide the
On 2015/02/02 19:07:40, Jamie wrote:
> s/set/true/

Done.

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/hangout_consent_dialog.js:56: *  authorization section.
On 2015/02/02 19:07:40, Jamie wrote:
> Authentication and authorization have very specific, and different, meanings,
so
> documenting an |authenticated| parameter as affecting an "authorization"
section
> is confusing. Pick whichever is more appropriate and be consistent.

Done.

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/hangout_consent_dialog.js:58: *  upon |opt_parentBounds|.
On 2015/02/02 19:07:40, Jamie wrote:
> s/upon/within/

Done.

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/hangout_consent_dialog.js:64: if
(!this.onConsentResponseDeferred_) {
On 2015/02/02 19:07:40, Jamie wrote:
> I think this needs to be an assert. Returning the same Promise for two
separate
> calls will (I think) leave one of them hanging, because the Promise will only
> resolve once.

Adding then() is equivalent to subscribing to the 'resolve/reject' event of a
promise, if the promise has already resolved, the then() function will be invoke
immediately (asynchronously of course).  E.g.

var p = Promise.resolve('world');
p.then(function(value){  // hooking then() to a resolved promise.
  console.log('hello ' + value);
}); 

p.then(function(value){  // hooking then() to a resolved promise.
  console.log('this is a small ' + value);
});

will print out 
'hello world'
'this is a small world'

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/hangout_consent_dialog.js:70: outerBounds: {width:
DIALOG_WIDTH, height: height}
On 2015/02/02 19:07:40, Jamie wrote:
> Space after '{' and before '}' (the only time we don't do this is in JSDoc
type
> annotations).

Done.

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/hangout_consent_dialog.js:74: DIALOG_URL + '?' +
AUTHENTICATED_PARAM + '=' + String(!!authenticated);
On 2015/02/02 19:07:40, Jamie wrote:
> I think Boolean(authenticated) would be clearer than String(!!authenticated).

Done.

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/h...
File remoting/webapp/crd/js/hangout_consent_dialog_main.js (right):

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/hangout_consent_dialog_main.js:55:
document.addEventListener("DOMContentLoaded", onDomContentLoaded);
On 2015/02/02 19:07:40, Jamie wrote:
> Single-quotes for JS strings, please.
> 
> Elsewhere, we have just used "load". I'm not sure whether it makes any
> difference here (one potential problem that springs to mind is that if you
> switch to measuring the DOM rather than hard-coding the dialog dimensions,
there
> might be a race-condition if you use DOMContentLoaded, since you might try to
> measure the DOM before the CSS is loaded).

Excellent catch!  Thank you.

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/i...
File remoting/webapp/crd/js/it2me_helpee_channel.js (right):

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/i...
remoting/webapp/crd/js/it2me_helpee_channel.js:253: var bounds = /** @type
{Bounds} */ (message['parentBounds']);
On 2015/02/02 19:07:40, Jamie wrote:
> getObjectAttr would be better (it does some minimal type-checking).

Done.

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/i...
remoting/webapp/crd/js/it2me_helpee_channel.js:269: /** @type {function(*):void}
*/(this.connectToHost_.bind(this, email))
On 2015/02/02 19:07:40, Jamie wrote:
> Why does this call need a cast, but the others don't? Maybe this one can be
> removed now that we're using a more modern compiler?

Gary added this check when we switched to the new JS Compiler.
I think the new JSCompiler has a promise definition that is too strict
(function(string):void doesn't match function(*))

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/i...
remoting/webapp/crd/js/it2me_helpee_channel.js:317: return Promise.reject(new
Error(remoting.Error.NOT_AUTHORIZED));
On 2015/02/02 19:07:40, Jamie wrote:
> I think CANCELLED is a better error here. Generally, if the user explicitly
> declines something (CANCELLED), then no error should be displayed. If some
> operation failed because the user doesn't have permission to do it
> (NOT_AUTHORIZED) then we should show an error. Even if this code-path doesn't
> touch any common error handling code where such logic might be implemented,
it's
> better to be consistent.

Done.

https://codereview.chromium.org/888323002/diff/60001/remoting/webapp/base/js/...
File remoting/webapp/base/js/base.js (right):

https://codereview.chromium.org/888323002/diff/60001/remoting/webapp/base/js/...
remoting/webapp/base/js/base.js:167: base.getUrlParameters = function() {
Grouping it closer to the other url utilities

https://codereview.chromium.org/888323002/diff/60001/remoting/webapp/base/js/...
File remoting/webapp/base/js/ipc.js (right):

https://codereview.chromium.org/888323002/diff/60001/remoting/webapp/base/js/...
remoting/webapp/base/js/ipc.js:92: * @param {function(...?)} handler The handler
can be invoked by calling
Fix this to make it less restrictive.

[Jamie, 2015-02-03 18:22:44]

lgtm

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/html...
File remoting/webapp/crd/html/dialog_hangout_consent.css (right):

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/html...
remoting/webapp/crd/html/dialog_hangout_consent.css:104: display: none
!important;
On 2015/02/03 01:15:32, kelvinp wrote:
> On 2015/02/02 19:07:39, Jamie wrote:
> > Do you need this? I don't see any other rules that affect display in this
CSS
> > file, and if this is needed as a result of a rule in some other CSS file,
then
> I
> > think it belongs there.
> 
> This is how the CRD app hides elements using JavaScript.
> In JavaScript, we set element.hidden = true, this css rule picks up the
> attribute change and set display:none on the element.

Isn't that the default though? IIRC, we need this in main.css because there are
other, more specific, rules that also control the "display" attribute, but
that's not the case here.

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/h...
File remoting/webapp/crd/js/hangout_consent_dialog.js (right):

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/hangout_consent_dialog.js:39: * @param {boolean} confirm
whether the user authorize the it2me connection
On 2015/02/03 01:15:32, kelvinp wrote:
> On 2015/02/02 19:07:40, Jamie wrote:
> > s/whether/Whether/ (also, capitalize the first word of the parameter
> description
> > elsewhere in this file).
> > 
> > s/authorize/authorized/
> > 
> > Full-stop at the end of the sentence, please.
> 
> Done.

You've only addressed the first comment in this group.

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/i...
File remoting/webapp/crd/js/it2me_helpee_channel.js (right):

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/i...
remoting/webapp/crd/js/it2me_helpee_channel.js:269: /** @type {function(*):void}
*/(this.connectToHost_.bind(this, email))
On 2015/02/03 01:15:33, kelvinp wrote:
> On 2015/02/02 19:07:40, Jamie wrote:
> > Why does this call need a cast, but the others don't? Maybe this one can be
> > removed now that we're using a more modern compiler?
> 
> Gary added this check when we switched to the new JS Compiler.
> I think the new JSCompiler has a promise definition that is too strict
> (function(string):void doesn't match function(*))

I'm still not sure why this one needs annotating, but the others done. But, if
this is what's needed to make it compile, then so be it; it certainly isn't the
ugliest piece of JSDoc in the code-base :)

https://codereview.chromium.org/888323002/diff/60001/remoting/webapp/crd/js/h...
File remoting/webapp/crd/js/hangout_consent_dialog.js (right):

https://codereview.chromium.org/888323002/diff/60001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/hangout_consent_dialog.js:77: authorized: 
String(Boolean(authorized))
Nit: Only one space after ":". I also don't think you need to cast to String
(urlJoin should handle that), but I don't feel strongly about it.

https://codereview.chromium.org/888323002/diff/60001/remoting/webapp/crd/js/h...
File remoting/webapp/crd/js/hangout_consent_dialog_main.js (right):

https://codereview.chromium.org/888323002/diff/60001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/hangout_consent_dialog_main.js:54:
appWindow.moveTo(outerBounds.left, outerBounds.top);
This auto-resize behaviour is used in a couple of places, and the moveTo hack is
not something you'd necessarily remember to include, which makes it a good
candidate for pulling out into a base class. It should be a follow-up CL, but
would you mind creating a bug for it?

https://codereview.chromium.org/888323002/diff/60001/remoting/webapp/crd/js/i...
File remoting/webapp/crd/js/it2me_helpee_channel.js (right):

https://codereview.chromium.org/888323002/diff/60001/remoting/webapp/crd/js/i...
remoting/webapp/crd/js/it2me_helpee_channel.js:363: * @return {Promise<string>}
Promise that resolves with the OAuth token as the
I'm not familiar with this annotation. Does it mean that the resolve function
takes a single string parameter?

[kelvinp, 2015-02-03 19:31:46]

FYI

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/html...
File remoting/webapp/crd/html/dialog_hangout_consent.css (right):

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/html...
remoting/webapp/crd/html/dialog_hangout_consent.css:104: display: none
!important;
On 2015/02/03 18:22:44, Jamie wrote:
> On 2015/02/03 01:15:32, kelvinp wrote:
> > On 2015/02/02 19:07:39, Jamie wrote:
> > > Do you need this? I don't see any other rules that affect display in this
> CSS
> > > file, and if this is needed as a result of a rule in some other CSS file,
> then
> > I
> > > think it belongs there.
> > 
> > This is how the CRD app hides elements using JavaScript.
> > In JavaScript, we set element.hidden = true, this css rule picks up the
> > attribute change and set display:none on the element.
> 
> Isn't that the default though? IIRC, we need this in main.css because there
are
> other, more specific, rules that also control the "display" attribute, but
> that's not the case here.

I see.  I think using element.hidden is a chrome specific behavior that is not
documented anywhere. Ok, I will remove the style.  If I bring up the developer
console, I noticed that the hidden rule is marked as user agent style sheet.

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/h...
File remoting/webapp/crd/js/hangout_consent_dialog.js (right):

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/hangout_consent_dialog.js:39: * @param {boolean} confirm
whether the user authorize the it2me connection
On 2015/02/03 18:22:44, Jamie wrote:
> On 2015/02/03 01:15:32, kelvinp wrote:
> > On 2015/02/02 19:07:40, Jamie wrote:
> > > s/whether/Whether/ (also, capitalize the first word of the parameter
> > description
> > > elsewhere in this file).
> > > 
> > > s/authorize/authorized/
> > > 
> > > Full-stop at the end of the sentence, please.
> > 
> > Done.
> 
> You've only addressed the first comment in this group.

Done.

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/i...
File remoting/webapp/crd/js/it2me_helpee_channel.js (right):

https://codereview.chromium.org/888323002/diff/20001/remoting/webapp/crd/js/i...
remoting/webapp/crd/js/it2me_helpee_channel.js:269: /** @type {function(*):void}
*/(this.connectToHost_.bind(this, email))
On 2015/02/03 18:22:44, Jamie wrote:
> On 2015/02/03 01:15:33, kelvinp wrote:
> > On 2015/02/02 19:07:40, Jamie wrote:
> > > Why does this call need a cast, but the others don't? Maybe this one can
be
> > > removed now that we're using a more modern compiler?
> > 
> > Gary added this check when we switched to the new JS Compiler.
> > I think the new JSCompiler has a promise definition that is too strict
> > (function(string):void doesn't match function(*))
> 
> I'm still not sure why this one needs annotating, but the others done. But, if
> this is what's needed to make it compile, then so be it; it certainly isn't
the
> ugliest piece of JSDoc in the code-base :)

Because initializeHost_ and fetchOAuthToken_ doesn't take any parameters and
connectToHost_ does, which doesn't match the then function signature.

https://codereview.chromium.org/888323002/diff/60001/remoting/webapp/crd/js/h...
File remoting/webapp/crd/js/hangout_consent_dialog.js (right):

https://codereview.chromium.org/888323002/diff/60001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/hangout_consent_dialog.js:77: authorized: 
String(Boolean(authorized))
On 2015/02/03 18:22:44, Jamie wrote:
> Nit: Only one space after ":". I also don't think you need to cast to String
> (urlJoin should handle that), but I don't feel strongly about it.

Done.

https://codereview.chromium.org/888323002/diff/60001/remoting/webapp/crd/js/h...
File remoting/webapp/crd/js/hangout_consent_dialog_main.js (right):

https://codereview.chromium.org/888323002/diff/60001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/hangout_consent_dialog_main.js:54:
appWindow.moveTo(outerBounds.left, outerBounds.top);
On 2015/02/03 18:22:44, Jamie wrote:
> This auto-resize behaviour is used in a couple of places, and the moveTo hack
is
> not something you'd necessarily remember to include, which makes it a good
> candidate for pulling out into a base class. It should be a follow-up CL, but
> would you mind creating a bug for it?

Done.

https://codereview.chromium.org/888323002/diff/60001/remoting/webapp/crd/js/i...
File remoting/webapp/crd/js/it2me_helpee_channel.js (right):

https://codereview.chromium.org/888323002/diff/60001/remoting/webapp/crd/js/i...
remoting/webapp/crd/js/it2me_helpee_channel.js:363: * @return {Promise<string>}
Promise that resolves with the OAuth token as the
On 2015/02/03 18:22:44, Jamie wrote:
> I'm not familiar with this annotation. Does it mean that the resolve function
> takes a single string parameter?

Yes, it does. JSCompile will complain if you return Promise.resolve(true) in the
function.

[kelvinp, 2015-02-03 19:33:25]

Patchset #3 (id:80001) has been deleted

[kelvinp, 2015-02-03 20:01:20]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2015-02-03 20:02:06]

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

[commit-bot: I haz the power, 2015-02-03 21:02:26]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2015-02-03 21:03:41]

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