Override SigninManager::SignOut if force-signin is enabled.

Override SigninManager::SignOut if force-signin is enabled.

When force-signin is enabled, all browser windows will be closed before sign out. If window closing is aborted by the user, the signout will be aborted too. Otherwise, the profile will be signed out and locked. Then UserManager will be shown.
Also skip the sync confirmation dialog if user signs in with a corp account and creates a new profile for the it.

BUG=642059
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/965d98c5622ab9b542966730cba35dc530b7a7da
Cr-Commit-Position: refs/heads/master@{#429434}

[zmin, 2016-10-21 19:25:53]

Description was changed from

==========
Override SigninManager::SignOut if force-signin is enabled.

When force-signin is enabled, all browser windows will be closed before sign
out. If window closing is aborted by the user, the signout will be aborted too.
Otherwise, the profile will be signed out and locked. Then UserManager will be
shown.
Also skip the sync confirmation dialog if user signs in with a corp account and
creates a new profile for the it.


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

to

==========
Override SigninManager::SignOut if force-signin is enabled.

When force-signin is enabled, all browser windows will be closed before sign
out. If window closing is aborted by the user, the signout will be aborted too.
Otherwise, the profile will be signed out and locked. Then UserManager will be
shown.
Also skip the sync confirmation dialog if user signs in with a corp account and
creates a new profile for the it.


BUG=642059
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[zmin, 2016-10-21 19:27:30]

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

[commit-bot: I haz the power, 2016-10-21 19:28:10]

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-21 19:32:28]

zmin@chromium.org changed reviewers:
+ anthonyvd@chromium.org, rogerta@chromium.org, tommycli@chromium.org

[zmin, 2016-10-21 19:32:29]

tommycli@chromium.org: Please review changes in 
chrome/browser/resources/settings/people_page/people_page.js

