chromeos: Make OOBE "Review sync options" open browser sync settings

Right now it opens OS settings, which has the same set of sync options
as browser settings. Soon the OS sync settings page will be different,
and we want to send users to the browser settings page either way.

This also helps if a user checks both "Review sync options" and
"Review Google Play options" during OOBE. They will see two windows,
OS settings and browser settings.

Making syncSetup a browser settings, rather than OS settings, subpage
requires changing ARC++ URL handling in ChromeNewWindowClient for
mojo enum ChromePage::SYNCSETUP. I think this is OK, because it's
closer to the original ARC++ behavior before split settings, and
also I can find no usage of the SYNCSETUP code path in the ARC++
code (or anywhere else in google3).

This CL is a partial revert of:
https://chromium-review.googlesource.com/c/chromium/src/+/1977139

Bug: 1054980, 901184
Change-Id: Ide777363aa83e9a7b0f87068c318bd1f274c7e2e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2068948Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#743994}

chrome/browser/ui/ash/chrome_new_window_client.cc

@@ -116,7 +116,6 @@ constexpr std::pair<arc::mojom::ChromePage, const char*> kOSSettingsMapping[] =
      {ChromePage::STORAGE, chrome::kStorageSubPage},
      {ChromePage::STYLUS, chrome::kStylusSubPage},
      {ChromePage::SWITCHACCESS, chrome::kSwitchAccessSubPage},
-     {ChromePage::SYNCSETUP, chrome::kSyncSetupSubPage},
      {ChromePage::TETHERSETTINGS, chrome::kTetherSettingsSubPage},
      {ChromePage::WIFI, chrome::kWiFiSettingsSubPage}};
 
@@ -130,7 +129,8 @@ constexpr std::pair<arc::mojom::ChromePage, const char*>
         {ChromePage::ONSTARTUP, chrome::kOnStartupSubPage},
         {ChromePage::PASSWORDS, chrome::kPasswordManagerSubPage},
         {ChromePage::PRIVACY, chrome::kPrivacySubPage},
-        {ChromePage::SEARCH, chrome::kSearchSubPage}};
+        {ChromePage::SEARCH, chrome::kSearchSubPage},
+        {ChromePage::SYNCSETUP, chrome::kSyncSetupSubPage}};
 
 constexpr std::pair<arc::mojom::ChromePage, const char*> kAboutPagesMapping[] =
     {{ChromePage::ABOUTBLANK, url::kAboutBlankURL},

chrome/browser/ui/ash/chrome_new_window_client_browsertest.cc

@@ -282,8 +282,6 @@ void TestAllOSSettingPages(const GURL& base_url) {
                      base_url.Resolve(chrome::kResetSubPage));
   TestOpenChromePage(ChromePage::STORAGE,
                      base_url.Resolve(chrome::kStorageSubPage));
-  TestOpenChromePage(ChromePage::SYNCSETUP,
-                     base_url.Resolve(chrome::kSyncSetupSubPage));
   TestOpenChromePage(ChromePage::ACCESSIBILITY,
                      base_url.Resolve(chrome::kAccessibilitySubPage));
   TestOpenChromePage(ChromePage::ACCOUNTMANAGER,
@@ -347,6 +345,8 @@ void TestAllBrowserSettingPages(const GURL& base_url) {
                      base_url.Resolve(chrome::kPasswordManagerSubPage));
   TestOpenChromePage(ChromePage::SEARCH,
                      base_url.Resolve(chrome::kSearchSubPage));
+  TestOpenChromePage(ChromePage::SYNCSETUP,
+                     base_url.Resolve(chrome::kSyncSetupSubPage));
 }
 
 void TestAllAboutPages() {

chrome/browser/ui/webui/settings/people_handler.cc

@@ -36,6 +36,7 @@
 #include "chrome/browser/ui/webui/signin/login_ui_service.h"
 #include "chrome/browser/ui/webui/signin/login_ui_service_factory.h"
 #include "chrome/common/url_constants.h"
+#include "chrome/common/webui_url_constants.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/autofill/core/common/autofill_prefs.h"
 #include "components/browsing_data/core/history_notice_utils.h"
@@ -64,9 +65,7 @@
 #include "ui/base/webui/web_ui_util.h"
 #include "ui/gfx/image/image.h"
 
-#if defined(OS_CHROMEOS)
-#include "chromeos/constants/chromeos_features.h"
-#else
+#if !defined(OS_CHROMEOS)
 #include "chrome/browser/ui/webui/profile_helper.h"
 #endif
 
@@ -95,15 +94,7 @@ struct SyncConfigInfo {
 };
 
 bool IsSyncSubpage(const GURL& current_url) {
-  if (current_url == chrome::GetSettingsUrl(chrome::kSyncSetupSubPage))
-    return true;
-#if defined(OS_CHROMEOS)
-  if (!chromeos::features::IsSplitSettingsSyncEnabled() &&
-      current_url == chrome::GetOSSettingsUrl(chrome::kSyncSetupSubPage)) {
-    return true;
-  }
-#endif  // defined(OS_CHROMEOS)
-  return false;
+  return current_url == chrome::GetSettingsUrl(chrome::kSyncSetupSubPage);
 }
 
 SyncConfigInfo::SyncConfigInfo()
@@ -893,6 +884,10 @@ void PeopleHandler::InitializeSyncBlocker() {
   if (!service)
     return;
 
+  // The user opened settings directly to the syncSetup sub-page, because they
+  // clicked "Settings" in the browser sync consent dialog or because they
+  // clicked "Review sync options" in the Chrome OS out-of-box experience.
+  // Don't start syncing until they finish setup.
   if (IsSyncSubpage(web_contents->GetVisibleURL())) {
     sync_blocker_ = service->GetSetupInProgressHandle();
   }

chrome/common/webui_url_constants.cc

@@ -10,10 +10,6 @@
 #include "components/safe_browsing/core/web_ui/constants.h"
 #include "extensions/buildflags/buildflags.h"
 
-#if defined(OS_CHROMEOS)
-#include "chromeos/constants/chromeos_features.h"
-#endif
-
 namespace chrome {
 
 // Please keep this file in the same order as the header.
@@ -485,6 +481,9 @@ bool IsOSSettingsSubPage(const std::string& sub_page) {
       kStorageSubPage,
       kStylusSubPage,
       kSwitchAccessSubPage,
+      // kSyncSetupSubPage is both an OS and browser sub page, but prefer the
+      // browser version. Delete this comment when SplitSettingsSync is the
+      // default, because it will introduce an "osSync" sub-page.
       kVPNSettingsSubPage,
       kWiFiSettingsSubPage,
   };
@@ -494,12 +493,6 @@ bool IsOSSettingsSubPage(const std::string& sub_page) {
   if (index != std::string::npos)
     sub_page_without_query.resize(index);
 
-  // SplitSettingsSync doesn't use the same sync subpage as browser.
-  if (!chromeos::features::IsSplitSettingsSyncEnabled() &&
-      sub_page_without_query == kSyncSetupSubPage) {
-    return true;
-  }
-
   for (const char* p : kSubPages) {
     if (sub_page_without_query == p)
       return true;