ChromeOS: should send old user GAPS cookie to GAIA on user reauthentication.

ChromeOS: should send old user GAPS cookie to GAIA on user reauthentication.

BUG=460136
TEST=manual

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

[Alexander Alekseev, 2015-07-09 22:42:08]

alemate@chromium.org changed reviewers:
+ dzhioev@chromium.org, xiyuan@chromium.org

[Alexander Alekseev, 2015-07-09 22:42:08]

Please review:

xiyuan@ :
chrome/browser/resources/gaia_auth_host/authenticator.js

dzhioev@ : all the rest

[Alexander Alekseev, 2015-07-09 23:04:39]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-09 23:05:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1229883003/1

[xiyuan, 2015-07-09 23:08:29]

https://codereview.chromium.org/1229883003/diff/1/chrome/browser/resources/ga...
File chrome/browser/resources/gaia_auth_host/authenticator.js (right):

https://codereview.chromium.org/1229883003/diff/1/chrome/browser/resources/ga...
chrome/browser/resources/gaia_auth_host/authenticator.js:121: this.gaps_cookie_
= null;
nit: gaps_cookie_ -> gapsCookie_

JS variable name should be like variableName. The existing |oauth_code_| also
violates the rule and should be fixed.

See http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#Naming

https://codereview.chromium.org/1229883003/diff/1/chrome/browser/resources/ga...
chrome/browser/resources/gaia_auth_host/authenticator.js:225:
this.webview_.request.onBeforeSendHeaders.addListener(
Why this code lives here instead of with other request filters in ctor around
line 160?

https://codereview.chromium.org/1229883003/diff/1/chrome/browser/resources/ga...
chrome/browser/resources/gaia_auth_host/authenticator.js:409: * @param@
{header_value} Original string value of Cookie header.
Wrong JSDoc format here and below.

Should be something like:

@param {string} headerValue Original string value of Cookie header.

[Alexander Alekseev, 2015-07-09 23:22:41]

https://codereview.chromium.org/1229883003/diff/1/chrome/browser/resources/ga...
File chrome/browser/resources/gaia_auth_host/authenticator.js (right):

https://codereview.chromium.org/1229883003/diff/1/chrome/browser/resources/ga...
chrome/browser/resources/gaia_auth_host/authenticator.js:121: this.gaps_cookie_
= null;
On 2015/07/09 23:08:29, xiyuan wrote:
> nit: gaps_cookie_ -> gapsCookie_
> 
> JS variable name should be like variableName. The existing |oauth_code_| also
> violates the rule and should be fixed.
> 
> See
http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#Naming

Done.

https://codereview.chromium.org/1229883003/diff/1/chrome/browser/resources/ga...
chrome/browser/resources/gaia_auth_host/authenticator.js:225:
this.webview_.request.onBeforeSendHeaders.addListener(
On 2015/07/09 23:08:29, xiyuan wrote:
> Why this code lives here instead of with other request filters in ctor around
> line 160?

Because it depends on load parameter (line 199):

this.idpOrigin_ = data.gaiaUrl || IDP_ORIGIN;

https://codereview.chromium.org/1229883003/diff/1/chrome/browser/resources/ga...
chrome/browser/resources/gaia_auth_host/authenticator.js:409: * @param@
{header_value} Original string value of Cookie header.
On 2015/07/09 23:08:29, xiyuan wrote:
> Wrong JSDoc format here and below.
> 
> Should be something like:
> 
> @param {string} headerValue Original string value of Cookie header.

Done.

[xiyuan, 2015-07-09 23:30:05]

authenticator.js LGTM

https://codereview.chromium.org/1229883003/diff/1/chrome/browser/resources/ga...
File chrome/browser/resources/gaia_auth_host/authenticator.js (right):

https://codereview.chromium.org/1229883003/diff/1/chrome/browser/resources/ga...
chrome/browser/resources/gaia_auth_host/authenticator.js:225:
this.webview_.request.onBeforeSendHeaders.addListener(
On 2015/07/09 23:22:40, Alexander Alekseev wrote:
> On 2015/07/09 23:08:29, xiyuan wrote:
> > Why this code lives here instead of with other request filters in ctor
around
> > line 160?
> 
> Because it depends on load parameter (line 199):
> 
> this.idpOrigin_ = data.gaiaUrl || IDP_ORIGIN;

I see. Could you add a comment to document why this is here and also mention
that we assume |idpOrigin_| would not change?

[Alexander Alekseev, 2015-07-09 23:43:22]

https://codereview.chromium.org/1229883003/diff/1/chrome/browser/resources/ga...
File chrome/browser/resources/gaia_auth_host/authenticator.js (right):

https://codereview.chromium.org/1229883003/diff/1/chrome/browser/resources/ga...
chrome/browser/resources/gaia_auth_host/authenticator.js:225:
this.webview_.request.onBeforeSendHeaders.addListener(
On 2015/07/09 23:30:05, xiyuan wrote:
> On 2015/07/09 23:22:40, Alexander Alekseev wrote:
> > On 2015/07/09 23:08:29, xiyuan wrote:
> > > Why this code lives here instead of with other request filters in ctor
> around
> > > line 160?
> > 
> > Because it depends on load parameter (line 199):
> > 
> > this.idpOrigin_ = data.gaiaUrl || IDP_ORIGIN;
> 
> I see. Could you add a comment to document why this is here and also mention
> that we assume |idpOrigin_| would not change?

Done.

[dzhioev (left Google), 2015-07-10 21:26:32]

https://codereview.chromium.org/1229883003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/gaia_auth_host/authenticator.js (right):

https://codereview.chromium.org/1229883003/diff/40001/chrome/browser/resource...
chrome/browser/resources/gaia_auth_host/authenticator.js:223: var filterPrefix =
this.idpOrigin_ + EMBEDDED_SETUP_CHROMEOS_ENDPOINT;
nit: Move this line to if-block below.

https://codereview.chromium.org/1229883003/diff/40001/chrome/browser/resource...
chrome/browser/resources/gaia_auth_host/authenticator.js:271:
this.newGapsCookie_ = null;
nit: why these lines was added here? They have nothing to do with URL, so they
should be moved to Authenticator.prototype.load().

[dzhioev (left Google), 2015-07-10 21:26:47]

LGTM with nits

[Alexander Alekseev, 2015-07-10 23:05:25]

The CQ bit was checked by alemate@chromium.org

[Alexander Alekseev, 2015-07-10 23:05:25]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
dzhioev@chromium.org
Link to the patchset: https://codereview.chromium.org/1229883003/#ps60001
(title: "Update after review.")

[commit-bot: I haz the power, 2015-07-10 23:05:34]

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

[Alexander Alekseev, 2015-07-10 23:05:34]

https://codereview.chromium.org/1229883003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/gaia_auth_host/authenticator.js (right):

https://codereview.chromium.org/1229883003/diff/40001/chrome/browser/resource...
chrome/browser/resources/gaia_auth_host/authenticator.js:223: var filterPrefix =
this.idpOrigin_ + EMBEDDED_SETUP_CHROMEOS_ENDPOINT;
On 2015/07/10 21:26:31, dzhioev wrote:
> nit: Move this line to if-block below.

Done.

https://codereview.chromium.org/1229883003/diff/40001/chrome/browser/resource...
chrome/browser/resources/gaia_auth_host/authenticator.js:271:
this.newGapsCookie_ = null;
On 2015/07/10 21:26:31, dzhioev wrote:
> nit: why these lines was added here? They have nothing to do with URL, so they
> should be moved to Authenticator.prototype.load().

Done.

[commit-bot: I haz the power, 2015-07-11 00:07:38]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-07-11 00:08:40]

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

[achuithb, 2015-07-13 21:15:08]

On 2015/07/11 00:08:40, commit-bot: I haz the power wrote:
> Patchset 4 (id:??) landed as
> https://crrev.com/edd54c70d4c80a829ea37e24af6261aff65fe83f
> Cr-Commit-Position: refs/heads/master@{#338404}

This needs to be merged into M45