Implement signin using webview

Implement signin using webview

This CL implements the Chrome signin on desktop using webview. Since the webview approach is
significantly different than the current iframe one, thus I created a fork of some files
instead of adding more logic to the existing over-complicated code. The feature is enabled
under a flag --enable-webview-based-signin. Once the feature is fully launched on both dekstop
and ChromeOS, the old files should be deleted.

There are a few known issues.

1. Gaia needs to update their script to post credentials directly to chrome://chrome-signin
instead of the gaia auth extension. Before the gaia code is updated, webview signin cannot scrape
password and the checkbox value for choosing what to sync.

2. Webview currently only loads in a full tab, need to investigate why it doesn't work when
embedded in the avatar menu.

3. Some webview apis are broken in an extension-less webui context.
https://codereview.chromium.org/670173002/ fixes a few that are required for a minimum desktop
signin flow. One important missing piece is the extension messaging api, and as a result Gnubby
does not work with a webview, tracked in crbug/426016.

4. Some standard WebUI features are missing, such as zoom, find, print preview, need to
decide whether it is worth to support them.

5. Webview is not fully accessibility proof. According to webview team, most accessibility bugs
have been fixed by the accessibility team (crbug/330307, crbug/368298), need to confirm.

6. This CL only implements the desktop signin flow. The ChromeOS signin flow is a superset
of the desktop one, and ChromeOS team needs to add extra logic to complete it for ChromeOS.

