Webview-based signin shouldn't create a partition for the embedder.

Webview-based signin shouldn't create a partition for the embedder.

In recent CLs that have turned on webview-based signin, the old chrome
signin url has been adopted by the new signin mechanism, but this has
resulted in the webview's embedder being placed in a separate storage
partition, which has broken both default page zoom behaviour.
It has also prevented the appearance of the signin
page's zoom level in the zoom exceptions list in
settings.

This CL restores previous behaviour, where the webview's embedder lives
in the default storage partition.

BUG=462559
Committed: https://crrev.com/edb762e8e21da7075b42220e0d78bae6f06431fb
Cr-Commit-Position: refs/heads/master@{#322050}

[wjmaclean, 2015-03-10 16:01:24]

wjmaclean@chromium.org changed reviewers:
+ thestig@chromium.org

[wjmaclean, 2015-03-10 16:01:34]

PTAL?

[wjmaclean, 2015-03-10 17:40:44]

wjmaclean@chromium.org changed reviewers:
+ creis@chromium.org
- thestig@chromium.org

[wjmaclean, 2015-03-10 17:41:41]

Switching reviewer to creis@ since it involves storage partitions ...

[Charlie Reis, 2015-03-11 00:07:27]

This was the behavior before the switch to iframe-based signin, right?  Seems
reasonable to me.

Can we add a check to a webview-based signin test that the embedder ends up in
the default storage partition?

[wjmaclean, 2015-03-11 13:12:17]

Added a test. Yes, the webview based signin used the default storage partition
until recently when it was regressed.

[wjmaclean, 2015-03-11 13:59:03]

On 2015/03/11 13:12:17, wjmaclean wrote:
> Added a test. Yes, the webview based signin used the default storage partition
> until recently when it was regressed.

It seems the new test is failing on ChromeOS ... I guess that the webview-based
signin is not on by default there?

Is there a way to force the flag on for this test? Or should I just disable it
on ChromeOS bots?

[wjmaclean, 2015-03-11 15:46:52]

wjmaclean@chromium.org changed reviewers:
+ blundell@chromium.org

[wjmaclean, 2015-03-11 15:46:53]

blundell@chromium.org: Please review changes in 

components/

Thanks!

[wjmaclean, 2015-03-11 19:21:40]

wjmaclean@chromium.org changed reviewers:
- blundell@chromium.org

[wjmaclean, 2015-03-11 19:22:24]

wjmaclean@chromium.org changed reviewers:
+ rogerta@chromium.org

[wjmaclean, 2015-03-11 19:23:04]

On 2015/03/11 15:46:53, wjmaclean (ooo 2015.03.12 -22) wrote:
> mailto:blundell@chromium.org: Please review changes in 
> 
> components/
> 
> Thanks!

Switching reviewer to rogerta@ as I didn't realize blundell@ was ooo ...

I'm ooo starting tomorrow ...

[Roger Tawa OOO till Jul 10th, 2015-03-11 19:48:05]

lgtm

Thanks James.

[Charlie Reis, 2015-03-11 20:23:11]

Thanks for the test!  A few minor things I'm uncertain about, but otherwise it
looks good.

https://codereview.chromium.org/996743003/diff/60001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/996743003/diff/60001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:888: // Chrome signin page has
an embedded iframe of extension and web content,
Please update this comment as well.

https://codereview.chromium.org/996743003/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/host_zoom_map_browsertest.cc (right):

https://codereview.chromium.org/996743003/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/host_zoom_map_browsertest.cc:241: #if
defined(OS_CHROMEOS)
You need a comment that explains why this is disabled on ChromeOS.

It may be better to append the switch in the test (as you do below with the
iframe switch) and let it run on ChromeOS.  But I realize webview-based signin
might have other issues that prevent it from working on ChromeOS, so a comment
is also ok.

https://codereview.chromium.org/996743003/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/host_zoom_map_browsertest.cc:297: // For the webview
based sign-in code, the sign in page uses the default host
Please update this comment.

https://codereview.chromium.org/996743003/diff/60001/components/signin/core/c...
File components/signin/core/common/profile_management_switches.cc (right):

https://codereview.chromium.org/996743003/diff/60001/components/signin/core/c...
components/signin/core/common/profile_management_switches.cc:168:
command_line->AppendSwitch(switches::kEnableIframeBasedSignin);
Is there a reason you need this method rather than just calling
command_line->AppendSwitch directly in your test class?  (That's what we do in
SitePerProcessBrowserTest, for example.)

If you do need this, then you should probably have a similar DCHECK as the
methods below for the IsEnableWebviewBasedSignin case.

[wjmaclean, 2015-03-23 15:31:52]

Back from vacay, updated, PTAL :-)

https://codereview.chromium.org/996743003/diff/60001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/996743003/diff/60001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:888: // Chrome signin page has
an embedded iframe of extension and web content,
On 2015/03/11 20:23:10, Charlie Reis wrote:
> Please update this comment as well.

Done.

https://codereview.chromium.org/996743003/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/host_zoom_map_browsertest.cc (right):

https://codereview.chromium.org/996743003/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/host_zoom_map_browsertest.cc:241: #if
defined(OS_CHROMEOS)
On 2015/03/11 20:23:10, Charlie Reis wrote:
> You need a comment that explains why this is disabled on ChromeOS.
> 
> It may be better to append the switch in the test (as you do below with the
> iframe switch) and let it run on ChromeOS.  But I realize webview-based signin
> might have other issues that prevent it from working on ChromeOS, so a comment
> is also ok.

Let's force the test to run for ChromeOS, but check that if Webview signin is
not being used, the partitions must be different.

https://codereview.chromium.org/996743003/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/host_zoom_map_browsertest.cc:297: // For the webview
based sign-in code, the sign in page uses the default host
On 2015/03/11 20:23:10, Charlie Reis wrote:
> Please update this comment.

Done.

https://codereview.chromium.org/996743003/diff/60001/components/signin/core/c...
File components/signin/core/common/profile_management_switches.cc (right):

https://codereview.chromium.org/996743003/diff/60001/components/signin/core/c...
components/signin/core/common/profile_management_switches.cc:168:
command_line->AppendSwitch(switches::kEnableIframeBasedSignin);
On 2015/03/11 20:23:10, Charlie Reis wrote:
> Is there a reason you need this method rather than just calling
> command_line->AppendSwitch directly in your test class?  (That's what we do in
> SitePerProcessBrowserTest, for example.)
> 
> If you do need this, then you should probably have a similar DCHECK as the
> methods below for the IsEnableWebviewBasedSignin case.

I can do it directly in the test ... I was just following an existing pattern
:-)

[Charlie Reis, 2015-03-24 18:27:18]

Thanks, LGTM

[wjmaclean, 2015-03-24 18:30:20]

The CQ bit was checked by wjmaclean@chromium.org

[wjmaclean, 2015-03-24 18:30:20]

The patchset sent to the CQ was uploaded after l-g-t-m from rogerta@chromium.org
Link to the patchset: https://codereview.chromium.org/996743003/#ps80001 (title:
"Address comments, re-enable test on ChromeOS.")

[commit-bot: I haz the power, 2015-03-24 18:30:38]

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

[commit-bot: I haz the power, 2015-03-24 19:41:10]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-24 19:41:55]

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