Export a private webstore API to call into the new inline sign-in flow.

chrome/browser/ui/sync/one_click_signin_sync_observer.cc

Index: chrome/browser/ui/sync/one_click_signin_sync_observer.cc
diff --git a/chrome/browser/ui/sync/one_click_signin_sync_observer.cc b/chrome/browser/ui/sync/one_click_signin_sync_observer.cc
new file mode 100644
index 0000000000000000000000000000000000000000..685e39bb5eb31c9265e4a8c59ca00158ae036853
--- /dev/null
+++ b/chrome/browser/ui/sync/one_click_signin_sync_observer.cc
@@ -0,0 +1,117 @@
+// 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.
+
+#include "chrome/browser/ui/sync/one_click_signin_sync_observer.h"
+
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/signin/signin_promo.h"
+#include "chrome/browser/sync/profile_sync_service.h"
+#include "chrome/browser/sync/profile_sync_service_factory.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_delegate.h"
+
+namespace {
+
+void CloseTab(content::WebContents* tab) {
+  content::WebContentsDelegate* tab_delegate = tab->GetDelegate();
+  if (tab_delegate)
+    tab_delegate->CloseContents(tab);
+}
+
+}  // namespace
+
+
+OneClickSigninSyncObserver::OneClickSigninSyncObserver(
+    content::WebContents* web_contents,
+    const GURL& continue_url)
+    : content::WebContentsObserver(web_contents),
+      continue_url_(continue_url) {
+  DCHECK(!continue_url_.is_empty());
+
+  ProfileSyncService* sync_service = GetSyncService(web_contents);
+  if (sync_service)
+    sync_service->AddObserver(this);
+  else
+    delete this;
+
+  // TODO(isherman): What's the appropriate behavior if there is no sync
+  // service?  Never redirect?

[Ilya Sherman, 2014/03/20 08:41:45]

^^^

[guohui, 2014/03/21 19:04:15]

PSS is null in incognito mode or if sync is disabled. chrome://chrome-signin
does not load in incognito mode. Thus the only remaining case for null PSS is
when sync is disabled. Personally i think you should ignore the error and
redirect back to webstore immediately, since signin is indeed successful.

[Ilya Sherman, 2014/03/22 00:06:28]

Done.

+}
+
+OneClickSigninSyncObserver::~OneClickSigninSyncObserver() {}
+
+void OneClickSigninSyncObserver::WebContentsDestroyed(
+    content::WebContents* web_contents) {
+  ProfileSyncService* sync_service = GetSyncService(web_contents);
+  if (sync_service)
+    sync_service->RemoveObserver(this);
+
+  delete this;
+}
+
+void OneClickSigninSyncObserver::OnStateChanged() {
+  ProfileSyncService* sync_service = GetSyncService(web_contents());
+
+  // At this point, the sign-in process is complete, and control has been handed
+  // back to the sync engine. Close the gaia sign in tab if the |continue_url_|
+  // contains the |auto_close| parameter. Otherwise, wait for sync setup to
+  // complete and then navigate to the |continue_url_|.
+  if (signin::IsAutoCloseEnabledInURL(continue_url_)) {
+    // Close the Gaia sign-in tab via a task to make sure we aren't in the
+    // middle of any WebUI handler code.
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(&CloseTab, base::Unretained(web_contents())));
+  } else {
+    if (sync_service->FirstSetupInProgress()) {
+      // Sync setup has not completed yet. Wait for it to complete.
+      return;
+    }
+
+    if (sync_service->sync_initialized() &&
+        signin::GetSourceForPromoURL(continue_url_)
+            != signin::SOURCE_SETTINGS) {
+      // TODO(isherman): I'm seeing this case reached with the following STR:
+      // 0. Open a tab pointed to chrome://settings.
+      // 1. In a new tab, navigate to the Chrome Webstore.
+      // 2. Trigger the private API to sign in to Chrome.
+      // 3. Check the box to "Choose what to syc".
+      // 4. Enter username and password to sign in.
+      // 5. Wait for the current tab to be redirected to
+      //    chrome://settings/syncSetup
+      // 6. Close the original chrome://settings tab.
+      // 7. At this point, the current tab will redirect back to the Chrome
+      //    Webstore, even though the user hasn't actually configured Sync.
+      // I don't really understand why closing the first chrome://settings tab
+      // triggers an OnStateChanged() call.  Interestingly, the behavior is
+      // different from the case where the user clicks "Cancel" in the current
+      // tab that's showing the "Configure sync" dialog.  Also, this code
+      // doesn't seem to handle the case where the user opts to choose something
+      // other than "Sync everything".  Again, I don't know why -- it works fine
+      // for the "Sync everything" case, and it's unchanged from the
+      // pre-existing OneClickSigninHelper logic.  This is admittedly all a pile
+      // of edge-cases, but I'm worried that there might be a lot of these edge
+      // cases that add up to a bad user experience vs. opening a new tab that
+      // houses the "Configure sync" settings dialog.  WDYT?

[Ilya Sherman, 2014/03/20 08:41:45]

^^^

[guohui, 2014/03/21 19:04:15]

these sounds like existing bugs in sync code. I suspect the culprit is in
sync_setup_handler, though i am not familiar with it. If you could repro the bug
on canary, then you may just file a bug for sync team and add a todo here. I
agree this is an edge case so we could fix it in later CLs.

[Ilya Sherman, 2014/03/21 19:30:18]

I don't actually know how to trigger a similar flow on Canary.  Could you give
me some STR for triggering an inline sign-in flow?

[guohui, 2014/03/21 20:00:21]

you can trigger inline sign-in flow from the signin button on ntp or wrench
menu. I actually just tried it on canary and did not see the issue, so it may be
a real bug in this CL. Have you debugged it? Could you please the callstack when
OnStateChanged is triggered from closing the first settings page?

Also could you please elaborate on what you mean by "doesn't seem to handle the
case other than sync everthing", does the if check above fail?

[Ilya Sherman, 2014/03/21 23:53:32]

I tried triggering the sign-in flow from the wrench menu.  I observed that if I
"Choose what to sync", the flow never redirects back to the NTP; whereas if I
leave that checkbox unchecked, it does redirect.  I'm testing on a Mac, fwiw.
Here's the trimmed callstack:

OneClickSigninSyncObserver::OnStateChanged() + 1900
ProfileSyncService::NotifyObservers() + 126
ProfileSyncService::OnConfigureStart() + 105
...
ProfileSyncService::ConfigureDataTypeManager() + 692
ProfileSyncService::ReconfigureDatatypeManager() + 301
ProfileSyncService::SetSetupInProgress(bool) + 170
SyncSetupHandler::CloseSyncSetup() + 734
SyncSetupHandler::~SyncSetupHandler() + 121
...
content::WebUIImpl::~WebUIImpl() + 146

It looks like a bug in the Sync WebUI handler code.  Though, I think it's
supposed to be guaranteed that the settings page is a singleton... so I'm not
sure if the maintainers of the handler would agree that this is a bug in that
code, vs. a bug in how we're using the settings page.
More specifically, this code is reached, and the LoadURL() call is issued.  It
seems like something else is also calling LoadURL() very shortly after it's
called here, and that call is overriding this one.

Here's a log with debugging statements in it:

[one_click_signin_sync_observer.cc(53)] Reached
OneClickSigninSyncObserver::OnStateChanged()
[one_click_signin_sync_observer.cc(70)] First setup is still in progress; return
early.
... The above lines are repeated three more times, then: ...
[one_click_signin_sync_observer.cc(53)] Reached
OneClickSigninSyncObserver::OnStateChanged()
[one_click_signin_sync_observer.cc(78)] Initialized successfully!  Time to
redirect to the continue URL.
[one_click_signin_sync_observer.cc(108)] Sync setup is done.  Removing self as
an observer and dying.

[guohui, 2014/03/24 21:31:52]

i don't think settings page is always a singleton. E.g. user can always open
multiple settings tab by typing the URL in omnibox or bookmark.

From the callstack, it seems the issue is CloseSyncSetup should not call
SetSetupInProgress when the first settings tab is closed since signin setup is
still in progress on a second tab. It does seem to me a bug in sync code. I
repro similar bug with the following steps:
1. open a settings tab
2. open a second tab and manually navigate to settings and click 'advanced sync
settings;
3. open a third tab and manually navigate to settings

at this point both tab 1 and 3 show "sync setup in progress".
Then close the tab 1 and switch to tab 3, observe that it no longer shows 'sync
setup in progress'. This means closing tab 1 does abort sync setup, even though
the setup is actually handled on the second tab.

I think you could file a bug for sync team and add a todo here.

+      web_contents()->GetController().LoadURL(
+          continue_url_,
+          content::Referrer(),
+          content::PAGE_TRANSITION_AUTO_TOPLEVEL,
+          std::string());
+    }
+
+    // TODO(isherman): What's the appropriate behavior if the user presses
+    // "Cancel" after opting to "Choose what to sync"?

[Ilya Sherman, 2014/03/20 08:41:45]

^^^

[guohui, 2014/03/21 19:04:15]

If user presses 'cancel', Chrome would sign out. In this case, i think Chrome
should just abort redirect, and stay on the settings page.

[Ilya Sherman, 2014/03/21 19:30:18]

Actually, the behavior that I observed was that Chrome doesn't sign out.  What
happens instead is that the user remains signed-in, but Sync is disabled.  Does
that change your answer?

[guohui, 2014/03/24 21:31:52]

this is actually a known bug, and roger is working on it now.

+  }
+
+  sync_service->RemoveObserver(this);
+  delete this;
+}
+
+ProfileSyncService* OneClickSigninSyncObserver::GetSyncService(
+    content::WebContents* web_contents) {
+  Profile* profile =
+      Profile::FromBrowserContext(web_contents->GetBrowserContext());
+  return ProfileSyncServiceFactory::GetForProfile(profile);
+}