Do not include Ctrl+N and Ctrl+Shift+N in...

Do not include Ctrl+N and Ctrl+Shift+N in chrome/browser/ui/views/accelerator_table.cc when USE_ASH is #defined.

The shortcuts should be excluded since they're already included in ash/accelerators/accelerator_table.cc. Removing this kind of duplication is necessary for implementing crbug.com/123856 correctly. Along with the change, add unit tests for preventing the same mistake in the future.

BUG=123856
TEST=ran unit_tests

Review URL: http://codereview.chromium.org/10177002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@133651 0039d316-1c4b-4281-b951-d872f2087c98

ash/accelerators/accelerator_table.h

@@ -6,6 +6,7 @@
 #define ASH_ACCELERATORS_ACCELERATOR_TABLE_H_
 #pragma once
 
+#include "ash/ash_export.h"
 #include "ui/aura/event.h"
 
 namespace ash {
@@ -71,10 +72,10 @@ struct AcceleratorData {
 };
 
 // Accelerators handled by AcceleratorController.
-extern const AcceleratorData kAcceleratorData[];
+ASH_EXPORT extern const AcceleratorData kAcceleratorData[];
 
 // The number of elements in kAcceleratorData.
-extern const size_t kAcceleratorDataLength;
+ASH_EXPORT extern const size_t kAcceleratorDataLength;
 
 // Actions allowed while user is not signed in or screen is locked.
 extern const AcceleratorAction kActionsAllowedAtLoginScreen[];

chrome/browser/ui/views/accelerator_table.cc

@@ -92,9 +92,13 @@ const AcceleratorMapping kAcceleratorMap[] = {
   { ui::VKEY_F12,            false, false, false, IDC_DEV_TOOLS },
   { ui::VKEY_J,              true,  true,  false, IDC_DEV_TOOLS_CONSOLE },
   { ui::VKEY_C,              true,  true,  false, IDC_DEV_TOOLS_INSPECT },
+#if !defined(USE_ASH)
   { ui::VKEY_N,              true,  true,  false, IDC_NEW_INCOGNITO_WINDOW },
+#endif
   { ui::VKEY_T,              false, true,  false, IDC_NEW_TAB },
+#if !defined(USE_ASH)
   { ui::VKEY_N,              false, true,  false, IDC_NEW_WINDOW },
+#endif
   { ui::VKEY_O,              false, true,  false, IDC_OPEN_FILE },
   { ui::VKEY_P,              false, true,  false, IDC_PRINT},
   { ui::VKEY_P,              true,  true,  false, IDC_ADVANCED_PRINT},

chrome/browser/ui/views/accelerator_table.h

-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -6,26 +6,27 @@
 #define CHROME_BROWSER_UI_VIEWS_ACCELERATOR_TABLE_H_
 #pragma once
 
-#include <stdio.h>
+#include <stddef.h>
 
 #include "ui/base/keycodes/keyboard_codes.h"
 
 // This contains the list of accelerators for the Aura implementation.
 namespace browser {
 
-  struct AcceleratorMapping {
-    ui::KeyboardCode keycode;
-    bool shift_pressed;
-    bool ctrl_pressed;
-    bool alt_pressed;
-    int command_id;
-  };
+struct AcceleratorMapping {
+  ui::KeyboardCode keycode;
+  bool shift_pressed;
+  bool ctrl_pressed;
+  bool alt_pressed;
+  int command_id;
+};
 
-  // The list of accelerators.
-  extern const AcceleratorMapping kAcceleratorMap[];
+// The list of accelerators.
+extern const AcceleratorMapping kAcceleratorMap[];
 
-  // The numbers of elements in kAcceleratorMap.
-  extern const size_t kAcceleratorMapLength;
-}
+// The numbers of elements in kAcceleratorMap.
+extern const size_t kAcceleratorMapLength;
+
+}  // namespace browser
 
 #endif  // CHROME_BROWSER_UI_VIEWS_ACCELERATOR_TABLE_H_

chrome/browser/ui/views/accelerator_table_unittest.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 <set>
+
+#include "base/basictypes.h"
+#include "chrome/browser/ui/views/accelerator_table.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+#if defined(USE_ASH)
+#include "ash/accelerators/accelerator_table.h"
+#endif  // USE_ASH
+
+namespace {
+
+struct Cmp {
+  bool operator()(const browser::AcceleratorMapping& lhs,
+                  const browser::AcceleratorMapping& rhs) {
+    if (lhs.keycode != rhs.keycode)
+      return lhs.keycode < rhs.keycode;
+    if (lhs.shift_pressed != rhs.shift_pressed)
+      return lhs.shift_pressed < rhs.shift_pressed;
+    if (lhs.ctrl_pressed != rhs.ctrl_pressed)
+      return lhs.ctrl_pressed < rhs.ctrl_pressed;
+    return lhs.alt_pressed < rhs.alt_pressed;
+    // Do not check |command_id|.
+  }
+};
+
+}  // namespace
+
+TEST(AcceleratorTableTest, CheckDuplicatedAccelerators) {
+  std::set<browser::AcceleratorMapping, Cmp> acclerators;
+  for (size_t i = 0; i < browser::kAcceleratorMapLength; ++i) {
+    const browser::AcceleratorMapping& entry = browser::kAcceleratorMap[i];
+    EXPECT_TRUE(acclerators.insert(entry).second)
+        << "Duplicated accelerator: " << entry.keycode << ", "
+        << entry.shift_pressed << ", " << entry.ctrl_pressed << ", "
+        << entry.alt_pressed;
+  }
+}
+
+#if defined(USE_ASH)
+TEST(AcceleratorTableTest, CheckDuplicatedAcceleratorsAsh) {
+  std::set<browser::AcceleratorMapping, Cmp> acclerators;
+  for (size_t i = 0; i < browser::kAcceleratorMapLength; ++i) {
+    const browser::AcceleratorMapping& entry = browser::kAcceleratorMap[i];
+    acclerators.insert(entry);
+  }
+  for (size_t i = 0; i < ash::kAcceleratorDataLength; ++i) {
+    const ash::AcceleratorData& ash_entry = ash::kAcceleratorData[i];
+    if (!ash_entry.trigger_on_press)
+      continue;  // kAcceleratorMap does not have any release accelerators.
+    browser::AcceleratorMapping entry;
+    entry.keycode = ash_entry.keycode;
+    entry.shift_pressed = ash_entry.shift;
+    entry.ctrl_pressed = ash_entry.ctrl;
+    entry.alt_pressed = ash_entry.alt;
+    entry.command_id = 0;  // dummy
+    EXPECT_TRUE(acclerators.insert(entry).second)
+        << "Duplicated accelerator: " << entry.keycode << ", "
+        << entry.shift_pressed << ", " << entry.ctrl_pressed << ", "
+        << entry.alt_pressed;
+  }
+}
+#endif  // USE_ASH

chrome/chrome_tests.gypi

@@ -1888,6 +1888,7 @@
         'browser/ui/toolbar/encoding_menu_controller_unittest.cc',
         'browser/ui/toolbar/toolbar_model_unittest.cc',
         'browser/ui/toolbar/wrench_menu_model_unittest.cc',
+        'browser/ui/views/accelerator_table_unittest.cc',
         'browser/ui/views/accessibility_event_router_views_unittest.cc',
         'browser/ui/views/ash/app_list/app_list_model_builder_unittest.cc',
         'browser/ui/views/ash/key_rewriter_unittest.cc',