[sync] [chromeos] Fix setup flow when sync data is cleared via dashboard and sync is set up again

[sync] [chromeos] Fix setup flow when sync data is cleared via dashboard and sync is set up again

On Chrome OS, when a user clears their sync data via the dashboard, logs
out, logs in again, and attempts to set up sync once more, the setup
flow ends up getting stuck after the backend is initialized, because
SigninTracker does not respond to the OnStateChanged call from
ProfileSyncService::OnBackendInitialized.

This regression can be traced back to r171190, where SigninTracker was
no longer initialized with the SigninTracker::SERVICES_INITIALIZING
state in SyncSetupHandler::DisplayConfigureSync. This was causing
SigninTracker to drop the OnStateChanged call from ProfileSyncService
in the scenario where the user attempted to set up sync while already
signed in to the Chrome OS device after clearing sync data via the
dashboard.

This patch restores the initial SigninTracker state in
SyncSetupHandler::DisplayConfigureSync. It also fixes the
unittests that were previously failing in the original CL.

BUG=167090, 168064, 168875, 81265
TEST=On ChromeOS, clear the dashboard, log out, log in, and set up sync.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176755

[Raghu Simha, 2013-01-09 09:21:09]

Roger, Nicolas: Please review.

This fixes the pri-1 ReleaseBlocker bugs 167090 and 168064.

[Roger Tawa OOO till Jul 10th, 2013-01-09 15:03:23]

lgtm, but please fix unit tests to handle web-based flow too.

https://codereview.chromium.org/11824021/diff/13002/chrome/browser/ui/webui/s...
File chrome/browser/ui/webui/sync_setup_handler_unittest.cc (right):

https://codereview.chromium.org/11824021/diff/13002/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_setup_handler_unittest.cc:561:
DISABLED_DisplayConfigureWithBackendDisabledAndSigninSuccess) {
Instead of always disabling this test, you should change it to behave as needed
based on Param().  See line 366 above on how to differentiate between web-based
flow and ClientLogin-based flow.

[Raghu Simha, 2013-01-09 16:01:06]

+jhawkins for OWNERS approval.

[James Hawkins, 2013-01-09 18:27:46]

https://codereview.chromium.org/11824021/diff/13002/chrome/browser/ui/webui/s...
File chrome/browser/ui/webui/sync_setup_handler_unittest.cc (right):

https://codereview.chromium.org/11824021/diff/13002/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_setup_handler_unittest.cc:561:
DISABLED_DisplayConfigureWithBackendDisabledAndSigninSuccess) {
On 2013/01/09 15:03:23, Roger Tawa wrote:
> Instead of always disabling this test, you should change it to behave as
needed
> based on Param().  See line 366 above on how to differentiate between
web-based
> flow and ClientLogin-based flow.

Yeah, I can't LG adding a disabled test.

[Raghu Simha, 2013-01-10 06:55:57]

Roger, James: Unittests are now fixed. PTAL.

[Raghu Simha, 2013-01-10 08:11:28]

On 2013/01/09 15:03:23, Roger Tawa wrote:
> Instead of always disabling this test, you should change it to behave as
needed
> based on Param().  See line 366 above on how to differentiate between
web-based
> flow and ClientLogin-based flow.

I took a closer look at the failing unittests. The behavior is identical for all
variations when it comes to the interesting web ui call to
SyncSetupOverlay.showSyncSetupPage. However, when it comes to the uninteresting
web ui call to OptionsPage.closeOverlay, the behavior changes based on 3
factors: Whether or not GetParam() is true, whether or not USE_AURA is defined,
and whether or not the tests are running on linux_chromeos.

Instead of brute-forcing checks for OptionsPage.closeOverlay in all variations,
I've gone ahead and updated a couple of the checks for the number of outstanding
web ui calls, while making sure that the web ui calls we are actually interested
in are still being verified properly.

Let me know what you think.

[Andrew T Wilson (Slow), 2013-01-10 09:20:23]

Do we have test coverage for the case in bug 167090 now, so this doesn't break
in the future?

[Raghu Simha, 2013-01-10 09:58:43]

On 2013/01/10 09:20:23, Andrew T Wilson wrote:
> Do we have test coverage for the case in bug 167090 now, so this doesn't break
> in the future?

Yes, now that DisplayConfigureWithBackendDisabledAndSigninSuccess has been
fixed, it covers the flow in bug 167090. I verified this by undoing the fix in
sync_setup_handler.cc. The unittest now detects the  spinner / timeout dialog
left sitting around, and flags it as an error.

[Raghu Simha, 2013-01-11 03:39:46]

Ping.

[Roger Tawa OOO till Jul 10th, 2013-01-11 18:33:51]

Instead of using ASSERT_LE, I think its better to be explicit about what is
expected for each param of the tests.

https://codereview.chromium.org/11824021/diff/7010/chrome/browser/ui/webui/sy...
File chrome/browser/ui/webui/sync_setup_handler_unittest.cc (right):

https://codereview.chromium.org/11824021/diff/7010/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_setup_handler_unittest.cc:552: 
The web-base sign in flow does not use the webui.  So it seems  it would be
better to do:

if (GetParam()) {
  ASSERT_EQ(1U, web_ui_.call_data().size());
} else {
  ASSERT_EQ(0U, web_ui_.call_data().size());
}

Similar for ASSERT_LE() below at line 605.

[Raghu Simha, 2013-01-11 18:53:04]

https://codereview.chromium.org/11824021/diff/7010/chrome/browser/ui/webui/sy...
File chrome/browser/ui/webui/sync_setup_handler_unittest.cc (right):

https://codereview.chromium.org/11824021/diff/7010/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_setup_handler_unittest.cc:552: 
On 2013/01/11 18:33:51, Roger Tawa wrote:
> The web-base sign in flow does not use the webui.  So it seems  it would be
> better to do:
> 
> if (GetParam()) {
>   ASSERT_EQ(1U, web_ui_.call_data().size());
> } else {
>   ASSERT_EQ(0U, web_ui_.call_data().size());
> }
> 
> Similar for ASSERT_LE() below at line 605.

When I was debugging this test on the various buildbots, I noticed that the size
of call_data() at this spot was 2 for web ui (SyncSetupOverlay.showSyncSetupPage
and OptionsPage.closeOverlay) and 1 for client login
(SyncSetupOverlay.showSyncSetupPage). Does this not seem right to you?

https://codereview.chromium.org/11824021/diff/7010/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_setup_handler_unittest.cc:605: ASSERT_LE(2U,
web_ui_.call_data().size());
At this spot, the size of call_data() was 3 for web ui and 2 for client login
when I ran the test on linux. However, there was a different number of calls
when the tests ran with USE_AURA.

