Enable Chrome Signin from Webstore

Enable Chrome Signin from Webstore

This CL adds webstore as a new source for Chrome signin. It also grabs password as soon as password form is submitted rather than at the end of navigation. The reason is webstore runs in its own context, redirect from gaia to webstore results in context swap during which password data is discarded, thus password must be sent to the oneclick signin helper before redirect occurs.

PM mock at https://docs.google.com/a/google.com/presentation/d/1zCvdhBXKL8RDQwuRauSZdgCbiRexJRZ564z9Uw0wS6U/edit?disco=AAAAAFg3OEI#slide=id.g996e03e1_00. This CL implements the transition from slide 7 to 8.

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

[guohui, 2013-01-30 21:38:35]

On 2013/01/30 21:36:49, guohui wrote:

Will add unit tests after the main code is reviewed.

[guohui, 2013-01-31 13:08:28]

On 2013/01/30 21:38:35, guohui wrote:
> On 2013/01/30 21:36:49, guohui wrote:
> 
> Will add unit tests after the main code is reviewed.

ping?

Our project is targeted at M26, so we need commit this change asap, could you
please have a look at the code at your earliest convenience?

[Roger Tawa OOO till Jul 10th, 2013-01-31 13:27:16]

This changes the code flow for all web-based sign in flows.  Can you please
verify that all scenarios described in this doc still function correctly? 
Thanks.

https://docs.google.com/a/google.com/presentation/d/1bjM0UJVoa7npBg38oRTFAwKh...

https://codereview.chromium.org/12086069/diff/2001/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/12086069/diff/2001/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:769: return handled;
If a message is not handled here, should the base class be called?

https://codereview.chromium.org/12086069/diff/2001/chrome/browser/ui/webui/sy...
File chrome/browser/ui/webui/sync_promo/sync_promo_trial.cc (right):

https://codereview.chromium.org/12086069/diff/2001/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_promo/sync_promo_trial.cc:27:
UMA_WEBSTORE_INSTALL_SIGNED_IN,
please add these right before UMA_MAX.

[guohui, 2013-01-31 16:44:01]

Thanks Roger.

I confirmed that all existing flows still work (first run, settings page,
advanced), though the UI of signin page is different from the mock, I guess its
because not all changes for the signin page are committed yet?

For the new signin case when triggered from webstore, I also confirmed that
signin without checking 'let me choose...' works. However, if the user checks
'let me choose...', then chrome navigates to the settings page with advanced
sync dialog open, and after sync setup is complete, chrome remains on the
settings page instead of navigating back to webstore. This is a known
limitation, and will be fixed in a later CL.

https://codereview.chromium.org/12086069/diff/2001/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/12086069/diff/2001/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:769: return handled;
On 2013/01/31 13:27:16, Roger Tawa wrote:
> If a message is not handled here, should the base class be called?

The base class WebContentsObserver just returns false.

https://codereview.chromium.org/12086069/diff/2001/chrome/browser/ui/webui/sy...
File chrome/browser/ui/webui/sync_promo/sync_promo_trial.cc (right):

https://codereview.chromium.org/12086069/diff/2001/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_promo/sync_promo_trial.cc:27:
UMA_WEBSTORE_INSTALL_SIGNED_IN,
On 2013/01/31 13:27:16, Roger Tawa wrote:
> please add these right before UMA_MAX.

Done.

[jam, 2013-01-31 17:47:45]

since you have multiple reviewers, you need to specify who should look at what

[Roger Tawa OOO till Jul 10th, 2013-01-31 18:23:05]

lgtm for one-click and webui changes

[guohui, 2013-01-31 19:35:50]

On 2013/01/31 17:47:45, jam wrote:
> since you have multiple reviewers, you need to specify who should look at what

rogerta for the sign in flow, and jam/sky for the new IPC message.

