[Signin] Factor out a helper class to load a continue URL once Sync is configured.

chrome/browser/ui/sync/one_click_signin_sync_observer.h

Index: chrome/browser/ui/sync/one_click_signin_sync_observer.h
diff --git a/chrome/browser/ui/sync/one_click_signin_sync_observer.h b/chrome/browser/ui/sync/one_click_signin_sync_observer.h
new file mode 100644
index 0000000000000000000000000000000000000000..055921fdcdfa81d5bd959b51f49dddf9d2bc238e
--- /dev/null
+++ b/chrome/browser/ui/sync/one_click_signin_sync_observer.h
@@ -0,0 +1,51 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_UI_SYNC_ONE_CLICK_SIGNIN_SYNC_OBSERVER_H_
+#define CHROME_BROWSER_UI_SYNC_ONE_CLICK_SIGNIN_SYNC_OBSERVER_H_
+
+#include "chrome/browser/sync/profile_sync_service_observer.h"
+#include "content/public/browser/web_contents_observer.h"
+#include "url/gurl.h"
+
+class ProfileSyncService;
+
+namespace content {
+class WebContents;
+}
+
+class OneClickSigninSyncObserver : public content::WebContentsObserver,
+                                   public ProfileSyncServiceObserver {

[Roger Tawa OOO till Jul 10th, 2014/03/31 20:10:38]

Oops, hit send too quickly in previous comment.  Why do we need this class
instead of adding this functionality to OneClickSigninSyncStarter?

[Ilya Sherman, 2014/04/01 02:00:35]

So, there are two ways to interpret this question:
(1) Why factor out a separate class for this functionality, rather than keeping
all the code in an existing class?
(2) Why does the OneClickSigninHelper class need to know about this factored-out
class, rather than having OneClickSigninSyncStarter handle this detail?

With respect to (1), I'm pretty firmly of the opinion that having many small
classes is an improvement over having a few large classes in terms of
testability, and hence in terms of maintainability.  The OneClickSigninHelper
class is a good example: It does so much, that it's hard for a new developer
like myself to fully grok it.  It also had much less test coverage for this
functionality, even though this CL introduces no changes in functionality.

With respect to (2), that might be possible; I'll need to do more research.  If
you're ok with this, I'd prefer to investigate that in a follow-up CL, since (a)
this one is already relatively large, and (b) my next CL was going to add a call
from the OneClickSigninSyncStarter to launch a OneClickSigninSyncObserver.  It
would definitely be nice if we could remove this logic from the OCSHelper class
entirely, since it would better unify the code paths between the web-based
sign-in flow and the inline one.

+ public:
+  // Waits for Sync to be initialized, then navigates the |web_contents| to the
+  // |continue_url|.  Instances of this class delete themselves once the job is
+  // done.
+  OneClickSigninSyncObserver(content::WebContents* web_contents,
+                             const GURL& continue_url);
+
+ protected:
+  // Exposed for testing.
+  virtual ~OneClickSigninSyncObserver();
+
+ private:
+  // content::WebContentsObserver:
+  virtual void WebContentsDestroyed(content::WebContents* contents) OVERRIDE;
+
+  // ProfileSyncServiceObserver:
+  virtual void OnStateChanged() OVERRIDE;
+
+  // Loads the |continue_url_| in the |web_contents()|.
+  void LoadContinueUrl();
+
+  // Returns the ProfileSyncService associated with the |web_contents|.
+  // The returned value may be NULL.
+  ProfileSyncService* GetSyncService(content::WebContents* web_contents);
+
+  // The URL to redirect to once Sync is initialized.
+  const GURL continue_url_;
+
+  DISALLOW_COPY_AND_ASSIGN(OneClickSigninSyncObserver);
+};
+
+#endif  // CHROME_BROWSER_UI_SYNC_ONE_CLICK_SIGNIN_SYNC_OBSERVER_H_