[sync] Sign user out of Chrome on dashboard stop and reset

[sync] Sign user out of Chrome on dashboard stop and reset

Currently every platform except Android and ChromeOS log out of chrome upon a
dashboard stop and clear. This CL removes the special case for Android and adds
some machinery to notify Android Java code that sign out occurred. This
notification prompts Android's SigninManager to clear Android specific state.

BUG=596611
R=maxbogue@chromium.org

Committed: https://crrev.com/359ced3af82ddfc61114a801c62b1da4c79e7565
Cr-Commit-Position: refs/heads/master@{#395099}

[Patrick Noland, 2016-05-16 20:32:29]

Patchset #1 (id:1) has been deleted

[Patrick Noland, 2016-05-16 20:37:51]

Description was changed from

==========
[sync] Sign user out of Chrome on dashboard stop and reset

BUG=596611
==========

to

==========
[sync] Sign user out of Chrome on dashboard stop and reset

Currently every platform except Android and ChromeOS log out of chrome upon a
dashboard stop and clear. This CL removes the special case for Android and adds
some machinery to notify Android Java code that sign out occurred. This
notification prompts Android's SigninManager to clear Android specific state.

BUG=596611

R=mbogue@chromium.org
==========

[Patrick Noland, 2016-05-16 20:37:52]

pnoland@chromium.org changed reviewers:
+ mbogue@chromium.org

[Patrick Noland, 2016-05-16 20:37:52]

pnoland@chromium.org changed required reviewers:
+ mbogue@chromium.org

[Patrick Noland, 2016-05-16 20:38:57]

Description was changed from

==========
[sync] Sign user out of Chrome on dashboard stop and reset

Currently every platform except Android and ChromeOS log out of chrome upon a
dashboard stop and clear. This CL removes the special case for Android and adds
some machinery to notify Android Java code that sign out occurred. This
notification prompts Android's SigninManager to clear Android specific state.

BUG=596611

R=mbogue@chromium.org
==========

to

==========
[sync] Sign user out of Chrome on dashboard stop and reset

Currently every platform except Android and ChromeOS log out of chrome upon a
dashboard stop and clear. This CL removes the special case for Android and adds
some machinery to notify Android Java code that sign out occurred. This
notification prompts Android's SigninManager to clear Android specific state.

BUG=596611

R=mbogue@chromium.org
==========

[Patrick Noland, 2016-05-16 20:56:23]

pnoland@chromium.org changed reviewers:
+ maxbogue@chromium.org
- mbogue@chromium.org

[Patrick Noland, 2016-05-16 20:56:23]

pnoland@chromium.org changed required reviewers:
+ maxbogue@chromium.org
- mbogue@chromium.org

[Patrick Noland, 2016-05-16 20:56:50]

Description was changed from

==========
[sync] Sign user out of Chrome on dashboard stop and reset

Currently every platform except Android and ChromeOS log out of chrome upon a
dashboard stop and clear. This CL removes the special case for Android and adds
some machinery to notify Android Java code that sign out occurred. This
notification prompts Android's SigninManager to clear Android specific state.

BUG=596611

R=mbogue@chromium.org
==========

to

==========
[sync] Sign user out of Chrome on dashboard stop and reset

Currently every platform except Android and ChromeOS log out of chrome upon a
dashboard stop and clear. This CL removes the special case for Android and adds
some machinery to notify Android Java code that sign out occurred. This
notification prompts Android's SigninManager to clear Android specific state.

BUG=596611

R=maxbogue@chromium.org
==========

[maxbogue, 2016-05-16 22:51:02]

Mostly lg, just a couple comments.

https://codereview.chromium.org/1976423002/diff/20001/chrome/android/sync_she...
File
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/SyncTest.java
(right):

https://codereview.chromium.org/1976423002/diff/20001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/SyncTest.java:71:
SyncTestUtil.verifySyncIsActiveForAccount(mContext, account);
setUpTestAccountAndSignInToSync() does this already:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/syn...

https://codereview.chromium.org/1976423002/diff/20001/chrome/browser/android/...
File chrome/browser/android/signin/signin_manager_android.h (right):

https://codereview.chromium.org/1976423002/diff/20001/chrome/browser/android/...
chrome/browser/android/signin/signin_manager_android.h:17: #include
"components/signin/core/browser/signin_manager.h"
signin_manager_base.h, since that's the class you reference.

https://codereview.chromium.org/1976423002/diff/20001/chrome/browser/android/...
chrome/browser/android/signin/signin_manager_android.h:80: void
GoogleSigninFailed(const GoogleServiceAuthError& error) override;
For implementations of interfaces, put a comment like "//
SigninManagerBase::Observer implementation" above the block and remove the
newlines between functions. Example:
https://code.google.com/p/chromium/codesearch#chromium/src/components/browser...

https://codereview.chromium.org/1976423002/diff/20001/components/browser_sync...
File components/browser_sync/browser/profile_sync_service_unittest.cc (right):

https://codereview.chromium.org/1976423002/diff/20001/components/browser_sync...
components/browser_sync/browser/profile_sync_service_unittest.cc:929:
EXPECT_TRUE(signin_manager()->GetAuthenticatedAccountId().empty());
You could move the #if inside the test and have an #else that tests we do the
right thing for ChromeOS (not sign out) as well.

[Patrick Noland, 2016-05-17 20:19:15]

Patchset #2 (id:40001) has been deleted

[Patrick Noland, 2016-05-17 20:19:24]

Patchset #2 (id:60001) has been deleted

[Patrick Noland, 2016-05-17 21:30:07]

https://codereview.chromium.org/1976423002/diff/20001/chrome/android/sync_she...
File
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/SyncTest.java
(right):

https://codereview.chromium.org/1976423002/diff/20001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/SyncTest.java:71:
SyncTestUtil.verifySyncIsActiveForAccount(mContext, account);
On 2016/05/16 22:51:02, maxbogue wrote:
> setUpTestAccountAndSignInToSync() does this already:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/syn...

Done.

https://codereview.chromium.org/1976423002/diff/20001/chrome/browser/android/...
File chrome/browser/android/signin/signin_manager_android.h (right):

https://codereview.chromium.org/1976423002/diff/20001/chrome/browser/android/...
chrome/browser/android/signin/signin_manager_android.h:17: #include
"components/signin/core/browser/signin_manager.h"
On 2016/05/16 22:51:02, maxbogue wrote:
> signin_manager_base.h, since that's the class you reference.

Done.

https://codereview.chromium.org/1976423002/diff/20001/chrome/browser/android/...
chrome/browser/android/signin/signin_manager_android.h:80: void
GoogleSigninFailed(const GoogleServiceAuthError& error) override;
On 2016/05/16 22:51:02, maxbogue wrote:
> For implementations of interfaces, put a comment like "//
> SigninManagerBase::Observer implementation" above the block and remove the
> newlines between functions. Example:
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/browser...

Done.

https://codereview.chromium.org/1976423002/diff/20001/components/browser_sync...
File components/browser_sync/browser/profile_sync_service_unittest.cc (right):

https://codereview.chromium.org/1976423002/diff/20001/components/browser_sync...
components/browser_sync/browser/profile_sync_service_unittest.cc:929:
EXPECT_TRUE(signin_manager()->GetAuthenticatedAccountId().empty());
On 2016/05/16 22:51:02, maxbogue wrote:
> You could move the #if inside the test and have an #else that tests we do the
> right thing for ChromeOS (not sign out) as well.

Done.

[maxbogue, 2016-05-18 18:23:43]

https://codereview.chromium.org/1976423002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java
(right):

https://codereview.chromium.org/1976423002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java:598:
if (ChromeSigninController.get(mContext).isSignedIn() && mSignOutCallback ==
null) {
I believe this code will cause signOut to get called twice for any Java signOut
calls without a callback, since the native call happens before
ChromeSigninController is cleared, but it doesn't loop because the native
SigninManager short-circuits after the first call.. Could you add an
isSignOutInProgress flag to SigninManager that we check here instead of these
two conditions?

https://codereview.chromium.org/1976423002/diff/80001/components/browser_sync...
File components/browser_sync/browser/profile_sync_service_unittest.cc (right):

https://codereview.chromium.org/1976423002/diff/80001/components/browser_sync...
components/browser_sync/browser/profile_sync_service_unittest.cc:926: #if
!defined(OS_CHROMEOS)
nit: preprocessor directives are always at the beginning of lines, even inside
functions:
https://google.github.io/styleguide/cppguide.html#Preprocessor_Directives

Here and the two below.

[Patrick Noland, 2016-05-18 22:33:27]

Patchset #3 (id:100001) has been deleted

[Patrick Noland, 2016-05-18 23:26:06]

https://codereview.chromium.org/1976423002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java
(right):

https://codereview.chromium.org/1976423002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java:598:
if (ChromeSigninController.get(mContext).isSignedIn() && mSignOutCallback ==
null) {
On 2016/05/18 18:23:43, maxbogue wrote:
> I believe this code will cause signOut to get called twice for any Java
signOut
> calls without a callback, since the native call happens before
> ChromeSigninController is cleared, but it doesn't loop because the native
> SigninManager short-circuits after the first call.. Could you add an
> isSignOutInProgress flag to SigninManager that we check here instead of these
> two conditions?

Done.

https://codereview.chromium.org/1976423002/diff/80001/components/browser_sync...
File components/browser_sync/browser/profile_sync_service_unittest.cc (right):

https://codereview.chromium.org/1976423002/diff/80001/components/browser_sync...
components/browser_sync/browser/profile_sync_service_unittest.cc:926: #if
!defined(OS_CHROMEOS)
On 2016/05/18 18:23:43, maxbogue wrote:
> nit: preprocessor directives are always at the beginning of lines, even inside
> functions:
> https://google.github.io/styleguide/cppguide.html#Preprocessor_Directives
> 
> Here and the two below.

Done.

[maxbogue, 2016-05-19 07:05:18]

lgtm with one last request. Thanks for your patience!

https://codereview.chromium.org/1976423002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java
(right):

https://codereview.chromium.org/1976423002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java:539:
mSignOutInProgress = false;
Could you move this to the beginning of onSignOutDone? It's minor but
conceptually the wiping of profile data is part of the signing out procedure.

[Patrick Noland, 2016-05-19 18:58:23]

Patchset #4 (id:140001) has been deleted

[Patrick Noland, 2016-05-20 16:51:11]

https://codereview.chromium.org/1976423002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java
(right):

https://codereview.chromium.org/1976423002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java:539:
mSignOutInProgress = false;
On 2016/05/19 07:05:18, maxbogue wrote:
> Could you move this to the beginning of onSignOutDone? It's minor but
> conceptually the wiping of profile data is part of the signing out procedure.

Done.

[Patrick Noland, 2016-05-20 16:53:54]

The CQ bit was checked by pnoland@chromium.org

[Patrick Noland, 2016-05-20 16:53:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
maxbogue@chromium.org
Link to the patchset: https://codereview.chromium.org/1976423002/#ps160001
(title: " ")

[commit-bot: I haz the power, 2016-05-20 16:54:09]

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

[commit-bot: I haz the power, 2016-05-20 16:58:35]

Description was changed from

==========
[sync] Sign user out of Chrome on dashboard stop and reset

Currently every platform except Android and ChromeOS log out of chrome upon a
dashboard stop and clear. This CL removes the special case for Android and adds
some machinery to notify Android Java code that sign out occurred. This
notification prompts Android's SigninManager to clear Android specific state.

BUG=596611

R=maxbogue@chromium.org
==========

to

==========
[sync] Sign user out of Chrome on dashboard stop and reset

Currently every platform except Android and ChromeOS log out of chrome upon a
dashboard stop and clear. This CL removes the special case for Android and adds
some machinery to notify Android Java code that sign out occurred. This
notification prompts Android's SigninManager to clear Android specific state.

BUG=596611

R=maxbogue@chromium.org
==========

[commit-bot: I haz the power, 2016-05-20 16:58:36]

Committed patchset #4 (id:160001)

[commit-bot: I haz the power, 2016-05-20 17:00:42]

Description was changed from

==========
[sync] Sign user out of Chrome on dashboard stop and reset

Currently every platform except Android and ChromeOS log out of chrome upon a
dashboard stop and clear. This CL removes the special case for Android and adds
some machinery to notify Android Java code that sign out occurred. This
notification prompts Android's SigninManager to clear Android specific state.

BUG=596611

R=maxbogue@chromium.org
==========

to

==========
[sync] Sign user out of Chrome on dashboard stop and reset

Currently every platform except Android and ChromeOS log out of chrome upon a
dashboard stop and clear. This CL removes the special case for Android and adds
some machinery to notify Android Java code that sign out occurred. This
notification prompts Android's SigninManager to clear Android specific state.

BUG=596611

R=maxbogue@chromium.org

Committed: https://crrev.com/359ced3af82ddfc61114a801c62b1da4c79e7565
Cr-Commit-Position: refs/heads/master@{#395099}
==========

[commit-bot: I haz the power, 2016-05-20 17:00:43]

Patchset 4 (id:??) landed as
https://crrev.com/359ced3af82ddfc61114a801c62b1da4c79e7565
Cr-Commit-Position: refs/heads/master@{#395099}

[aelias_OOO_until_Jul13, 2016-05-20 18:03:46]

A revert of this CL (patchset #4 id:160001) has been created in
https://codereview.chromium.org/2004523002/ by aelias@chromium.org.

The reason for reverting is: New test testStopAndClear causes findbugs error on
Clank ToT bot.

BUG=613629.