Display signin dialog for the default profile after the last profile being deleted.

Display signin dialog with system profile after the last profile being deleted when force signin enabled.

BUG=642059
Committed: https://crrev.com/438598fc3ff38423311a37341023690405bc34a0
Cr-Commit-Position: refs/heads/master@{#423879}

[zmin, 2016-10-04 16:11:22]

Description was changed from

==========
Display signin dialog for the default profile after the last profile being
deleted.

BUG=642059
==========

to

==========
Display signin dialog for the default profile after the last profile being
deleted for force signin.

BUG=642059
==========

[zmin, 2016-10-04 16:14:24]

zmin@chromium.org changed reviewers:
+ anthonyvd@chromium.org, sky@chromium.org

[zmin, 2016-10-04 16:14:25]

Hi Scott, Anthony,

Can you review the CL please?

Owen

[sky, 2016-10-04 18:22:27]

https://codereview.chromium.org/2392953002/diff/1/chrome/browser/ui/views/pro...
File chrome/browser/ui/views/profiles/user_manager_view.cc (right):

https://codereview.chromium.org/2392953002/diff/1/chrome/browser/ui/views/pro...
chrome/browser/ui/views/profiles/user_manager_view.cc:292: if (browser_context
== nullptr)
This seems wrong to me. How is it that web_view_ would have the right
BrowserContext, but it wouldn't be passed to this function?

[zmin, 2016-10-04 19:02:57]

On 2016/10/04 18:22:27, sky wrote:
>
https://codereview.chromium.org/2392953002/diff/1/chrome/browser/ui/views/pro...
> File chrome/browser/ui/views/profiles/user_manager_view.cc (right):
> 
>
https://codereview.chromium.org/2392953002/diff/1/chrome/browser/ui/views/pro...
> chrome/browser/ui/views/profiles/user_manager_view.cc:292: if (browser_context
> == nullptr)
> This seems wrong to me. How is it that web_view_ would have the right
> BrowserContext, but it wouldn't be passed to this function?

UserManager always uses System Profile as BrowserContext while the Gaia signin
dialog doesn't have to. So BrowserContext needs to be passed into the function
as a parameter.
However, the web_view_'s BrowserContext can still be used as a default value
here so that the function is abled to be called without an instance of System
Profile as parameter.

[sky, 2016-10-05 15:45:21]

On 2016/10/04 19:02:57, zmin wrote:
> On 2016/10/04 18:22:27, sky wrote:
> >
>
https://codereview.chromium.org/2392953002/diff/1/chrome/browser/ui/views/pro...
> > File chrome/browser/ui/views/profiles/user_manager_view.cc (right):
> > 
> >
>
https://codereview.chromium.org/2392953002/diff/1/chrome/browser/ui/views/pro...
> > chrome/browser/ui/views/profiles/user_manager_view.cc:292: if
(browser_context
> > == nullptr)
> > This seems wrong to me. How is it that web_view_ would have the right
> > BrowserContext, but it wouldn't be passed to this function?
> 
> UserManager always uses System Profile as BrowserContext while the Gaia signin
> dialog doesn't have to. So BrowserContext needs to be passed into the function
> as a parameter.
> However, the web_view_'s BrowserContext can still be used as a default value
> here so that the function is abled to be called without an instance of System
> Profile as parameter.

This is still confusing. I think this logic belongs in the callers, not in the
view.

[zmin, 2016-10-05 15:50:58]

On 2016/10/05 15:45:21, sky wrote:
> On 2016/10/04 19:02:57, zmin wrote:
> > On 2016/10/04 18:22:27, sky wrote:
> > >
> >
>
https://codereview.chromium.org/2392953002/diff/1/chrome/browser/ui/views/pro...
> > > File chrome/browser/ui/views/profiles/user_manager_view.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2392953002/diff/1/chrome/browser/ui/views/pro...
> > > chrome/browser/ui/views/profiles/user_manager_view.cc:292: if
> (browser_context
> > > == nullptr)
> > > This seems wrong to me. How is it that web_view_ would have the right
> > > BrowserContext, but it wouldn't be passed to this function?
> > 
> > UserManager always uses System Profile as BrowserContext while the Gaia
signin
> > dialog doesn't have to. So BrowserContext needs to be passed into the
function
> > as a parameter.
> > However, the web_view_'s BrowserContext can still be used as a default value
> > here so that the function is abled to be called without an instance of
System
> > Profile as parameter.
> 
> This is still confusing. I think this logic belongs in the callers, not in the
> view.

OK, I can move it to the caller.

[zmin, 2016-10-05 19:18:05]

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

[commit-bot: I haz the power, 2016-10-05 19:18:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[zmin, 2016-10-05 19:18:59]

On 2016/10/05 15:50:58, zmin wrote:
> On 2016/10/05 15:45:21, sky wrote:
> > On 2016/10/04 19:02:57, zmin wrote:
> > > On 2016/10/04 18:22:27, sky wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2392953002/diff/1/chrome/browser/ui/views/pro...
> > > > File chrome/browser/ui/views/profiles/user_manager_view.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2392953002/diff/1/chrome/browser/ui/views/pro...
> > > > chrome/browser/ui/views/profiles/user_manager_view.cc:292: if
> > (browser_context
> > > > == nullptr)
> > > > This seems wrong to me. How is it that web_view_ would have the right
> > > > BrowserContext, but it wouldn't be passed to this function?
> > > 
> > > UserManager always uses System Profile as BrowserContext while the Gaia
> signin
> > > dialog doesn't have to. So BrowserContext needs to be passed into the
> function
> > > as a parameter.
> > > However, the web_view_'s BrowserContext can still be used as a default
value
> > > here so that the function is abled to be called without an instance of
> System
> > > Profile as parameter.
> > 
> > This is still confusing. I think this logic belongs in the callers, not in
the
> > view.
> 
> OK, I can move it to the caller.

I just realized that the default value is not safe anyway because the system
profile creation is asyn. I have updated the code to move this logic to the
caller.

[commit-bot: I haz the power, 2016-10-05 21:14:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-05 21:14:21]

Dry run: This issue passed the CQ dry run.

[sky, 2016-10-06 16:02:08]

Please update the description of the cl too.

https://codereview.chromium.org/2392953002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/profile_helper.cc (right):

https://codereview.chromium.org/2392953002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/profile_helper.cc:36: #if !defined(OS_MACOSX)
Is it expected this branch (force signin enabled) does nothing if mac?

[zmin, 2016-10-06 16:38:16]

Description was changed from

==========
Display signin dialog for the default profile after the last profile being
deleted for force signin.

BUG=642059
==========

to

==========
Display signin dialog for the default profile after the last profile being
deleted for force signin. The signin dialog will use system profile as
browsercontext.

BUG=642059
==========

[zmin, 2016-10-06 16:38:31]

Description was changed from

==========
Display signin dialog for the default profile after the last profile being
deleted for force signin. The signin dialog will use system profile as
browsercontext.

BUG=642059
==========

to

==========
Display signin dialog for the default profile after the last profile being
deleted for force signin. The signin dialog will use system profile as its
browsercontext.

BUG=642059
==========

[zmin, 2016-10-06 16:42:08]

On 2016/10/06 16:02:08, sky wrote:
> Please update the description of the cl too.
> 
>
https://codereview.chromium.org/2392953002/diff/20001/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/profile_helper.cc (right):
> 
>
https://codereview.chromium.org/2392953002/diff/20001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/profile_helper.cc:36: #if !defined(OS_MACOSX)
> Is it expected this branch (force signin enabled) does nothing if mac?

No, the mac part is still under construction and it will be sent to code review
soon. Remove it from Mac right now to avoid Compiler Error.

[sky, 2016-10-06 21:19:38]

Update the description and LGTM

[zmin, 2016-10-06 21:24:45]

Description was changed from

==========
Display signin dialog for the default profile after the last profile being
deleted for force signin. The signin dialog will use system profile as its
browsercontext.

BUG=642059
==========

to

==========
Display signin dialog with system profile after the last profile being deleted
when force signin enabled.


BUG=642059
==========

[zmin, 2016-10-06 21:25:15]

zmin@chromium.org changed reviewers:
- anthonyvd@chromium.org

[zmin, 2016-10-07 15:48:32]

The CQ bit was checked by zmin@chromium.org

[commit-bot: I haz the power, 2016-10-07 15:48:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-07 16:30:10]

Description was changed from

==========
Display signin dialog with system profile after the last profile being deleted
when force signin enabled.


BUG=642059
==========

to

==========
Display signin dialog with system profile after the last profile being deleted
when force signin enabled.


BUG=642059
==========

[commit-bot: I haz the power, 2016-10-07 16:30:11]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-07 16:33:55]

Description was changed from

==========
Display signin dialog with system profile after the last profile being deleted
when force signin enabled.


BUG=642059
==========

to

==========
Display signin dialog with system profile after the last profile being deleted
when force signin enabled.

BUG=642059

Committed: https://crrev.com/438598fc3ff38423311a37341023690405bc34a0
Cr-Commit-Position: refs/heads/master@{#423879}
==========

[commit-bot: I haz the power, 2016-10-07 16:33:55]

Patchset 2 (id:??) landed as
https://crrev.com/438598fc3ff38423311a37341023690405bc34a0
Cr-Commit-Position: refs/heads/master@{#423879}