sync: final pieces to sync deferred initialization

sync: final pieces to sync deferred initialization

Refactors startup logic and state tracking into a new StartupController.
Adds a fallback timer in the event no data type triggers initialization.

I get the impression the various "IsSync{Enabled[AndLoggedIn],Accessible}"
etc methods could be consolidated somewhat, but I'll leave that to a
future CL.

TBR=kalman@chromium.org, bauerb@chromium.org
BUG=80194
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=253550

[haitaol1, 2014-02-20 00:01:05]

https://codereview.chromium.org/162443004/diff/920001/chrome/browser/sync/pro...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/162443004/diff/920001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service.cc:195:
startup_controller_weak_factory_.GetWeakPtr())) {
Why don't just use base::Unretained() if PSS always outlives
startup_controller_?

https://codereview.chromium.org/162443004/diff/920001/chrome/browser/sync/pro...
File chrome/browser/sync/profile_sync_service.h (right):

https://codereview.chromium.org/162443004/diff/920001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service.h:1010: browser_sync::StartupController
startup_controller_;
How about using scoped_ptr to always start fresh on new backend?

https://codereview.chromium.org/162443004/diff/920001/chrome/browser/sync/sta...
File chrome/browser/sync/startup_controller.cc (right):

https://codereview.chromium.org/162443004/diff/920001/chrome/browser/sync/sta...
chrome/browser/sync/startup_controller.cc:73: if (deferred_option ==
STARTUP_BACKEND_DEFERRED &&
I think the code can be simplified if this block is moved to TryStart(), i.e.
backend is always started when StartUp() is called. 
* StartUp() is only called once so first_start is not needed 
* You can bind to StartUp() directly, so OnFallbackStartupTimerExpired() is not
needed
* Only need to to update UMA "Sync.Startup.TimeDeferred" in StartUp().

Let me know if you'd like chat offline.

https://codereview.chromium.org/162443004/diff/920001/chrome/browser/sync/sta...
chrome/browser/sync/startup_controller.cc:94: void StartupController::TryStart()
{
can you return a bool to indicate whether the conditions for startup are met?
Could be useful  later for determining whether should run in backup mode.

https://codereview.chromium.org/162443004/diff/920001/chrome/browser/sync/sta...
chrome/browser/sync/startup_controller.cc:160: TryStart();
can you call StartUp() here?

https://codereview.chromium.org/162443004/diff/920001/chrome/browser/sync/sta...
chrome/browser/sync/startup_controller.cc:191:
UMA_HISTOGRAM_ENUMERATION("Sync.Startup.TypeTriggeringInit",
This UMA and the one below don't have to be in if{}. Seems better to update UMA
and TryStart() if the type is in pss->GetPreferredDataTypes().

[tim (not reviewing), 2014-02-20 22:52:52]

https://codereview.chromium.org/162443004/diff/920001/chrome/browser/sync/pro...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/162443004/diff/920001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service.cc:195:
startup_controller_weak_factory_.GetWeakPtr())) {
On 2014/02/20 00:01:06, haitaol1 wrote:
> Why don't just use base::Unretained() if PSS always outlives
> startup_controller_?

See my comment in startup_controller.h.  If StartupController's implementation
were to ever change and post tasks to a message loop (which is a practice I've
seen us do sometimes to "unwind the stack"), the fact that PSS outlives
StartupController is not enough to guarantee bad things don't happen.  In
general, my preference is to have objects vend weak pointers to themselves
defensively when you'd otherwise use a wildcard like Unretained.

https://codereview.chromium.org/162443004/diff/920001/chrome/browser/sync/pro...
File chrome/browser/sync/profile_sync_service.h (right):

https://codereview.chromium.org/162443004/diff/920001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service.h:1010: browser_sync::StartupController
startup_controller_;
On 2014/02/20 00:01:06, haitaol1 wrote:
> How about using scoped_ptr to always start fresh on new backend?

There isn't a great place to instantiate it right now because the equivalent
state in PSS is setup as part of Initialize which only happens once. This patch
takes that state and puts it into a different container but keeps the
initialization roughly the same. Maybe we can move more around later, but in
this case I might opt for parity with the current impl..

https://codereview.chromium.org/162443004/diff/920001/chrome/browser/sync/sta...
File chrome/browser/sync/startup_controller.cc (right):

https://codereview.chromium.org/162443004/diff/920001/chrome/browser/sync/sta...
chrome/browser/sync/startup_controller.cc:73: if (deferred_option ==
STARTUP_BACKEND_DEFERRED &&
On 2014/02/20 00:01:06, haitaol1 wrote:
> I think the code can be simplified if this block is moved to TryStart(), i.e.
> backend is always started when StartUp() is called. 
> * StartUp() is only called once so first_start is not needed 
> * You can bind to StartUp() directly, so OnFallbackStartupTimerExpired() is
not
> needed
> * Only need to to update UMA "Sync.Startup.TimeDeferred" in StartUp().
> 
> Let me know if you'd like chat offline.

I played around with this and it gets a little messy. I might still be
misunderstanding.  Here are the things I hit:

* TryStart can in theory be continually called by ProfileSyncService even after
we've started the backend and the conditions can continue to be true. Meaning
it'll take additional checks to ensure StartUp is only called once. We could
require the caller to not call in the already-initialized case, but I'd rather
it just be handled by no-op.
* We want to set start_up_time_ in the deferred init case but not call StartUp
to invoke the callback, meaning we'd have to inline the initialization of that
value in TryStart for one case but not the others.
* Having OnFallbackTimerExpired & OnDataTypeTriggered methods lets us record
extra UMA (DeferredInitTrigger) and gives an opportunity to add DCHECKs specific
to each case without extra parameters and if-cases to StartUp.  I like this for
the moment :/

So, the code I had at the end of TryStart wound up looking like

  if (sync_prefs_->HasSyncSetupCompleted()) {
    if (!received_start_request_) {
      if (start_up_time_.is_null()) {
        start_up_time_ = base::Time::Now();
        if (CommandLine::ForCurrentProcess()->
            HasSwitch(switches::kSyncEnableDeferredStartup)) {
          base::MessageLoop::current()->PostDelayedTask(
              FROM_HERE,
              base::Bind(&StartupController::OnFallbackStartupTimerExpired,
                         weak_factory_.GetWeakPtr()), fallback_timeout_);
          return;
        }
      }
    } else {
      StartUp();
    }
  } else if (setup_in_progress_ || auto_start_enabled_) {
    // We haven't completed sync setup. Start immediately if the user explicitly
    // kicked this off or we're supposed to automatically start syncing.
    StartUp();
  } 

So my inclination is to leave it for now, but maybe once the command line flag
is gone we can simplify?  That said, this led me to fix a few idempotency issues
(with start_backend_time_).

https://codereview.chromium.org/162443004/diff/920001/chrome/browser/sync/sta...
chrome/browser/sync/startup_controller.cc:94: void StartupController::TryStart()
{
On 2014/02/20 00:01:06, haitaol1 wrote:
> can you return a bool to indicate whether the conditions for startup are met?
> Could be useful  later for determining whether should run in backup mode.

Done.

https://codereview.chromium.org/162443004/diff/920001/chrome/browser/sync/sta...
chrome/browser/sync/startup_controller.cc:160: TryStart();
On 2014/02/20 00:01:06, haitaol1 wrote:
> can you call StartUp() here? 

The current model and one I'd like to stick to is that we always re-check
conditions when initializing sync.  It's protection against things like
IsManaged changing while the closure sits on the message loop, and is more
future-proof to new TryStart conditions that we deem "important".

https://codereview.chromium.org/162443004/diff/920001/chrome/browser/sync/sta...
chrome/browser/sync/startup_controller.cc:191:
UMA_HISTOGRAM_ENUMERATION("Sync.Startup.TypeTriggeringInit",
On 2014/02/20 00:01:06, haitaol1 wrote:
> This UMA and the one below don't have to be in if{}. Seems better to update
UMA
> and TryStart() if the type is in pss->GetPreferredDataTypes().

That data-type filtering is handled by the caller. I'll make a note in the
header.

As for the "if", nothing technically enforces that a flare can't be sent twice
by a datatype, so it seems prudent for us to protect against misuse like that
(bug or otherwise)? Or am I misunderstanding?

[tim (not reviewing), 2014-02-24 18:36:30]

On 2014/02/20 22:52:52, timsteele wrote:
>
https://codereview.chromium.org/162443004/diff/920001/chrome/browser/sync/pro...
> File chrome/browser/sync/profile_sync_service.cc (right):
> 
>
https://codereview.chromium.org/162443004/diff/920001/chrome/browser/sync/pro...
> chrome/browser/sync/profile_sync_service.cc:195:
> startup_controller_weak_factory_.GetWeakPtr())) {
> On 2014/02/20 00:01:06, haitaol1 wrote:
> > Why don't just use base::Unretained() if PSS always outlives
> > startup_controller_?
> 
> See my comment in startup_controller.h.  If StartupController's implementation
> were to ever change and post tasks to a message loop (which is a practice I've
> seen us do sometimes to "unwind the stack"), the fact that PSS outlives
> StartupController is not enough to guarantee bad things don't happen.  In
> general, my preference is to have objects vend weak pointers to themselves
> defensively when you'd otherwise use a wildcard like Unretained.
> 
>
https://codereview.chromium.org/162443004/diff/920001/chrome/browser/sync/pro...
> File chrome/browser/sync/profile_sync_service.h (right):
> 
>
https://codereview.chromium.org/162443004/diff/920001/chrome/browser/sync/pro...
> chrome/browser/sync/profile_sync_service.h:1010:
browser_sync::StartupController
> startup_controller_;
> On 2014/02/20 00:01:06, haitaol1 wrote:
> > How about using scoped_ptr to always start fresh on new backend?
> 
> There isn't a great place to instantiate it right now because the equivalent
> state in PSS is setup as part of Initialize which only happens once. This
patch
> takes that state and puts it into a different container but keeps the
> initialization roughly the same. Maybe we can move more around later, but in
> this case I might opt for parity with the current impl..
> 
>
https://codereview.chromium.org/162443004/diff/920001/chrome/browser/sync/sta...
> File chrome/browser/sync/startup_controller.cc (right):
> 
>
https://codereview.chromium.org/162443004/diff/920001/chrome/browser/sync/sta...
> chrome/browser/sync/startup_controller.cc:73: if (deferred_option ==
> STARTUP_BACKEND_DEFERRED &&
> On 2014/02/20 00:01:06, haitaol1 wrote:
> > I think the code can be simplified if this block is moved to TryStart(),
i.e.
> > backend is always started when StartUp() is called. 
> > * StartUp() is only called once so first_start is not needed 
> > * You can bind to StartUp() directly, so OnFallbackStartupTimerExpired() is
> not
> > needed
> > * Only need to to update UMA "Sync.Startup.TimeDeferred" in StartUp().
> > 
> > Let me know if you'd like chat offline.
> 
> I played around with this and it gets a little messy. I might still be
> misunderstanding.  Here are the things I hit:
> 
> * TryStart can in theory be continually called by ProfileSyncService even
after
> we've started the backend and the conditions can continue to be true. Meaning
> it'll take additional checks to ensure StartUp is only called once. We could
> require the caller to not call in the already-initialized case, but I'd rather
> it just be handled by no-op.
> * We want to set start_up_time_ in the deferred init case but not call StartUp
> to invoke the callback, meaning we'd have to inline the initialization of that
> value in TryStart for one case but not the others.
> * Having OnFallbackTimerExpired & OnDataTypeTriggered methods lets us record
> extra UMA (DeferredInitTrigger) and gives an opportunity to add DCHECKs
specific
> to each case without extra parameters and if-cases to StartUp.  I like this
for
> the moment :/
> 
> So, the code I had at the end of TryStart wound up looking like
> 
>   if (sync_prefs_->HasSyncSetupCompleted()) {
>     if (!received_start_request_) {
>       if (start_up_time_.is_null()) {
>         start_up_time_ = base::Time::Now();
>         if (CommandLine::ForCurrentProcess()->
>             HasSwitch(switches::kSyncEnableDeferredStartup)) {
>           base::MessageLoop::current()->PostDelayedTask(
>               FROM_HERE,
>               base::Bind(&StartupController::OnFallbackStartupTimerExpired,
>                          weak_factory_.GetWeakPtr()), fallback_timeout_);
>           return;
>         }
>       }
>     } else {
>       StartUp();
>     }
>   } else if (setup_in_progress_ || auto_start_enabled_) {
>     // We haven't completed sync setup. Start immediately if the user
explicitly
>     // kicked this off or we're supposed to automatically start syncing.
>     StartUp();
>   } 
> 
> So my inclination is to leave it for now, but maybe once the command line flag
> is gone we can simplify?  That said, this led me to fix a few idempotency
issues
> (with start_backend_time_).
> 
>
https://codereview.chromium.org/162443004/diff/920001/chrome/browser/sync/sta...
> chrome/browser/sync/startup_controller.cc:94: void
StartupController::TryStart()
> {
> On 2014/02/20 00:01:06, haitaol1 wrote:
> > can you return a bool to indicate whether the conditions for startup are
met?
> > Could be useful  later for determining whether should run in backup mode.
> 
> Done.
> 
>
https://codereview.chromium.org/162443004/diff/920001/chrome/browser/sync/sta...
> chrome/browser/sync/startup_controller.cc:160: TryStart();
> On 2014/02/20 00:01:06, haitaol1 wrote:
> > can you call StartUp() here? 
> 
> The current model and one I'd like to stick to is that we always re-check
> conditions when initializing sync.  It's protection against things like
> IsManaged changing while the closure sits on the message loop, and is more
> future-proof to new TryStart conditions that we deem "important".
> 
>
https://codereview.chromium.org/162443004/diff/920001/chrome/browser/sync/sta...
> chrome/browser/sync/startup_controller.cc:191:
> UMA_HISTOGRAM_ENUMERATION("Sync.Startup.TypeTriggeringInit",
> On 2014/02/20 00:01:06, haitaol1 wrote:
> > This UMA and the one below don't have to be in if{}. Seems better to update
> UMA
> > and TryStart() if the type is in pss->GetPreferredDataTypes().
> 
> That data-type filtering is handled by the caller. I'll make a note in the
> header.
> 
> As for the "if", nothing technically enforces that a flare can't be sent twice
> by a datatype, so it seems prudent for us to protect against misuse like that
> (bug or otherwise)? Or am I misunderstanding?

ping

[haitaol1, 2014-02-25 00:53:00]

https://codereview.chromium.org/162443004/diff/920001/chrome/browser/sync/sta...
File chrome/browser/sync/startup_controller.cc (right):

https://codereview.chromium.org/162443004/diff/920001/chrome/browser/sync/sta...
chrome/browser/sync/startup_controller.cc:73: if (deferred_option ==
STARTUP_BACKEND_DEFERRED &&
fair enough

On 2014/02/20 22:52:52, timsteele wrote:
> On 2014/02/20 00:01:06, haitaol1 wrote:
> > I think the code can be simplified if this block is moved to TryStart(),
i.e.
> > backend is always started when StartUp() is called. 
> > * StartUp() is only called once so first_start is not needed 
> > * You can bind to StartUp() directly, so OnFallbackStartupTimerExpired() is
> not
> > needed
> > * Only need to to update UMA "Sync.Startup.TimeDeferred" in StartUp().
> > 
> > Let me know if you'd like chat offline.
> 
> I played around with this and it gets a little messy. I might still be
> misunderstanding.  Here are the things I hit:
> 
> * TryStart can in theory be continually called by ProfileSyncService even
after
> we've started the backend and the conditions can continue to be true. Meaning
> it'll take additional checks to ensure StartUp is only called once. We could
> require the caller to not call in the already-initialized case, but I'd rather
> it just be handled by no-op.
> * We want to set start_up_time_ in the deferred init case but not call StartUp
> to invoke the callback, meaning we'd have to inline the initialization of that
> value in TryStart for one case but not the others.
> * Having OnFallbackTimerExpired & OnDataTypeTriggered methods lets us record
> extra UMA (DeferredInitTrigger) and gives an opportunity to add DCHECKs
specific
> to each case without extra parameters and if-cases to StartUp.  I like this
for
> the moment :/
> 
> So, the code I had at the end of TryStart wound up looking like
> 
>   if (sync_prefs_->HasSyncSetupCompleted()) {
>     if (!received_start_request_) {
>       if (start_up_time_.is_null()) {
>         start_up_time_ = base::Time::Now();
>         if (CommandLine::ForCurrentProcess()->
>             HasSwitch(switches::kSyncEnableDeferredStartup)) {
>           base::MessageLoop::current()->PostDelayedTask(
>               FROM_HERE,
>               base::Bind(&StartupController::OnFallbackStartupTimerExpired,
>                          weak_factory_.GetWeakPtr()), fallback_timeout_);
>           return;
>         }
>       }
>     } else {
>       StartUp();
>     }
>   } else if (setup_in_progress_ || auto_start_enabled_) {
>     // We haven't completed sync setup. Start immediately if the user
explicitly
>     // kicked this off or we're supposed to automatically start syncing.
>     StartUp();
>   } 
> 
> So my inclination is to leave it for now, but maybe once the command line flag
> is gone we can simplify?  That said, this led me to fix a few idempotency
issues
> (with start_backend_time_).

https://codereview.chromium.org/162443004/diff/920001/chrome/browser/sync/sta...
chrome/browser/sync/startup_controller.cc:191:
UMA_HISTOGRAM_ENUMERATION("Sync.Startup.TypeTriggeringInit",
I'm more concerned that backend is started by disabled types? Is that possible?


On 2014/02/20 22:52:52, timsteele wrote:
> On 2014/02/20 00:01:06, haitaol1 wrote:
> > This UMA and the one below don't have to be in if{}. Seems better to update
> UMA
> > and TryStart() if the type is in pss->GetPreferredDataTypes().
> 
> That data-type filtering is handled by the caller. I'll make a note in the
> header.
> 
> As for the "if", nothing technically enforces that a flare can't be sent twice
> by a datatype, so it seems prudent for us to protect against misuse like that
> (bug or otherwise)? Or am I misunderstanding?

https://codereview.chromium.org/162443004/diff/1110002/chrome/browser/sync/st...
File chrome/browser/sync/startup_controller.cc (right):

https://codereview.chromium.org/162443004/diff/1110002/chrome/browser/sync/st...
chrome/browser/sync/startup_controller.cc:148: return false;
NOTREACHED()?

[tim (not reviewing), 2014-02-25 01:17:43]

https://codereview.chromium.org/162443004/diff/920001/chrome/browser/sync/sta...
File chrome/browser/sync/startup_controller.cc (right):

https://codereview.chromium.org/162443004/diff/920001/chrome/browser/sync/sta...
chrome/browser/sync/startup_controller.cc:191:
UMA_HISTOGRAM_ENUMERATION("Sync.Startup.TypeTriggeringInit",
On 2014/02/25 00:53:01, haitaol1 wrote:
> I'm more concerned that backend is started by disabled types? Is that
possible?
> 
> 
> On 2014/02/20 22:52:52, timsteele wrote:
> > On 2014/02/20 00:01:06, haitaol1 wrote:
> > > This UMA and the one below don't have to be in if{}. Seems better to
update
> > UMA
> > > and TryStart() if the type is in pss->GetPreferredDataTypes().
> > 
> > That data-type filtering is handled by the caller. I'll make a note in the
> > header.
> > 
> > As for the "if", nothing technically enforces that a flare can't be sent
twice
> > by a datatype, so it seems prudent for us to protect against misuse like
that
> > (bug or otherwise)? Or am I misunderstanding?
> 

No, I don't believe so, because the caller (PSS) is responsible for filtering
the type by active types.

https://codereview.chromium.org/162443004/diff/1110002/chrome/browser/sync/st...
File chrome/browser/sync/startup_controller.cc (right):

https://codereview.chromium.org/162443004/diff/1110002/chrome/browser/sync/st...
chrome/browser/sync/startup_controller.cc:148: return false;
On 2014/02/25 00:53:02, haitaol1 wrote:
> NOTREACHED()?

We call TryStart pretty liberally in ProfileSyncService (like
OnSyncManagedPrefChange), so it's probably fine to wind up here?

[haitaol1, 2014-02-25 17:45:45]

lgtm

[tim (not reviewing), 2014-02-25 18:05:17]

kalman@chromium.org: TBR for extensions

bauerb@chromium.org: TBR for managed_mode

rogerta@chromium.org: Please review changes in c/b/ui/webui.

[Roger Tawa OOO till Jul 10th, 2014-02-25 21:44:49]

lgtm c/b/ui/webui

[tim (not reviewing), 2014-02-25 22:54:47]

The CQ bit was checked by tim@chromium.org

[commit-bot: I haz the power, 2014-02-25 22:58:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tim@chromium.org/162443004/1110002

[commit-bot: I haz the power, 2014-02-25 22:58:28]

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

[commit-bot: I haz the power, 2014-02-25 22:58:28]

Failed to apply patch for
chrome/browser/ui/webui/sync_setup_handler_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/webui/sync_setup_handler_unittest.cc
  Hunk #2 FAILED at 320.
  Hunk #3 succeeded at 375 (offset 4 lines).
  Hunk #4 succeeded at 388 (offset 4 lines).
  Hunk #5 FAILED at 436.
  Hunk #6 FAILED at 471.
  Hunk #7 FAILED at 526.
  Hunk #8 FAILED at 554.
  5 out of 8 hunks FAILED -- saving rejects to file
chrome/browser/ui/webui/sync_setup_handler_unittest.cc.rej

Patch:       chrome/browser/ui/webui/sync_setup_handler_unittest.cc
Index: chrome/browser/ui/webui/sync_setup_handler_unittest.cc
diff --git a/chrome/browser/ui/webui/sync_setup_handler_unittest.cc
b/chrome/browser/ui/webui/sync_setup_handler_unittest.cc
index
8f4725b8112f4a40e60cb4dc9b76fe743da9b484..b689ce239578c5551548c77f2f6e4d1e8d9f20fb
100644
--- a/chrome/browser/ui/webui/sync_setup_handler_unittest.cc
+++ b/chrome/browser/ui/webui/sync_setup_handler_unittest.cc
@@ -280,10 +280,11 @@ class SyncSetupHandlerTest : public testing::Test {
     error_ = GoogleServiceAuthError::AuthErrorNone();
     profile_.reset(ProfileSyncServiceMock::MakeSignedInTestingProfile());
 
+    profile_->GetPrefs()->SetString(
+        prefs::kGoogleServicesUsername, GetTestUser());
     mock_signin_ = static_cast<SigninManagerBase*>(
         SigninManagerFactory::GetInstance()->SetTestingFactoryAndUse(
             profile_.get(), FakeSigninManagerBase::Build));
-
     mock_pss_ = static_cast<ProfileSyncServiceMock*>(
         ProfileSyncServiceFactory::GetInstance()->SetTestingFactoryAndUse(
             profile_.get(),
@@ -319,12 +320,6 @@ class SyncSetupHandlerTest : public testing::Test {
     // An initialized ProfileSyncService will have already completed sync setup
     // and will have an initialized sync backend.
     ASSERT_TRUE(mock_signin_->IsInitialized());
-    EXPECT_CALL(*mock_pss_, IsSyncEnabledAndLoggedIn())
-        .WillRepeatedly(Return(true));
-    EXPECT_CALL(*mock_pss_, IsOAuthRefreshTokenAvailable())
-        .WillRepeatedly(Return(true));
-    EXPECT_CALL(*mock_pss_, HasSyncSetupCompleted())
-        .WillRepeatedly(Return(true));
     EXPECT_CALL(*mock_pss_, sync_initialized()).WillRepeatedly(Return(true));
   }
 
@@ -370,6 +365,10 @@ class SyncSetupHandlerTest : public testing::Test {
       handler_->sync_startup_tracker_->OnStateChanged();
   }
 
+  virtual std::string GetTestUser() {
+    return std::string(kTestUser);
+  }
+
   content::TestBrowserThreadBundle thread_bundle_;
   scoped_ptr<Profile> profile_;
   ProfileSyncServiceMock* mock_pss_;
@@ -379,11 +378,15 @@ class SyncSetupHandlerTest : public testing::Test {
   scoped_ptr<TestingSyncSetupHandler> handler_;
 };
 
+class SyncSetupHandlerFirstSigninTest : public SyncSetupHandlerTest {
+  virtual std::string GetTestUser() OVERRIDE { return std::string(); }
+};
+
 TEST_F(SyncSetupHandlerTest, Basic) {
 }
 
 #if !defined(OS_CHROMEOS)
-TEST_F(SyncSetupHandlerTest, DisplayBasicLogin) {
+TEST_F(SyncSetupHandlerFirstSigninTest, DisplayBasicLogin) {
   EXPECT_CALL(*mock_pss_, IsSyncEnabledAndLoggedIn())
       .WillRepeatedly(Return(false));
   EXPECT_CALL(*mock_pss_, IsOAuthRefreshTokenAvailable())
@@ -427,14 +430,13 @@ TEST_F(SyncSetupHandlerTest, ShowSyncSetupWhenNotSignedIn)
{
             LoginUIServiceFactory::GetForProfile(
                 profile_.get())->current_login_ui());
 }
-#endif
+#endif  // !defined(OS_CHROMEOS)
 
 // Verifies that the handler correctly handles a cancellation when
 // it is displaying the spinner to the user.
 TEST_F(SyncSetupHandlerTest, DisplayConfigureWithBackendDisabledAndCancel) {
   EXPECT_CALL(*mock_pss_, IsSyncEnabledAndLoggedIn())
       .WillRepeatedly(Return(true));
-  profile_->GetPrefs()->SetString(prefs::kGoogleServicesUsername, kTestUser);
   EXPECT_CALL(*mock_pss_, IsOAuthRefreshTokenAvailable())
       .WillRepeatedly(Return(true));
   EXPECT_CALL(*mock_pss_, HasSyncSetupCompleted())
@@ -462,7 +464,6 @@ TEST_F(SyncSetupHandlerTest,
        DisplayConfigureWithBackendDisabledAndSyncStartupCompleted) {
   EXPECT_CALL(*mock_pss_, IsSyncEnabledAndLoggedIn())
       .WillRepeatedly(Return(true));
-  profile_->GetPrefs()->SetString(prefs::kGoogleServicesUsername, kTestUser);
   EXPECT_CALL(*mock_pss_, IsOAuthRefreshTokenAvailable())
       .WillRepeatedly(Return(true));
   EXPECT_CALL(*mock_pss_, HasSyncSetupCompleted())
@@ -517,7 +518,6 @@ TEST_F(SyncSetupHandlerTest,
        DisplayConfigureWithBackendDisabledAndCancelAfterSigninSuccess) {
   EXPECT_CALL(*mock_pss_, IsSyncEnabledAndLoggedIn())
       .WillRepeatedly(Return(true));
-  profile_->GetPrefs()->SetString(prefs::kGoogleServicesUsername, kTestUser);
   EXPECT_CALL(*mock_pss_, IsOAuthRefreshTokenAvailable())
       .WillRepeatedly(Return(true));
   EXPECT_CALL(*mock_pss_, HasSyncSetupCompleted())
@@ -545,7 +545,6 @@ TEST_F(SyncSetupHandlerTest,
        DisplayConfigureWithBackendDisabledAndSigninFailed) {
   EXPECT_CALL(*mock_pss_, IsSyncEnabledAndLoggedIn())
       .WillRepeatedly(Return(true));
-  profile_->GetPrefs()->SetString(prefs::kGoogleServicesUsername, kTestUser);
   EXPECT_CALL(*mock_pss_, IsOAuthRefreshTokenAvailable())
       .WillRepeatedly(Return(true));
   EXPECT_CALL(*mock_pss_, HasSyncSetupCompleted())

[tim (not reviewing), 2014-02-26 05:53:34]

The CQ bit was checked by tim@chromium.org

[commit-bot: I haz the power, 2014-02-26 06:13:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tim@chromium.org/162443004/1260001

[commit-bot: I haz the power, 2014-02-26 10:10:00]

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

[commit-bot: I haz the power, 2014-02-26 10:10:01]

Try jobs failed on following builders: linux_chromium_rel

[tim (not reviewing), 2014-02-26 17:28:58]

The CQ bit was checked by tim@chromium.org

[commit-bot: I haz the power, 2014-02-26 17:29:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tim@chromium.org/162443004/1260001

[commit-bot: I haz the power, 2014-02-26 19:27:39]

Change committed as 253550