BUG=364432
Committed: https://crrev.com/69cf14737441b102917f3773500e3ae8c6bea794
Cr-Commit-Position: refs/heads/master@{#301130}

[guohui, 2014-10-21 20:29:50]

Patchset #1 (id:1) has been deleted

[guohui, 2014-10-21 20:51:22]

Patchset #1 (id:20001) has been deleted

[guohui, 2014-10-21 20:51:46]

guohui@chromium.org changed reviewers:
+ rogerta@chromium.org, xiyuan@chromium.org

[guohui, 2014-10-21 20:51:47]

Hey,

could you please review the CL?

Thanks,

Hui

[guohui, 2014-10-21 20:53:27]

guohui@chromium.org changed reviewers:
+ fsamuel@chromium.org

[guohui, 2014-10-21 20:54:31]

On 2014/10/21 20:51:47, guohui wrote:
> Hey,
> 
> could you please review the CL?
> 
> Thanks,
> 
> Hui

+fsamuel@ for extensions/

[Fady Samuel, 2014-10-21 21:32:57]

https://codereview.chromium.org/646983008/diff/40001/extensions/browser/api/w...
File extensions/browser/api/web_request/web_request_api_helpers.cc (right):

https://codereview.chromium.org/646983008/diff/40001/extensions/browser/api/w...
extensions/browser/api/web_request/web_request_api_helpers.cc:1212:
runtime_data->SetHasUsedWebRequest(extension.get(), true);
What's going on here? I'm confused.

https://codereview.chromium.org/646983008/diff/40001/extensions/browser/exten...
File extensions/browser/extension_function.h (right):

https://codereview.chromium.org/646983008/diff/40001/extensions/browser/exten...
extensions/browser/extension_function.h:223: return extension_ ?
extension_->id() : base::EmptyString();
I think Ben Kalman should review this change. It might make sense to break down
this CL into multiple CLs as there are many stakeholders here.

https://codereview.chromium.org/646983008/diff/40001/extensions/browser/exten...
File extensions/browser/extension_function_dispatcher.cc (right):

https://codereview.chromium.org/646983008/diff/40001/extensions/browser/exten...
extensions/browser/extension_function_dispatcher.cc:235: const std::string&
extension_id = extension ?
I'd prefer if these changes were in a separate CL.

[xiyuan, 2014-10-21 22:19:16]

gnubby needs extension messaging API.

[Fady Samuel, 2014-10-21 22:20:19]

On 2014/10/21 22:19:16, xiyuan wrote:
> gnubby needs extension messaging API.

I'm working on fixing that now. Hopefully I'll have a fix soonish.

[Roger Tawa OOO till Jul 10th, 2014-10-22 14:49:54]

https://codereview.chromium.org/646983008/diff/60001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/646983008/diff/60001/chrome/app/generated_res...
chrome/app/generated_resources.grd:13811: +      When enabled, will use a
webview-based Chrome sign-in flow.
Mention that this overrides web-based sign in if both are enabled.

[guohui, 2014-10-22 15:35:34]

With the crash fix in zoomcontroller, the cl has grown much bigger, so as
suggested by fady, i split the extension framework changes into a separate cl at
https://codereview.chromium.org/670173002/.

https://codereview.chromium.org/646983008/diff/40001/extensions/browser/api/w...
File extensions/browser/api/web_request/web_request_api_helpers.cc (right):

https://codereview.chromium.org/646983008/diff/40001/extensions/browser/api/w...
extensions/browser/api/web_request/web_request_api_helpers.cc:1212:
runtime_data->SetHasUsedWebRequest(extension.get(), true);
On 2014/10/21 21:32:57, Fady Samuel wrote:
> What's going on here? I'm confused.

lets continue discussion in the other cl
https://codereview.chromium.org/670173002/.

https://codereview.chromium.org/646983008/diff/40001/extensions/browser/exten...
File extensions/browser/extension_function.h (right):

https://codereview.chromium.org/646983008/diff/40001/extensions/browser/exten...
extensions/browser/extension_function.h:223: return extension_ ?
extension_->id() : base::EmptyString();
On 2014/10/21 21:32:57, Fady Samuel wrote:
> I think Ben Kalman should review this change. It might make sense to break
down
> this CL into multiple CLs as there are many stakeholders here.

Done.

https://codereview.chromium.org/646983008/diff/40001/extensions/browser/exten...
File extensions/browser/extension_function_dispatcher.cc (right):

https://codereview.chromium.org/646983008/diff/40001/extensions/browser/exten...
extensions/browser/extension_function_dispatcher.cc:235: const std::string&
extension_id = extension ?
On 2014/10/21 21:32:57, Fady Samuel wrote:
> I'd prefer if these changes were in a separate CL.

Done.

https://codereview.chromium.org/646983008/diff/60001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/646983008/diff/60001/chrome/app/generated_res...
chrome/app/generated_resources.grd:13811: +      When enabled, will use a
webview-based Chrome sign-in flow.
On 2014/10/22 14:49:54, Roger Tawa wrote:
> Mention that this overrides web-based sign in if both are enabled.

Done.

[xiyuan, 2014-10-22 17:20:53]

LGTM with nits

https://codereview.chromium.org/646983008/diff/80001/chrome/browser/resources...
File chrome/browser/resources/gaia_auth_host/authenticator.js (right):

https://codereview.chromium.org/646983008/diff/80001/chrome/browser/resources...
chrome/browser/resources/gaia_auth_host/authenticator.js:175:
chrome.send('switchToFullTab', [currentUrl]);
This assumes webui page. Maybe we should have a callback or something to let
embedder of Authenticator to decide what to do?

https://codereview.chromium.org/646983008/diff/80001/chrome/browser/resources...
chrome/browser/resources/gaia_auth_host/authenticator.js:181:
this.frame_.contentWindow.postMessage('', this.frame_.src);
nit: Maybe using an empty object {} instead of ''?

https://codereview.chromium.org/646983008/diff/80001/chrome/browser/resources...
chrome/browser/resources/gaia_auth_host/authenticator.js:273:
window.open(e.targetUrl, '_blank');
I wonder if we should let the embedder to decide what to do with "NewWindow"
request.

[Roger Tawa OOO till Jul 10th, 2014-10-22 19:30:01]

https://codereview.chromium.org/646983008/diff/80001/chrome/browser/resources...
File chrome/browser/resources/gaia_auth_host/authenticator.js (right):

https://codereview.chromium.org/646983008/diff/80001/chrome/browser/resources...
chrome/browser/resources/gaia_auth_host/authenticator.js:61:
assert(this.frame_);
Nit: rename frame_ to webView_ ?
Rename container to webView ?

https://codereview.chromium.org/646983008/diff/80001/components/signin/core/c...
File components/signin/core/common/signin_switches.cc (right):

https://codereview.chromium.org/646983008/diff/80001/components/signin/core/c...
components/signin/core/common/signin_switches.cc:39: const char
kEnableWebviewBasedSignin[] = "enable-webview-based-signin";
Please add this flag to the chrome://signin-internals pages.

[guohui, 2014-10-23 19:12:01]

Removed events from Authenticator.js, and instead created a listener interface
for receiving notifications of authenticator events, e.g. authentication
completed, UI ready, etc.

https://codereview.chromium.org/646983008/diff/80001/chrome/browser/resources...
File chrome/browser/resources/gaia_auth_host/authenticator.js (right):

https://codereview.chromium.org/646983008/diff/80001/chrome/browser/resources...
chrome/browser/resources/gaia_auth_host/authenticator.js:61:
assert(this.frame_);
On 2014/10/22 19:30:00, Roger Tawa wrote:
> Nit: rename frame_ to webView_ ?
> Rename container to webView ?

Done.

https://codereview.chromium.org/646983008/diff/80001/chrome/browser/resources...
chrome/browser/resources/gaia_auth_host/authenticator.js:175:
chrome.send('switchToFullTab', [currentUrl]);
On 2014/10/22 17:20:53, xiyuan wrote:
> This assumes webui page. Maybe we should have a callback or something to let
> embedder of Authenticator to decide what to do?

Done.

https://codereview.chromium.org/646983008/diff/80001/chrome/browser/resources...
chrome/browser/resources/gaia_auth_host/authenticator.js:181:
this.frame_.contentWindow.postMessage('', this.frame_.src);
On 2014/10/22 17:20:53, xiyuan wrote:
> nit: Maybe using an empty object {} instead of ''? 

Done.

https://codereview.chromium.org/646983008/diff/80001/chrome/browser/resources...
chrome/browser/resources/gaia_auth_host/authenticator.js:273:
window.open(e.targetUrl, '_blank');
On 2014/10/22 17:20:53, xiyuan wrote:
> I wonder if we should let the embedder to decide what to do with "NewWindow"
> request.

Done.

https://codereview.chromium.org/646983008/diff/80001/components/signin/core/c...
File components/signin/core/common/signin_switches.cc (right):

https://codereview.chromium.org/646983008/diff/80001/components/signin/core/c...
components/signin/core/common/signin_switches.cc:39: const char
kEnableWebviewBasedSignin[] = "enable-webview-based-signin";
On 2014/10/22 19:30:01, Roger Tawa wrote:
> Please add this flag to the chrome://signin-internals pages.

Done.

[guohui, 2014-10-23 19:16:16]

Patchset #4 (id:100001) has been deleted

[guohui, 2014-10-23 19:16:54]

guohui@chromium.org changed reviewers:
+ sky@chromium.org

[guohui, 2014-10-23 19:16:55]

+sky@ for owner review of  chrome\browser\browser_resources.grd

[guohui, 2014-10-23 19:21:04]

Patchset #4 (id:120001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-10-23 19:27:18]

components/signin lgtm

[xiyuan, 2014-10-23 19:32:04]

SLGTM

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

https://codereview.chromium.org/646983008/diff/140001/chrome/browser/resource...
chrome/browser/resources/gaia_auth_host/authenticator.js:52: * @param
{Authenticator.Listener=} listener An optional listener for events.
nit: listener -> opt_listner

https://codereview.chromium.org/646983008/diff/140001/chrome/browser/resource...
chrome/browser/resources/gaia_auth_host/authenticator.js:75:
Authenticator.prototype = Object.create(cr.EventTarget.prototype);
If we don't dispatch event on this class, it does not need to be derived from
EventTarget.

https://codereview.chromium.org/646983008/diff/140001/chrome/browser/resource...
chrome/browser/resources/gaia_auth_host/authenticator.js:211: if
(this.listener_) this.listener_.onReady();
nit: Put "this.listener_.onReady()" on a separate line here and update other
one-line "if"s.

[guohui, 2014-10-23 20:13:34]

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

https://codereview.chromium.org/646983008/diff/140001/chrome/browser/resource...
chrome/browser/resources/gaia_auth_host/authenticator.js:52: * @param
{Authenticator.Listener=} listener An optional listener for events.
On 2014/10/23 19:32:03, xiyuan wrote:
> nit: listener -> opt_listner

Done.

https://codereview.chromium.org/646983008/diff/140001/chrome/browser/resource...
chrome/browser/resources/gaia_auth_host/authenticator.js:75:
Authenticator.prototype = Object.create(cr.EventTarget.prototype);
On 2014/10/23 19:32:03, xiyuan wrote:
> If we don't dispatch event on this class, it does not need to be derived from
> EventTarget.

Unfortunately, to be compatible with the old event based signin flow, it still
has to inherit the EventTarget interface, otherwise we will have to add extra
logic to every caller before they call GaiaAuthHost.addEventListener.

https://codereview.chromium.org/646983008/diff/140001/chrome/browser/resource...
chrome/browser/resources/gaia_auth_host/authenticator.js:211: if
(this.listener_) this.listener_.onReady();
On 2014/10/23 19:32:03, xiyuan wrote:
> nit: Put "this.listener_.onReady()" on a separate line here and update other
> one-line "if"s.

Done.

[guohui, 2014-10-23 20:14:26]

guohui@chromium.org changed reviewers:
+ asvitkine@chromium.org

[guohui, 2014-10-23 20:14:27]

+asvitkine for owner review of histograms.xml

[Alexei Svitkine (slow), 2014-10-23 20:26:53]

histograms lgtm, I didn't look at anything else

[guohui, 2014-10-24 02:05:17]

The CQ bit was checked by guohui@chromium.org

[commit-bot: I haz the power, 2014-10-24 02:05:52]

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

[guohui, 2014-10-24 02:09:09]

The CQ bit was unchecked by guohui@chromium.org

[guohui, 2014-10-24 15:54:13]

On 2014/10/24 02:05:52, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/646983008/160001

ping sky@ for owner review fo chrome/browser/browser_resources.grd?

[sky, 2014-10-24 16:29:10]

browser_resources.grd LGTM

[guohui, 2014-10-24 16:29:48]

The CQ bit was checked by guohui@chromium.org

[commit-bot: I haz the power, 2014-10-24 16:30:41]

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

[commit-bot: I haz the power, 2014-10-24 17:14:35]

Committed patchset #5 (id:160001)

[commit-bot: I haz the power, 2014-10-24 17:15:20]

Patchset 5 (id:??) landed as
https://crrev.com/69cf14737441b102917f3773500e3ae8c6bea794
Cr-Commit-Position: refs/heads/master@{#301130}