Re-add backspace-goes-back as a default disabled finch trial

chrome/browser/ui/browser_command_controller.cc

 // Copyright (c) 2012 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/browser_command_controller.h"
 
 #include <stddef.h>
 
 #include <string>
 
 #include "base/command_line.h"
 #include "base/debug/debugging_flags.h"
 #include "base/debug/profiler.h"
+#include "base/feature_list.h"
 #include "base/macros.h"
 #include "build/build_config.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/defaults.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_util.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
 #include "chrome/browser/prefs/incognito_mode_prefs.h"
@@ skipping 86 matching lines @@
   if (entry->GetVirtualURL().SchemeIs(content::kViewSourceScheme))
     return entry->GetURL().SchemeIs(content::kChromeUIScheme);
 
   return false;
 }
 
 }  // namespace
 
 namespace chrome {
 
+const base::Feature kBackspaceGoesBackFeature {
+  "BackspaceGoesBack", base::FEATURE_DISABLED_BY_DEFAULT
+};
+
 ///////////////////////////////////////////////////////////////////////////////
 // BrowserCommandController, public:
 
 BrowserCommandController::BrowserCommandController(Browser* browser)
     : browser_(browser),
       command_updater_(this),
       block_command_execution_(false),
       last_blocked_command_id_(-1),
       last_blocked_command_disposition_(CURRENT_TAB) {
   browser_->tab_strip_model()->AddObserver(this);
@@ skipping 178 matching lines @@
     DCHECK_EQ(last_blocked_command_id_, -1);
     last_blocked_command_id_ = id;
     last_blocked_command_disposition_ = disposition;
     return;
   }
 
   // The order of commands in this switch statement must match the function
   // declaration order in browser.h!
   switch (id) {
     // Navigation commands
+    case IDC_BACKSPACE_BACK:
+      if (base::FeatureList::IsEnabled(kBackspaceGoesBackFeature))
+        GoBack(browser_, disposition);
+      break;
+    case IDC_BACKSPACE_FORWARD:
+      if (base::FeatureList::IsEnabled(kBackspaceGoesBackFeature))
+        GoForward(browser_, disposition);
+      break;
     case IDC_BACK:
       GoBack(browser_, disposition);
       break;

[Peter Kasting, 2016/05/19 09:28:30]

Nit: Slightly shorter, and abides by the comment about keeping the command order
exactly matching the function declaration order:

    case IDC_BACKSPACE_BACK:
      if (!base::FeatureList::IsEnabled(kBackspaceGoesBackFeature))
        break;
    case IDC_BACK:
      GoBack(browser_, disposition);
      break;

(& similar)

[ojan, 2016/05/20 16:33:48]

done

[Matt Giuca, 2016/05/23 01:12:01]

Um, this is kind of gross and I'm going to have to put it back the way it was in
my follow-up CL (which adds an else clause)... I have never seen a conditional
fall-through before.

[Matt Giuca, 2016/05/23 01:15:40]

Sorry, actually I can keep the conditional fall-through if it's desired. Seems
bad though. Will discuss on my follow-up CL
https://codereview.chromium.org/1983803002/

     case IDC_FORWARD:
       GoForward(browser_, disposition);
       break;
     case IDC_RELOAD:
       Reload(browser_, disposition);
       break;
     case IDC_RELOAD_CLEARING_CACHE:
       ClearCache(browser_);
       // FALL THROUGH
     case IDC_RELOAD_BYPASSING_CACHE:
@@ skipping 640 matching lines @@
   }
 }
 
 void BrowserCommandController::UpdateCommandsForTabState() {
   WebContents* current_web_contents =
       browser_->tab_strip_model()->GetActiveWebContents();
   if (!current_web_contents)  // May be NULL during tab restore.
     return;
 
   // Navigation commands
+  command_updater_.UpdateCommandEnabled(IDC_BACKSPACE_BACK,
+                                        CanGoBack(browser_));
+  command_updater_.UpdateCommandEnabled(IDC_BACKSPACE_FORWARD,

[Peter Kasting, 2016/05/19 09:28:30]

Nit: Swap this with next line to keep the backs/forwards together

[ojan, 2016/05/20 16:33:48]

done

+                                        CanGoForward(browser_));
   command_updater_.UpdateCommandEnabled(IDC_BACK, CanGoBack(browser_));
   command_updater_.UpdateCommandEnabled(IDC_FORWARD, CanGoForward(browser_));
   command_updater_.UpdateCommandEnabled(IDC_RELOAD, CanReload(browser_));
   command_updater_.UpdateCommandEnabled(IDC_RELOAD_BYPASSING_CACHE,
                                         CanReload(browser_));
   command_updater_.UpdateCommandEnabled(IDC_RELOAD_CLEARING_CACHE,
                                         CanReload(browser_));
 
   // Window management commands
   command_updater_.UpdateCommandEnabled(IDC_DUPLICATE_TAB,
@@ skipping 260 matching lines @@
 
 BrowserWindow* BrowserCommandController::window() {
   return browser_->window();
 }
 
 Profile* BrowserCommandController::profile() {
   return browser_->profile();
 }
 
 }  // namespace chrome