rogerta@chromium.org: Please review changes in 
chrome/browser/signin/*
chrome/browser/ui/sync/*

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

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

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

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[zmin, 2016-10-21 20:26:02]

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

[commit-bot: I haz the power, 2016-10-21 20:26:48]

Dry run: 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-21 21:12:08]

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

[commit-bot: I haz the power, 2016-10-21 21:12:08]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[zmin, 2016-10-23 02:15:12]

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

[commit-bot: I haz the power, 2016-10-23 02:15:20]

Dry run: 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-23 02:30:54]

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

[commit-bot: I haz the power, 2016-10-23 02:30:55]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[zmin, 2016-10-23 03:29:46]

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

[commit-bot: I haz the power, 2016-10-23 03:29:56]

Dry run: 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-23 05:24:42]

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

[commit-bot: I haz the power, 2016-10-23 05:24:43]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[tommycli, 2016-10-25 18:20:22]

https://codereview.chromium.org/2442843002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/people_page.js (right):

https://codereview.chromium.org/2442843002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/people_page.js:232:
onDisconnectClosed_: function() {
This is fired when a Javascript dialog closes, so it can occur before all the
other browser windows close.

I think if you need to defer signout until after all the browser windows close,
that needs to be handled on the C++ side, and can't be done here in JavaScript.

I don't think you can determine if all the browser windows have closed in
JavaScript.

[zmin, 2016-10-25 18:52:12]

https://codereview.chromium.org/2442843002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/people_page.js (right):

https://codereview.chromium.org/2442843002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/people_page.js:232:
onDisconnectClosed_: function() {
On 2016/10/25 18:20:21, tommycli wrote:
> This is fired when a Javascript dialog closes, so it can occur before all the
> other browser windows close.
> 
> I think if you need to defer signout until after all the browser windows
close,
> that needs to be handled on the C++ side, and can't be done here in
JavaScript.
> 
> I don't think you can determine if all the browser windows have closed in
> JavaScript.

Browser window closing is triggered by signout when force signin enabled. 
Moving the signout here to make sure the window will only be closed after
settings.navigateToPreviousRoute() finished. So that when the browser is opened
next time, the session will be restored to chrome://md-settings but not
chrome://me-settings/signOut

[tommycli, 2016-10-25 19:03:30]

https://codereview.chromium.org/2442843002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/people_page.js (right):

https://codereview.chromium.org/2442843002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/people_page.js:232:
onDisconnectClosed_: function() {
On 2016/10/25 18:52:12, zmin wrote:
> On 2016/10/25 18:20:21, tommycli wrote:
> > This is fired when a Javascript dialog closes, so it can occur before all
the
> > other browser windows close.
> > 
> > I think if you need to defer signout until after all the browser windows
> close,
> > that needs to be handled on the C++ side, and can't be done here in
> JavaScript.
> > 
> > I don't think you can determine if all the browser windows have closed in
> > JavaScript.
> 
> Browser window closing is triggered by signout when force signin enabled. 
> Moving the signout here to make sure the window will only be closed after
> settings.navigateToPreviousRoute() finished. So that when the browser is
opened
> next time, the session will be restored to chrome://md-settings but not
> chrome://me-settings/signOut

I see. If that's all you need, I suggest within the onDisconnectConfirm_ method,
do:

// Navigate to previous route before signout so session restore does
// not restore to the signout dialog. Dialog will be automatically
// closed by the route change.
settings.navigateToPreviousRoute();
this.syncBrowserProxy_.signOut(deleteProfile);


And you can remove the this.$.disconnectDialog.close() call. See if that works.

[zmin, 2016-10-25 20:40:31]

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

[commit-bot: I haz the power, 2016-10-25 20:40:54]

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-25 20:41:12]

Patchset #5 (id:80001) has been deleted

[commit-bot: I haz the power, 2016-10-25 20:43:32]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[zmin, 2016-10-25 20:50:31]

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

[commit-bot: I haz the power, 2016-10-25 20:51:13]

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-25 20:52:17]

Patchset #5 (id:100001) has been deleted

[commit-bot: I haz the power, 2016-10-25 20:53:55]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[zmin, 2016-10-25 21:02:58]

https://codereview.chromium.org/2442843002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/people_page.js (right):

https://codereview.chromium.org/2442843002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/people_page.js:232:
onDisconnectClosed_: function() {
On 2016/10/25 19:03:30, tommycli wrote:
> On 2016/10/25 18:52:12, zmin wrote:
> > On 2016/10/25 18:20:21, tommycli wrote:
> > > This is fired when a Javascript dialog closes, so it can occur before all
> the
> > > other browser windows close.
> > > 
> > > I think if you need to defer signout until after all the browser windows
> > close,
> > > that needs to be handled on the C++ side, and can't be done here in
> > JavaScript.
> > > 
> > > I don't think you can determine if all the browser windows have closed in
> > > JavaScript.
> > 
> > Browser window closing is triggered by signout when force signin enabled. 
> > Moving the signout here to make sure the window will only be closed after
> > settings.navigateToPreviousRoute() finished. So that when the browser is
> opened
> > next time, the session will be restored to chrome://md-settings but not
> > chrome://me-settings/signOut
> 
> I see. If that's all you need, I suggest within the onDisconnectConfirm_
method,
> do:
> 
> // Navigate to previous route before signout so session restore does
> // not restore to the signout dialog. Dialog will be automatically
> // closed by the route change.
> settings.navigateToPreviousRoute();
> this.syncBrowserProxy_.signOut(deleteProfile);
> 
> 
> And you can remove the this.$.disconnectDialog.close() call. See if that
works.

Done.

[tommycli, 2016-10-25 21:04:00]

people_page lgtm (if the suggested changes work)

[zmin, 2016-10-25 21:05:27]

On 2016/10/25 21:04:00, tommycli wrote:
> people_page lgtm (if the suggested changes work)

Yes, it works. I have tested it.

[Roger Tawa OOO till Jul 10th, 2016-10-26 18:25:32]

See comments below for specific comments.

Overall though I don't believe creating a new subclass of SigninManager is the
right approach.  This functionality should go directly into SigninManager.

A few years ago the split of SigninManager into two classes, SigninManager and
SigninManagerBase caused a lot problems (i.e. crashes), specifically casting to
the derived class.  It made writing tests harder because they could not assume
which concrete class was in use.  This CL introduces the potential for the same
problems.

To keep signin componentized, ForceSigninManager (assuming we keep it) really
should live in //components/signin, not //chrome/browser/signin.  I suspect this
was required here because of the dependency on Profile* for the browser list. 
However, this functionality should be move to ChromeSigninClient instead, which
is the way that objects in //component/signin depend on and use Profile*.  That
is, ChromeSigninClient should do the work related to closing all windows and
then simply tell SigninManager when it is done.

https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
File chrome/browser/signin/force_signin_manager.cc (right):

https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
chrome/browser/signin/force_signin_manager.cc:58: }
I wonder if it is better to always call
BrowserList::CloseAllBrowsersWithProfile() here, and add the check for
system/guest profile inside DoSignOut() ?

https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
chrome/browser/signin/force_signin_manager.cc:73: }
This static cast seems dangerous.  How are we guaranteed that |signin_manager|
is always an instance of ForceSigninManager?  For example, what if the pref
value is changed after the profile is created?

https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
File chrome/browser/signin/force_signin_manager.h (right):

https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
chrome/browser/signin/force_signin_manager.h:34: // Next signout won't open
UserManager after all browser windows are closed.
Can you add a comment explaining why this is needed?

https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
chrome/browser/signin/force_signin_manager.h:34: // Next signout won't open
UserManager after all browser windows are closed.
If I understood correctly, this is called in only one place when creating a corp
profile. And in that case the next SignOut() is the very next function call. 
Better to add a new argument to SignOut() then add this function.

https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
File chrome/browser/signin/force_signin_manager_unittest.cc (right):

https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
chrome/browser/signin/force_signin_manager_unittest.cc:88: }
Should have tests for system profile and guest profile.

https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
File chrome/browser/signin/signin_manager_factory.cc (right):

https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
chrome/browser/signin/signin_manager_factory.cc:135:
GaiaCookieManagerServiceFactory::GetForProfile(profile));
Nit: add { and } around if and else blocks.

https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/ui/sync...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/ui/sync...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:323:
old_signin_manager);
Instead of calling this here, seems like it would be better to do inside of
CopyCredentialsFrom().

[zmin, 2016-10-26 23:16:08]

On 2016/10/26 18:25:32, Roger Tawa wrote:
> See comments below for specific comments.
> 
> Overall though I don't believe creating a new subclass of SigninManager is the
> right approach.  This functionality should go directly into SigninManager.
> 
> A few years ago the split of SigninManager into two classes, SigninManager and
> SigninManagerBase caused a lot problems (i.e. crashes), specifically casting
to
> the derived class.  It made writing tests harder because they could not assume
> which concrete class was in use.  This CL introduces the potential for the
same
> problems.
> 
> To keep signin componentized, ForceSigninManager (assuming we keep it) really
> should live in //components/signin, not //chrome/browser/signin.  I suspect
this
> was required here because of the dependency on Profile* for the browser list. 
> However, this functionality should be move to ChromeSigninClient instead,
which
> is the way that objects in //component/signin depend on and use Profile*. 
That
> is, ChromeSigninClient should do the work related to closing all windows and
> then simply tell SigninManager when it is done.
> 

I don't think ForceSigninManager should be moved to //components as it's a
feature of browser.
If you're worry about the risk of new subclass, moving the implementation into
ChromeSigninClient is a good idea.

How about I create two new API in SigninClient: PreSignedout and PostSignedout

PreSignedout will close all Browsers and call SigninManager::Signout as a
callback
PostSignedout will lock the profile and open UserManager.


>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
> File chrome/browser/signin/force_signin_manager.cc (right):
> 
>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
> chrome/browser/signin/force_signin_manager.cc:58: }
> I wonder if it is better to always call
> BrowserList::CloseAllBrowsersWithProfile() here, and add the check for
> system/guest profile inside DoSignOut() ?

The purpose of window closing is prevent user using Chrome without cloud policy
after Signout
There is no policy for System/Guest profile anyway, so I don't think it's
necessary to close the window here.


> 
>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
> chrome/browser/signin/force_signin_manager.cc:73: }
> This static cast seems dangerous.  How are we guaranteed that |signin_manager|
> is always an instance of ForceSigninManager?  For example, what if the pref
> value is changed after the profile is created?

The dynamic_refresh is set to false for this policy. Because there're some cases
hard to covered if the value is changed during runtime. 

>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
> File chrome/browser/signin/force_signin_manager.h (right):
> 
>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
> chrome/browser/signin/force_signin_manager.h:34: // Next signout won't open
> UserManager after all browser windows are closed.
> Can you add a comment explaining why this is needed?
> 
>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
> chrome/browser/signin/force_signin_manager.h:34: // Next signout won't open
> UserManager after all browser windows are closed.
> If I understood correctly, this is called in only one place when creating a
corp
> profile. And in that case the next SignOut() is the very next function call. 
> Better to add a new argument to SignOut() then add this function.
> 
There're multiple comments about this static method so I just response them
together:

Yes, static method is a ugly approach here. However, there're few reasons I
don't want to add an additional argument here.

1) This argument makes no sense without force signin
2) SigninManager should not aware of the UserManager which belongs to the Chrome

I don't have a good answer about how to disable UserManager::Show with the rule
above. 


>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
> File chrome/browser/signin/force_signin_manager_unittest.cc (right):
> 
>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
> chrome/browser/signin/force_signin_manager_unittest.cc:88: }
> Should have tests for system profile and guest profile.
Sure
> 
>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
> File chrome/browser/signin/signin_manager_factory.cc (right):
> 
>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
> chrome/browser/signin/signin_manager_factory.cc:135:
> GaiaCookieManagerServiceFactory::GetForProfile(profile));
> Nit: add { and } around if and else blocks.
> 
Done.

>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/ui/sync...
> File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):
> 
>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/ui/sync...
> chrome/browser/ui/sync/one_click_signin_sync_starter.cc:323:
> old_signin_manager);
> Instead of calling this here, seems like it would be better to do inside of
> CopyCredentialsFrom().
I put it here because it's related to Signout.

[Roger Tawa OOO till Jul 10th, 2016-10-27 14:44:01]

On 2016/10/26 23:16:08, zmin wrote:
> On 2016/10/26 18:25:32, Roger Tawa wrote:
> > See comments below for specific comments.
> > 
> > Overall though I don't believe creating a new subclass of SigninManager is
the
> > right approach.  This functionality should go directly into SigninManager.
> > 
> > A few years ago the split of SigninManager into two classes, SigninManager
and
> > SigninManagerBase caused a lot problems (i.e. crashes), specifically casting
> to
> > the derived class.  It made writing tests harder because they could not
assume
> > which concrete class was in use.  This CL introduces the potential for the
> same
> > problems.
> > 
> > To keep signin componentized, ForceSigninManager (assuming we keep it)
really
> > should live in //components/signin, not //chrome/browser/signin.  I suspect
> this
> > was required here because of the dependency on Profile* for the browser
list. 
> > However, this functionality should be move to ChromeSigninClient instead,
> which
> > is the way that objects in //component/signin depend on and use Profile*. 
> That
> > is, ChromeSigninClient should do the work related to closing all windows and
> > then simply tell SigninManager when it is done.
> > 
> 
> I don't think ForceSigninManager should be moved to //components as it's a
> feature of browser.
> If you're worry about the risk of new subclass, moving the implementation into
> ChromeSigninClient is a good idea.

Assuming we want to keep ForceSigninManager, which I don't think we do, it
definitely should be moved to //components.  The model used to divide generic
features from browser specific features is to use a delegate interface, which in
this case is SigninClient.  So moving the code that accesses the profile and
handles closing the windows and such should be moved to ChromeSigninClient.

As for whether to keep ForceSigninManager, once you've move the code to
ChromeSigninClient and written more tests, we'll see if it is still required.

> 
> How about I create two new API in SigninClient: PreSignedout and PostSignedout
> 
> PreSignedout will close all Browsers and call SigninManager::Signout as a
> callback
> PostSignedout will lock the profile and open UserManager.

We already have a post signout method: SigninClient::SignOut().  You can add a
pre signout.

> 
> 
> >
>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
> > File chrome/browser/signin/force_signin_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
> > chrome/browser/signin/force_signin_manager.cc:58: }
> > I wonder if it is better to always call
> > BrowserList::CloseAllBrowsersWithProfile() here, and add the check for
> > system/guest profile inside DoSignOut() ?
> 
> The purpose of window closing is prevent user using Chrome without cloud
policy
> after Signout
> There is no policy for System/Guest profile anyway, so I don't think it's
> necessary to close the window here.

(Please reply to the specific comment in the code.  Much easier to follow the
discussion that way.  Right now it does not appear like you addressed any of my
specific comments, even though you did).

> 
> 
> > 
> >
>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
> > chrome/browser/signin/force_signin_manager.cc:73: }
> > This static cast seems dangerous.  How are we guaranteed that
|signin_manager|
> > is always an instance of ForceSigninManager?  For example, what if the pref
> > value is changed after the profile is created?
> 
> The dynamic_refresh is set to false for this policy. Because there're some
cases
> hard to covered if the value is changed during runtime. 
> 
> >
>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
> > File chrome/browser/signin/force_signin_manager.h (right):
> > 
> >
>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
> > chrome/browser/signin/force_signin_manager.h:34: // Next signout won't open
> > UserManager after all browser windows are closed.
> > Can you add a comment explaining why this is needed?
> > 
> >
>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
> > chrome/browser/signin/force_signin_manager.h:34: // Next signout won't open
> > UserManager after all browser windows are closed.
> > If I understood correctly, this is called in only one place when creating a
> corp
> > profile. And in that case the next SignOut() is the very next function call.

> > Better to add a new argument to SignOut() then add this function.
> > 
> There're multiple comments about this static method so I just response them
> together:
> 
> Yes, static method is a ugly approach here. However, there're few reasons I
> don't want to add an additional argument here.
> 
> 1) This argument makes no sense without force signin
> 2) SigninManager should not aware of the UserManager which belongs to the
Chrome
> 
> I don't have a good answer about how to disable UserManager::Show with the
rule
> above. 
> 
> 
> >
>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
> > File chrome/browser/signin/force_signin_manager_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
> > chrome/browser/signin/force_signin_manager_unittest.cc:88: }
> > Should have tests for system profile and guest profile.
> Sure
> > 
> >
>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
> > File chrome/browser/signin/signin_manager_factory.cc (right):
> > 
> >
>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
> > chrome/browser/signin/signin_manager_factory.cc:135:
> > GaiaCookieManagerServiceFactory::GetForProfile(profile));
> > Nit: add { and } around if and else blocks.
> > 
> Done.
> 
> >
>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/ui/sync...
> > File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):
> > 
> >
>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/ui/sync...
> > chrome/browser/ui/sync/one_click_signin_sync_starter.cc:323:
> > old_signin_manager);
> > Instead of calling this here, seems like it would be better to do inside of
> > CopyCredentialsFrom().
> I put it here because it's related to Signout.

[Roger Tawa OOO till Jul 10th, 2016-10-27 14:44:01]

On 2016/10/26 23:16:08, zmin wrote:
> On 2016/10/26 18:25:32, Roger Tawa wrote:
> > See comments below for specific comments.
> > 
> > Overall though I don't believe creating a new subclass of SigninManager is
the
> > right approach.  This functionality should go directly into SigninManager.
> > 
> > A few years ago the split of SigninManager into two classes, SigninManager
and
> > SigninManagerBase caused a lot problems (i.e. crashes), specifically casting
> to
> > the derived class.  It made writing tests harder because they could not
assume
> > which concrete class was in use.  This CL introduces the potential for the
> same
> > problems.
> > 
> > To keep signin componentized, ForceSigninManager (assuming we keep it)
really
> > should live in //components/signin, not //chrome/browser/signin.  I suspect
> this
> > was required here because of the dependency on Profile* for the browser
list. 
> > However, this functionality should be move to ChromeSigninClient instead,
> which
> > is the way that objects in //component/signin depend on and use Profile*. 
> That
> > is, ChromeSigninClient should do the work related to closing all windows and
> > then simply tell SigninManager when it is done.
> > 
> 
> I don't think ForceSigninManager should be moved to //components as it's a
> feature of browser.
> If you're worry about the risk of new subclass, moving the implementation into
> ChromeSigninClient is a good idea.

Assuming we want to keep ForceSigninManager, which I don't think we do, it
definitely should be moved to //components.  The model used to divide generic
features from browser specific features is to use a delegate interface, which in
this case is SigninClient.  So moving the code that accesses the profile and
handles closing the windows and such should be moved to ChromeSigninClient.

As for whether to keep ForceSigninManager, once you've move the code to
ChromeSigninClient and written more tests, we'll see if it is still required.

> 
> How about I create two new API in SigninClient: PreSignedout and PostSignedout
> 
> PreSignedout will close all Browsers and call SigninManager::Signout as a
> callback
> PostSignedout will lock the profile and open UserManager.

We already have a post signout method: SigninClient::SignOut().  You can add a
pre signout.

> 
> 
> >
>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
> > File chrome/browser/signin/force_signin_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
> > chrome/browser/signin/force_signin_manager.cc:58: }
> > I wonder if it is better to always call
> > BrowserList::CloseAllBrowsersWithProfile() here, and add the check for
> > system/guest profile inside DoSignOut() ?
> 
> The purpose of window closing is prevent user using Chrome without cloud
policy
> after Signout
> There is no policy for System/Guest profile anyway, so I don't think it's
> necessary to close the window here.

(Please reply to the specific comment in the code.  Much easier to follow the
discussion that way.  Right now it does not appear like you addressed any of my
specific comments, even though you did).

> 
> 
> > 
> >
>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
> > chrome/browser/signin/force_signin_manager.cc:73: }
> > This static cast seems dangerous.  How are we guaranteed that
|signin_manager|
> > is always an instance of ForceSigninManager?  For example, what if the pref
> > value is changed after the profile is created?
> 
> The dynamic_refresh is set to false for this policy. Because there're some
cases
> hard to covered if the value is changed during runtime. 
> 
> >
>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
> > File chrome/browser/signin/force_signin_manager.h (right):
> > 
> >
>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
> > chrome/browser/signin/force_signin_manager.h:34: // Next signout won't open
> > UserManager after all browser windows are closed.
> > Can you add a comment explaining why this is needed?
> > 
> >
>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
> > chrome/browser/signin/force_signin_manager.h:34: // Next signout won't open
> > UserManager after all browser windows are closed.
> > If I understood correctly, this is called in only one place when creating a
> corp
> > profile. And in that case the next SignOut() is the very next function call.

> > Better to add a new argument to SignOut() then add this function.
> > 
> There're multiple comments about this static method so I just response them
> together:
> 
> Yes, static method is a ugly approach here. However, there're few reasons I
> don't want to add an additional argument here.
> 
> 1) This argument makes no sense without force signin
> 2) SigninManager should not aware of the UserManager which belongs to the
Chrome
> 
> I don't have a good answer about how to disable UserManager::Show with the
rule
> above. 
> 
> 
> >
>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
> > File chrome/browser/signin/force_signin_manager_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
> > chrome/browser/signin/force_signin_manager_unittest.cc:88: }
> > Should have tests for system profile and guest profile.
> Sure
> > 
> >
>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
> > File chrome/browser/signin/signin_manager_factory.cc (right):
> > 
> >
>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/signin/...
> > chrome/browser/signin/signin_manager_factory.cc:135:
> > GaiaCookieManagerServiceFactory::GetForProfile(profile));
> > Nit: add { and } around if and else blocks.
> > 
> Done.
> 
> >
>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/ui/sync...
> > File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):
> > 
> >
>
https://codereview.chromium.org/2442843002/diff/120001/chrome/browser/ui/sync...
> > chrome/browser/ui/sync/one_click_signin_sync_starter.cc:323:
> > old_signin_manager);
> > Instead of calling this here, seems like it would be better to do inside of
> > CopyCredentialsFrom().
> I put it here because it's related to Signout.

[zmin, 2016-10-28 18:47:38]

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

[commit-bot: I haz the power, 2016-10-28 18:48:16]

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-28 18:49:34]

Hi Roger,

I have refactored the code and moved implementation to ChromeSigninClient.
Please review again.


Thanks,
Owen

[zmin, 2016-10-28 19:06:40]

Patchset #6 (id:140001) has been deleted

[commit-bot: I haz the power, 2016-10-28 19:07:02]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[zmin, 2016-10-28 19:10:20]

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

[commit-bot: I haz the power, 2016-10-28 19:10:47]

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-28 19:24:16]

Patchset #6 (id:160001) has been deleted

[zmin, 2016-10-28 19:25:51]

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

[commit-bot: I haz the power, 2016-10-28 19:26:27]

Dry run: 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-28 20:38:34]

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

[commit-bot: I haz the power, 2016-10-28 20:38:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[zmin, 2016-10-28 22:37:29]

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

[commit-bot: I haz the power, 2016-10-28 22:37:50]

Dry run: 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-28 22:55:57]

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

[commit-bot: I haz the power, 2016-10-28 22:55:58]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[zmin, 2016-10-28 23:41:32]

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

[commit-bot: I haz the power, 2016-10-28 23:41:52]

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-29 00:18:30]

Patchset #7 (id:200001) has been deleted

[commit-bot: I haz the power, 2016-10-29 00:27:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2016-10-29 01:34:23]

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

[commit-bot: I haz the power, 2016-10-29 01:34:24]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[zmin, 2016-10-31 16:27:45]

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

[commit-bot: I haz the power, 2016-10-31 16:28:02]

Dry run: 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-31 16:58:06]

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

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

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[zmin, 2016-10-31 17:13:31]

Patchset #7 (id:220001) has been deleted

[zmin, 2016-11-01 15:42:47]

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

[commit-bot: I haz the power, 2016-11-01 15:43:06]

Dry run: 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-11-01 16:46:35]

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

[commit-bot: I haz the power, 2016-11-01 16:46:36]

Dry run: This issue passed the CQ dry run.

[tommycli, 2016-11-01 17:28:55]

https://codereview.chromium.org/2442843002/diff/240001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/people_page.js (right):

https://codereview.chromium.org/2442843002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/people_page.js:254:
setTimeout(function() {
Thinking further on this: I think using setTimeout here is wrong. We have to
wait until the browse has finished its Back navigation.

So the right thing to do would be:

window.addEventListener('popstate', function callback() {
  window.removeEventListener('popstate', callback);
  this.syncBrowserProxy_.signOut(deleteProfile);
}.bind(this));

See if that fixes your tests too.

[Dan Beam, 2016-11-01 18:51:37]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2016-11-01 18:51:38]

https://codereview.chromium.org/2442843002/diff/240001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/people_page.js (right):

https://codereview.chromium.org/2442843002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/people_page.js:254:
setTimeout(function() {
On 2016/11/01 17:28:54, tommycli wrote:
> Thinking further on this: I think using setTimeout here is wrong. We have to
> wait until the browse has finished its Back navigation.
> 
> So the right thing to do would be:
> 
> window.addEventListener('popstate', function callback() {
>   window.removeEventListener('popstate', callback);
>   this.syncBrowserProxy_.signOut(deleteProfile);
> }.bind(this));
> 
> See if that fixes your tests too.

see also: listenOnce()

[Dan Beam, 2016-11-01 18:51:50]

Description was changed from

==========
Override SigninManager::SignOut if force-signin is enabled.

When force-signin is enabled, all browser windows will be closed before sign
out. If window closing is aborted by the user, the signout will be aborted too.
Otherwise, the profile will be signed out and locked. Then UserManager will be
shown.
Also skip the sync confirmation dialog if user signs in with a corp account and
creates a new profile for the it.


BUG=642059
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Override SigninManager::SignOut if force-signin is enabled.

When force-signin is enabled, all browser windows will be closed before sign
out. If window closing is aborted by the user, the signout will be aborted too.
Otherwise, the profile will be signed out and locked. Then UserManager will be
shown.
Also skip the sync confirmation dialog if user signs in with a corp account and
creates a new profile for the it.


BUG=642059
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2016-11-01 18:51:50]

dbeam@chromium.org changed reviewers:
- dbeam@chromium.org

[zmin, 2016-11-01 19:32:51]

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

[commit-bot: I haz the power, 2016-11-01 19:33:21]

Dry run: 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-11-01 20:34:47]

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

[commit-bot: I haz the power, 2016-11-01 20:34:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[zmin, 2016-11-01 20:55:23]

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

[commit-bot: I haz the power, 2016-11-01 20:56:10]

Dry run: 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-11-01 21:42:08]

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

[commit-bot: I haz the power, 2016-11-01 21:42:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[zmin, 2016-11-01 23:30:33]

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

[commit-bot: I haz the power, 2016-11-01 23:31:06]

Dry run: 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-11-01 23:35:15]

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

[commit-bot: I haz the power, 2016-11-01 23:35:17]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[zmin, 2016-11-02 00:12:26]

Patchset #10 (id:300001) has been deleted

[zmin, 2016-11-02 00:31:43]

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

[commit-bot: I haz the power, 2016-11-02 00:33:04]

Dry run: 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-11-02 01:37:23]

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

[commit-bot: I haz the power, 2016-11-02 01:37:24]

Dry run: This issue passed the CQ dry run.

[Roger Tawa OOO till Jul 10th, 2016-11-02 13:20:40]

lgtm with a few comments below.  Thanks.

https://codereview.chromium.org/2442843002/diff/320001/chrome/browser/signin/...
File chrome/browser/signin/chrome_signin_client.cc (right):

https://codereview.chromium.org/2442843002/diff/320001/chrome/browser/signin/...
chrome/browser/signin/chrome_signin_client.cc:202:
entry->SetIsSigninRequired(false);
Can we call LockProfile() from here to reduce code duplication?  Would need a
new bool arg to method.

https://codereview.chromium.org/2442843002/diff/320001/chrome/browser/signin/...
chrome/browser/signin/chrome_signin_client.cc:429: is_user_manager_displayed_ =
false;
Nit: since comment is below the if() statement, I'd put { and } around the
block.

https://codereview.chromium.org/2442843002/diff/320001/chrome/browser/signin/...
chrome/browser/signin/chrome_signin_client.cc:442: is_user_manager_displayed_ =
true;
Maybe this member could be renamed should_display_user_manager_ ?

Why does it it need to be set at line 442?  I think  OnCredentialsBeingCopied()
can be called but OnCloseBrowsersSuccess() is not, like when aborted.  Would it
be better instead to set it somewhere else?

https://codereview.chromium.org/2442843002/diff/320001/chrome/browser/signin/...
File chrome/browser/signin/chrome_signin_client_unittest.cc (right):

https://codereview.chromium.org/2442843002/diff/320001/chrome/browser/signin/...
chrome/browser/signin/chrome_signin_client_unittest.cc:211:
EXPECT_CALL(*client_, ShowUserManager(browser()->profile()->GetPath()))
Do you need to check and reset the expectations before this call?

https://codereview.chromium.org/2442843002/diff/320001/chrome/browser/signin/...
chrome/browser/signin/chrome_signin_client_unittest.cc:256: #endif
Please add comments to the #endifs.

https://codereview.chromium.org/2442843002/diff/320001/chrome/browser/ui/sync...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

https://codereview.chromium.org/2442843002/diff/320001/chrome/browser/ui/sync...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:335:
entry->SetIsSigninRequired(false);
Could we try to reuse the LockProfile() method you added to the signin client? 
Probably means adding a LockProfile() method to the SigninClient interface.

https://codereview.chromium.org/2442843002/diff/320001/components/signin/core...
File components/signin/core/browser/signin_client.h (right):

https://codereview.chromium.org/2442843002/diff/320001/components/signin/core...
components/signin/core/browser/signin_client.h:128: virtual void
OnCredentialsBeingCopied() {}
Could this be renamed to OnCredentialsCopied() ?  With "Being" in the name it's
not clear if this is called before, during, or after the copy.  Could also
replace "once" with "after" too.

[tommycli, 2016-11-02 18:46:57]

not lgtm. really sorry to do this...

https://codereview.chromium.org/2442843002/diff/320001/chrome/browser/resourc...
File chrome/browser/resources/settings/route.js (right):

https://codereview.chromium.org/2442843002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:353: if
(isPreviousRouteExistedInHistory())
I'm sorry - but I just don't think this is a good idea --

The core idea of the CL is that we need to remove chrome://settings/signOut from
the Session Restore list after a Force signout.

With these changes, it's not obvious how this is accomplished - future
developers would have to disentangle how the navigable dialog interacts with the
routing.

I think it would be much better to discard chrome://settings/signOut from the
list of Session Restored URLs - or redirect to chrome://settings

At least - we should give an effort to investigate how complicated that is. I
think the above would be much simpler.

There is already precedent in the Session Restore code for treating internal
URLs differently:
https://cs.chromium.org/chromium/src/chrome/browser/sessions/session_restore_...

[zmin, 2016-11-02 19:26:00]

On 2016/11/02 18:46:57, tommycli wrote:
> not lgtm. really sorry to do this...
> 
>
https://codereview.chromium.org/2442843002/diff/320001/chrome/browser/resourc...
> File chrome/browser/resources/settings/route.js (right):
> 
>
https://codereview.chromium.org/2442843002/diff/320001/chrome/browser/resourc...
> chrome/browser/resources/settings/route.js:353: if
> (isPreviousRouteExistedInHistory())
> I'm sorry - but I just don't think this is a good idea --
> 
> The core idea of the CL is that we need to remove chrome://settings/signOut
from
> the Session Restore list after a Force signout.
> 
> With these changes, it's not obvious how this is accomplished - future
> developers would have to disentangle how the navigable dialog interacts with
the
> routing.
> 
> I think it would be much better to discard chrome://settings/signOut from the
> list of Session Restored URLs - or redirect to chrome://settings
> 
> At least - we should give an effort to investigate how complicated that is. I
> think the above would be much simpler.
> 
> There is already precedent in the Session Restore code for treating internal
> URLs differently:
>
https://cs.chromium.org/chromium/src/chrome/browser/sessions/session_restore_...

Talk with @tommycli offline, we agree that session restore is a better approach
here. I'll remove the webui change from this CL and do the session restore in a
separate one.

[zmin, 2016-11-02 20:29:22]

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

[commit-bot: I haz the power, 2016-11-02 20:30:04]

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

[zmin, 2016-11-02 20:30:22]

https://codereview.chromium.org/2442843002/diff/320001/chrome/browser/signin/...
File chrome/browser/signin/chrome_signin_client.cc (right):

https://codereview.chromium.org/2442843002/diff/320001/chrome/browser/signin/...
chrome/browser/signin/chrome_signin_client.cc:202:
entry->SetIsSigninRequired(false);
On 2016/11/02 13:20:39, Roger Tawa wrote:
> Can we call LockProfile() from here to reduce code duplication?  Would need a
> new bool arg to method.

OnSignedOut is not always called during signout. For example, it won't be called
if user give up sign in by the manager confirm dialog.

https://codereview.chromium.org/2442843002/diff/320001/chrome/browser/signin/...
chrome/browser/signin/chrome_signin_client.cc:429: is_user_manager_displayed_ =
false;
On 2016/11/02 13:20:39, Roger Tawa wrote:
> Nit: since comment is below the if() statement, I'd put { and } around the
> block.

Done.

https://codereview.chromium.org/2442843002/diff/320001/chrome/browser/signin/...
chrome/browser/signin/chrome_signin_client.cc:442: is_user_manager_displayed_ =
true;
On 2016/11/02 13:20:39, Roger Tawa wrote:
> Maybe this member could be renamed should_display_user_manager_ ?
> 
> Why does it it need to be set at line 442?  I think 
OnCredentialsBeingCopied()
> can be called but OnCloseBrowsersSuccess() is not, like when aborted.  Would
it
> be better instead to set it somewhere else?

Done.

Good point. Because displaying UserManager is in the end of sign out process,
I'll just reset this flag in the abort callback as well.

https://codereview.chromium.org/2442843002/diff/320001/chrome/browser/signin/...
File chrome/browser/signin/chrome_signin_client_unittest.cc (right):

https://codereview.chromium.org/2442843002/diff/320001/chrome/browser/signin/...
chrome/browser/signin/chrome_signin_client_unittest.cc:211:
EXPECT_CALL(*client_, ShowUserManager(browser()->profile()->GetPath()))
On 2016/11/02 13:20:39, Roger Tawa wrote:
> Do you need to check and reset the expectations before this call?

Done.

https://codereview.chromium.org/2442843002/diff/320001/chrome/browser/signin/...
chrome/browser/signin/chrome_signin_client_unittest.cc:256: #endif
On 2016/11/02 13:20:39, Roger Tawa wrote:
> Please add comments to the #endifs.

Done.

https://codereview.chromium.org/2442843002/diff/320001/chrome/browser/ui/sync...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

https://codereview.chromium.org/2442843002/diff/320001/chrome/browser/ui/sync...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:335:
entry->SetIsSigninRequired(false);
On 2016/11/02 13:20:39, Roger Tawa wrote:
> Could we try to reuse the LockProfile() method you added to the signin client?

> Probably means adding a LockProfile() method to the SigninClient interface.

In the next CL(coming soon). The way of lock/unlock profile with force signin
enabled will be different. It may not be simple to reuse LockProfile here after
that change.

But yes, I do agree that the way we set ProfileAttributreseEntry is a bit
complicated. Maybe we shall create an util function to do that.

https://codereview.chromium.org/2442843002/diff/320001/components/signin/core...
File components/signin/core/browser/signin_client.h (right):

https://codereview.chromium.org/2442843002/diff/320001/components/signin/core...
components/signin/core/browser/signin_client.h:128: virtual void
OnCredentialsBeingCopied() {}
On 2016/11/02 13:20:39, Roger Tawa wrote:
> Could this be renamed to OnCredentialsCopied() ?  With "Being" in the name
it's
> not clear if this is called before, during, or after the copy.  Could also
> replace "once" with "after" too.

Done.

[zmin, 2016-11-02 20:31:00]

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

[tommycli, 2016-11-02 20:32:00]

lgtm since JavaScript changes are gone. I did not review the C++ changes.

[Roger Tawa OOO till Jul 10th, 2016-11-02 20:42:12]

C++ changes lgtm

Yeah would be good in next CL to have a helper function to lock/unlock a profile
to reduce code duplication.

[zmin, 2016-11-02 21:57:17]

The CQ bit was unchecked by zmin@chromium.org

[zmin, 2016-11-02 21:57:30]

The CQ bit was checked by zmin@chromium.org

[commit-bot: I haz the power, 2016-11-02 21:58:10]

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-11-02 22:50:57]

Description was changed from

==========
Override SigninManager::SignOut if force-signin is enabled.

When force-signin is enabled, all browser windows will be closed before sign
out. If window closing is aborted by the user, the signout will be aborted too.
Otherwise, the profile will be signed out and locked. Then UserManager will be
shown.
Also skip the sync confirmation dialog if user signs in with a corp account and
creates a new profile for the it.


BUG=642059
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Override SigninManager::SignOut if force-signin is enabled.

When force-signin is enabled, all browser windows will be closed before sign
out. If window closing is aborted by the user, the signout will be aborted too.
Otherwise, the profile will be signed out and locked. Then UserManager will be
shown.
Also skip the sync confirmation dialog if user signs in with a corp account and
creates a new profile for the it.


BUG=642059
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-11-02 22:51:00]

Committed patchset #11 (id:340001)

[commit-bot: I haz the power, 2016-11-02 22:54:29]

Description was changed from

==========
Override SigninManager::SignOut if force-signin is enabled.

When force-signin is enabled, all browser windows will be closed before sign
out. If window closing is aborted by the user, the signout will be aborted too.
Otherwise, the profile will be signed out and locked. Then UserManager will be
shown.
Also skip the sync confirmation dialog if user signs in with a corp account and
creates a new profile for the it.


BUG=642059
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Override SigninManager::SignOut if force-signin is enabled.

When force-signin is enabled, all browser windows will be closed before sign
out. If window closing is aborted by the user, the signout will be aborted too.
Otherwise, the profile will be signed out and locked. Then UserManager will be
shown.
Also skip the sync confirmation dialog if user signs in with a corp account and
creates a new profile for the it.

BUG=642059
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/965d98c5622ab9b542966730cba35dc530b7a7da
Cr-Commit-Position: refs/heads/master@{#429434}
==========

[commit-bot: I haz the power, 2016-11-02 22:54:31]

Patchset 11 (id:??) landed as
https://crrev.com/965d98c5622ab9b542966730cba35dc530b7a7da
Cr-Commit-Position: refs/heads/master@{#429434}