Convert --{enable,disable}-simplified-fullscreen-ui to a feature flag. (issue 1685043002 by tapted@chromium.org)

[tap...@chromium.org]

Reviewers: scheib, Alexei Svitkine
CL: https://codereview.chromium.org/1685043002/

Message:
Hi Vince and Alexei - please take a look. (I think I'm doing this right)

The finch change is in cl/114291338 but I'm not sure what order these should
land in (also I'm still total a google3/critique n00b).

Thanks!

Description:
Convert --{enable,disable}-simplified-fullscreen-ui to a feature flag.

It is on by default on Linux, Windows and ChromeOS. To bring the feature
up on Mac we are trying the already-written toolkit-views UI rather than
Cocoa. Currently it's working on Mac behind --enable-simplified-fullscreen-ui
but since this is the first time we are running toolkit-views UI by-default
on Mac, we want to bring it up via an experiment.

BUG=585009

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+22, -26 lines):
  M chrome/browser/about_flags.cc
  M chrome/browser/ui/exclusive_access/exclusive_access_manager.h
  M chrome/browser/ui/exclusive_access/exclusive_access_manager.cc
  M chrome/common/chrome_switches.h
  M chrome/common/chrome_switches.cc


Index: chrome/browser/about_flags.cc
diff --git a/chrome/browser/about_flags.cc b/chrome/browser/about_flags.cc
index 09ba9415359f77552d6bd72ed17cfcf087385863..d53ac7b77435e421ef9efb94d8ad0eb02f058dba 100644
--- a/chrome/browser/about_flags.cc
+++ b/chrome/browser/about_flags.cc
@@ -104,6 +104,10 @@
 #include "ui/ozone/public/ozone_switches.h"
 #endif
 
+#if defined(TOOLKIT_VIEWS)
+#include "chrome/browser/ui/exclusive_access/exclusive_access_manager.h"
+#endif
+
 using flags_ui::FeatureEntry;
 using flags_ui::kOsMac;
 using flags_ui::kOsWin;
@@ -1614,10 +1618,11 @@ const FeatureEntry kFeatureEntries[] = {
      IDS_FLAGS_NEW_TASK_MANAGER_DESCRIPTION, kOsDesktop,
      SINGLE_DISABLE_VALUE_TYPE(switches::kDisableNewTaskManager)},
 #endif  // defined(ENABLE_TASK_MANAGER)
+#if defined(TOOLKIT_VIEWS)
     {"simplified-fullscreen-ui", IDS_FLAGS_SIMPLIFIED_FULLSCREEN_UI_NAME,
      IDS_FLAGS_SIMPLIFIED_FULLSCREEN_UI_DESCRIPTION, kOsDesktop,
-     ENABLE_DISABLE_VALUE_TYPE(switches::kEnableSimplifiedFullscreenUI,
-                               switches::kDisableSimplifiedFullscreenUI)},
+     FEATURE_VALUE_TYPE(ExclusiveAccessManager::kSimplifiedUIFeature)},
+#endif  // defined(TOOLKIT_VIEWS)
 #if defined(OS_ANDROID)
     {"progress-bar-animation", IDS_FLAGS_PROGRESS_BAR_ANIMATION_NAME,
      IDS_FLAGS_PROGRESS_BAR_ANIMATION_DESCRIPTION, kOsAndroid,
Index: chrome/browser/ui/exclusive_access/exclusive_access_manager.cc
diff --git a/chrome/browser/ui/exclusive_access/exclusive_access_manager.cc b/chrome/browser/ui/exclusive_access/exclusive_access_manager.cc
index edb4aa8007ac94e8010a2ccf00d203b48bd0a753..8537e5b19eeb2b95c93247004ac2028634ee8e9e 100644
--- a/chrome/browser/ui/exclusive_access/exclusive_access_manager.cc
+++ b/chrome/browser/ui/exclusive_access/exclusive_access_manager.cc
@@ -18,6 +18,15 @@
 
 using content::WebContents;
 
+const base::Feature ExclusiveAccessManager::kSimplifiedUIFeature = {
+    "ViewsSimplifiedFullscreenUI",
+#if defined(USE_AURA)
+    base::FEATURE_ENABLED_BY_DEFAULT,
+#else
+    base::FEATURE_DISABLED_BY_DEFAULT,
+#endif
+};
+
 ExclusiveAccessManager::ExclusiveAccessManager(
     ExclusiveAccessContext* exclusive_access_context)
     : exclusive_access_context_(exclusive_access_context),
@@ -91,22 +100,7 @@ GURL ExclusiveAccessManager::GetExclusiveAccessBubbleURL() const {
 
 // static
 bool ExclusiveAccessManager::IsSimplifiedFullscreenUIEnabled() {
-  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kEnableSimplifiedFullscreenUI)) {
-    return true;
-  }
-
-  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kDisableSimplifiedFullscreenUI)) {
-    return false;
-  }
-
-  // Enabled by default on Aura platforms only.
-#if defined(USE_AURA)
-  return true;
-#else
-  return false;
-#endif  // defined(USE_AURA)
+  return base::FeatureList::IsEnabled(kSimplifiedUIFeature);
 }
 
 void ExclusiveAccessManager::OnTabDeactivated(WebContents* web_contents) {
Index: chrome/browser/ui/exclusive_access/exclusive_access_manager.h
diff --git a/chrome/browser/ui/exclusive_access/exclusive_access_manager.h b/chrome/browser/ui/exclusive_access/exclusive_access_manager.h
index 68d7a5a6c3eb3c53ecaf0b2e4c66f162092e5af4..0a91afe647a324b5b3b048ca56d1e284ea7d6f14 100644
--- a/chrome/browser/ui/exclusive_access/exclusive_access_manager.h
+++ b/chrome/browser/ui/exclusive_access/exclusive_access_manager.h
@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_UI_EXCLUSIVE_ACCESS_EXCLUSIVE_ACCESS_MANAGER_H_
 #define CHROME_BROWSER_UI_EXCLUSIVE_ACCESS_EXCLUSIVE_ACCESS_MANAGER_H_
 
+#include "base/feature_list.h"
 #include "base/macros.h"
 #include "base/memory/scoped_ptr.h"
 #include "chrome/browser/ui/exclusive_access/exclusive_access_bubble_type.h"
@@ -26,6 +27,10 @@ class WebContents;
 // the exit bubble to reflect the combined state.
 class ExclusiveAccessManager {
  public:
+  // A new user experience for transitioning into fullscreen and mouse pointer
+  // lock states.
+  static const base::Feature kSimplifiedUIFeature;
+
   explicit ExclusiveAccessManager(
       ExclusiveAccessContext* exclusive_access_context);
   ~ExclusiveAccessManager();
Index: chrome/common/chrome_switches.cc
diff --git a/chrome/common/chrome_switches.cc b/chrome/common/chrome_switches.cc
index abeece33c2917a7e418a3461d4d0ca6e1323904d..3dcd622cca10dc21d3b64566af9c64f660e89a4f 100644
--- a/chrome/common/chrome_switches.cc
+++ b/chrome/common/chrome_switches.cc
@@ -515,12 +515,6 @@ const char kEnableSessionCrashedBubble[] = "enable-session-crashed-bubble";
 const char kEnableSettingsWindow[]           = "enable-settings-window";
 const char kDisableSettingsWindow[]          = "disable-settings-window";
 
-// A new user experience for transitioning into fullscreen and mouse pointer
-// lock states.
-const char kEnableSimplifiedFullscreenUI[]  = "enable-simplified-fullscreen-ui";
-const char kDisableSimplifiedFullscreenUI[] =
-    "disable-simplified-fullscreen-ui";
-
 // Enable the Site Engagement App Banner which triggers app install banners
 // using the site engagement service rather than a navigation-based heuristic.
 // Implicitly enables the site engagement service.
Index: chrome/common/chrome_switches.h
diff --git a/chrome/common/chrome_switches.h b/chrome/common/chrome_switches.h
index 9507ae88360b6d24f21f4b9b20e215fb582c1a86..a0cef19a7d329e8a50a25a0abce5bb378bc943e9 100644
--- a/chrome/common/chrome_switches.h
+++ b/chrome/common/chrome_switches.h
@@ -148,8 +148,6 @@ extern const char kEnableAlternativeServices[];
 extern const char kEnableSessionCrashedBubble[];
 extern const char kEnableSettingsWindow[];
 extern const char kDisableSettingsWindow[];
-extern const char kEnableSimplifiedFullscreenUI[];
-extern const char kDisableSimplifiedFullscreenUI[];
 extern const char kEnableSiteEngagementAppBanner[];
 extern const char kEnableSiteEngagementEvictionPolicy[];
 extern const char kEnableSiteEngagementService[];

[asvi...@chromium.org]

lgtm



https://codereview.chromium.org/1685043002/

[asvi...@chromium.org]

In terms of order of this change vs. the server-side change - there's no danger
in this case in landing server side change earlier - but obviously it will have
no effect until clients have the client-side changes to actually do something
based on it.

https://codereview.chromium.org/1685043002/

[sch...@chromium.org]

LGTM - but it's the first time I've seen use of the feautre_list. BTW, the bug
linked seems too broad -- there should probably be one specifically for enabling
on mac views.

https://codereview.chromium.org/1685043002/

[tap...@chromium.org]

Thanks!

On 2016/02/11 17:08:58, scheib wrote:
> LGTM - but it's the first time I've seen use of the feautre_list. BTW, the bug
> linked seems too broad -- there should probably be one specifically for
enabling
> on mac views.

I've made http://crbug.com/586304 to track the Mac-specific Eng work required. I
had http://crbug.com/585009 which is mac-views-specific, but it's a restricted
launch bug, so it should be neater this way. I've also linked a more detailed
design doc we have specific to the mac views stuff at
http://crbug.com/585009#c2. (for non-mac platforms, there is Launch=
http://crbug.com/585002 Eng= http://crbug.com/352425 but that Eng bug is epic).

https://codereview.chromium.org/1685043002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1685043002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1685043002/20001


https://codereview.chromium.org/1685043002/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #2 (id:20001)

https://codereview.chromium.org/1685043002/

[commit-bot@chromium.org via codereview.chromium.org]

Patchset 2 (id:??) landed as
https://crrev.com/a68ddbf069fae1f348b8038f65dd424f8bc92e93
Cr-Commit-Position: refs/heads/master@{#375048}

https://codereview.chromium.org/1685043002/