[Roger Tawa OOO till Jul 10th, 2013-01-14 13:16:25]

Lgtm
On Jan 11, 2013 1:53 PM, <rsimha@chromium.org> wrote:

>
> https://codereview.chromium.**org/11824021/diff/7010/chrome/**
>
browser/ui/webui/sync_setup_**handler_unittest.cc<https://codereview.chromium.org/11824021/diff/7010/chrome/browser/ui/webui/sync_setup_handler_unittest.cc>
> File chrome/browser/ui/webui/sync_**setup_handler_unittest.cc (right):
>
> https://codereview.chromium.**org/11824021/diff/7010/chrome/**
>
browser/ui/webui/sync_setup_**handler_unittest.cc#newcode552<https://codereview.chromium.org/11824021/diff/7010/chrome/browser/ui/webui/sync_setup_handler_unittest.cc#newcode552>
> chrome/browser/ui/webui/sync_**setup_handler_unittest.cc:552:
> On 2013/01/11 18:33:51, Roger Tawa wrote:
>
>> The web-base sign in flow does not use the webui.  So it seems  it
>>
> would be
>
>> better to do:
>>
>
>  if (GetParam()) {
>>    ASSERT_EQ(1U, web_ui_.call_data().size());
>> } else {
>>    ASSERT_EQ(0U, web_ui_.call_data().size());
>> }
>>
>
>  Similar for ASSERT_LE() below at line 605.
>>
>
> When I was debugging this test on the various buildbots, I noticed that
> the size of call_data() at this spot was 2 for web ui
> (SyncSetupOverlay.**showSyncSetupPage and OptionsPage.closeOverlay) and 1
> for client login (SyncSetupOverlay.**showSyncSetupPage). Does this not
> seem right to you?
>
> https://codereview.chromium.**org/11824021/diff/7010/chrome/**
>
browser/ui/webui/sync_setup_**handler_unittest.cc#newcode605<https://codereview.chromium.org/11824021/diff/7010/chrome/browser/ui/webui/sync_setup_handler_unittest.cc#newcode605>
> chrome/browser/ui/webui/sync_**setup_handler_unittest.cc:605:
> ASSERT_LE(2U, web_ui_.call_data().size());
> At this spot, the size of call_data() was 3 for web ui and 2 for client
> login when I ran the test on linux. However, there was a different
> number of calls when the tests ran with USE_AURA.
>
>
https://codereview.chromium.**org/11824021/<https://codereview.chromium.org/1...
>

[James Hawkins, 2013-01-14 22:30:18]

LGTM with nit.

https://codereview.chromium.org/11824021/diff/7010/chrome/browser/ui/webui/sy...
File chrome/browser/ui/webui/sync_setup_handler_unittest.cc (right):

https://codereview.chromium.org/11824021/diff/7010/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_setup_handler_unittest.cc:551: ASSERT_LE(1U,
web_ui_.call_data().size());
EXPECT_LE

Here and the other ASSERTs you added.

[Raghu Simha, 2013-01-14 22:45:46]

https://codereview.chromium.org/11824021/diff/7010/chrome/browser/ui/webui/sy...
File chrome/browser/ui/webui/sync_setup_handler_unittest.cc (right):

https://codereview.chromium.org/11824021/diff/7010/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_setup_handler_unittest.cc:551: ASSERT_LE(1U,
web_ui_.call_data().size());
On 2013/01/14 22:30:18, James Hawkins wrote:
> EXPECT_LE
> 
> Here and the other ASSERTs you added.

Done.