[commit-bot: I haz the power, 2013-01-31 23:58:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/guohui@chromium.org/12086069/16001

[commit-bot: I haz the power, 2013-01-31 23:58:06]

Presubmit check for 12086069-16001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome
    chrome/common/one_click_signin_messages.h
    ipc/ipc_message_start.h

Presubmit checks took 1.5s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[guohui, 2013-02-01 00:00:14]

On 2013/01/31 23:58:06, I haz the power (commit-bot) wrote:
> Presubmit check for 12086069-16001 failed and returned exit status 1.
> 
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for files in these directories:
>     chrome
>     chrome/common/one_click_signin_messages.h
>     ipc/ipc_message_start.h
> 
> Presubmit checks took 1.5s to calculate.
> 
> Was the presubmit check useful? Please send feedback & hate mail to
> maruel@chromium.org!

ping? still missing approval from jam@ for the new IPC message.

[guohui, 2013-02-01 02:25:52]

On 2013/02/01 00:00:14, guohui wrote:
> On 2013/01/31 23:58:06, I haz the power (commit-bot) wrote:
> > Presubmit check for 12086069-16001 failed and returned exit status 1.
> > 
> > 
> > Running presubmit commit checks ...
> > 
> > ** Presubmit ERRORS **
> > Missing LGTM from an OWNER for files in these directories:
> >     chrome
> >     chrome/common/one_click_signin_messages.h
> >     ipc/ipc_message_start.h
> > 
> > Presubmit checks took 1.5s to calculate.
> > 
> > Was the presubmit check useful? Please send feedback & hate mail to
> > maruel@chromium.org!
> 
> ping? still missing approval from jam@ for the new IPC message.

+creis@, as suggested by jam@.

[jam, 2013-02-01 02:45:59]

lgtm, you'll need a security person for the IPCs though

https://codereview.chromium.org/12086069/diff/16001/chrome/renderer/one_click...
File chrome/renderer/one_click_signin_agent.h (right):

https://codereview.chromium.org/12086069/diff/16001/chrome/renderer/one_click...
chrome/renderer/one_click_signin_agent.h:12: class WebFrame;
nit: don't need these forward declares since they're in overridden methods, so
they must be in the parent class' header

[guohui, 2013-02-01 02:54:26]

On 2013/02/01 02:45:59, jam wrote:
> lgtm, you'll need a security person for the IPCs though
> 
>
https://codereview.chromium.org/12086069/diff/16001/chrome/renderer/one_click...
> File chrome/renderer/one_click_signin_agent.h (right):
> 
>
https://codereview.chromium.org/12086069/diff/16001/chrome/renderer/one_click...
> chrome/renderer/one_click_signin_agent.h:12: class WebFrame;
> nit: don't need these forward declares since they're in overridden methods, so
> they must be in the parent class' header

Thanks John!

[guohui, 2013-02-01 02:55:03]

On 2013/02/01 02:54:26, guohui wrote:
> On 2013/02/01 02:45:59, jam wrote:
> > lgtm, you'll need a security person for the IPCs though
> > 
> >
>
https://codereview.chromium.org/12086069/diff/16001/chrome/renderer/one_click...
> > File chrome/renderer/one_click_signin_agent.h (right):
> > 
> >
>
https://codereview.chromium.org/12086069/diff/16001/chrome/renderer/one_click...
> > chrome/renderer/one_click_signin_agent.h:12: class WebFrame;
> > nit: don't need these forward declares since they're in overridden methods,
so
> > they must be in the parent class' header
> 
> Thanks John!

+tsepez@ for security review of the new IPC message.

[Tom Sepez, 2013-02-01 04:04:11]

The message in and of itself is LGTM, but have you been working with anyone from
the security team on the larger picture?

[guohui, 2013-02-01 04:20:57]

On 2013/02/01 04:04:11, Tom Sepez wrote:
> The message in and of itself is LGTM, but have you been working with anyone
from
> the security team on the larger picture?

Thanks Tom.

By the larger picture, do you mean the feature of enabling chrome signin from
launcher? Yes, we have opened a security ticket at b/8034020 and it is lgtm'ed
today.

[commit-bot: I haz the power, 2013-02-01 04:21:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/guohui@chromium.org/12086069/21001

[Charlie Reis, 2013-02-01 19:40:49]

In the future, please do not commit if you add someone as a reviewer until
you've gotten their feedback, or at least heard from them that it's ok to go
ahead.

In this case, this LGTM (apart from a nit that can be fixed later).

https://codereview.chromium.org/12086069/diff/30/chrome/browser/ui/sync/one_c...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/12086069/diff/30/chrome/browser/ui/sync/one_c...
chrome/browser/ui/sync/one_click_signin_helper.cc:921: // If this explicit sign
in is not from settings page/webostre, show the NTP
nit: webstore

[guohui, 2013-02-01 19:43:03]

On 2013/02/01 19:40:49, creis wrote:
> In the future, please do not commit if you add someone as a reviewer until
> you've gotten their feedback, or at least heard from them that it's ok to go
> ahead.
> 
> In this case, this LGTM (apart from a nit that can be fixed later).
> 
>
https://codereview.chromium.org/12086069/diff/30/chrome/browser/ui/sync/one_c...
> File chrome/browser/ui/sync/one_click_signin_helper.cc (right):
> 
>
https://codereview.chromium.org/12086069/diff/30/chrome/browser/ui/sync/one_c...
> chrome/browser/ui/sync/one_click_signin_helper.cc:921: // If this explicit
sign
> in is not from settings page/webostre, show the NTP
> nit: webstore

Sorry creiz, we were in a rush to commit this change yesterday so that we could
start testing with nightly build today.

I will fix the nit in a later CL.

Thanks!