Add test to check search key based accelerators.

ash/common/accelerators/accelerator_table_unittest.cc

Index: ash/common/accelerators/accelerator_table_unittest.cc
diff --git a/ash/common/accelerators/accelerator_table_unittest.cc b/ash/common/accelerators/accelerator_table_unittest.cc
index e469625545a0282e11a622e8ac2eff0d9b5d65da..a40a999cae1342fd8817b4ab3c2dc7b1c19d059d 100644
--- a/ash/common/accelerators/accelerator_table_unittest.cc
+++ b/ash/common/accelerators/accelerator_table_unittest.cc
@@ -5,13 +5,23 @@
 #include <set>
 
 #include "ash/common/accelerators/accelerator_table.h"
+#include "base/bit_cast.h"

[afakhry, 2017/04/06 17:50:29]

You don't need this header, do you?

[wutao, 2017/04/06 19:06:10]

I do not need it, removed.

+#include "base/macros.h"
+#include "base/md5.h"
 #include "base/strings/string_util.h"
+#include "base/strings/stringprintf.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace ash {
 
 namespace {
 
+// The snapshot of non-Search-based acclerators number.

[Daniel Erat, 2017/04/06 18:04:55]

nit: s/acclerators/accelerators/, but something like this may be clearer:

// The number of non-Search-based accelerators as of 2017-MM-DD.

[wutao, 2017/04/06 19:06:11]

Done.

+const int kNonSearchAcceleratorsNum = 92;

[afakhry, 2017/04/06 17:50:29]

constexpr

[wutao, 2017/04/06 19:06:10]

Done.

+// The snapshot of non-Search-based acclerators hash.

[Daniel Erat, 2017/04/06 18:04:56]

also change this to something like:

// The hash of non-Search-based accelerators as of 2017-MM-DD.
// See HashAcceleratorData().

[wutao, 2017/04/06 19:06:11]

Done.

+const std::string kNonSearchAcceleratorsHash =

[afakhry, 2017/04/06 17:50:29]

A global string object? Hmm, I haven't seen that being done before in Chromium.
It's probably fine, but normally we'd do:

constexpr char kNonSearchAcceleratorsHash[] = ...

https://cs.chromium.org/search/?q=%22constexpr+char%22+f:.*%5C.cc+package:%5E...

[Daniel Erat, 2017/04/06 18:04:56]

nope, this isn't allowed (since std::string isn't a POD type). see
https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

ahmed's suggestion is correct.

[wutao, 2017/04/06 19:06:10]

Acknowledged.

[wutao, 2017/04/06 19:06:10]

Done.

+    "a8801bb755aba2d1097a4d870386eab4";
+
 struct Cmp {
   bool operator()(const AcceleratorData& lhs, const AcceleratorData& rhs) {
     if (lhs.trigger_on_press != rhs.trigger_on_press)
@@ -23,6 +33,24 @@ struct Cmp {
   }
 };
 
+std::string AcceleratorDataToString(const AcceleratorData& accelerator) {
+  return base::StringPrintf(
+      "trigger_on_press=%s keycode=%d SHIFT=%s CONTROL=%s ALT=%s SEARCH=%s",

[Daniel Erat, 2017/04/06 18:04:55]

did you mean to include the action here as well? it seems like it'd be useful to
include.

also, this may be a bit more readable if you make shift, control, etc. lowercase
instead of uppercase.

[wutao, 2017/04/06 19:06:10]

Changed to lowercase.
For the action, is the enum value good enough or I need to write some function
to print the value as a text? e.g. print "BRIGHTNESS_DOWN" instead of 0.

+      accelerator.trigger_on_press ? "true" : "false", accelerator.keycode,
+      (accelerator.modifiers & ui::EF_SHIFT_DOWN) ? "true" : "false",
+      (accelerator.modifiers & ui::EF_CONTROL_DOWN) ? "true" : "false",
+      (accelerator.modifiers & ui::EF_ALT_DOWN) ? "true" : "false",
+      (accelerator.modifiers & ui::EF_COMMAND_DOWN) ? "true" : "false");
+}
+
+std::string HashAcceleratorData(
+    const std::vector<AcceleratorData> accelerators) {
+  base::MD5Digest digest;
+  base::MD5Sum(reinterpret_cast<const char*>(&accelerators[0]),
+               sizeof(accelerators[0]) * accelerators.size(), &digest);

[Daniel Erat, 2017/04/06 18:04:55]

will this have endianness issues?

i'm wondering if you could instead just loop through these, pass each to
AcceleratorDataToString(), and use MD5Context, MD5Update (called once for each
string), and MD5Final instead.

[wutao, 2017/04/06 19:06:10]

Nice! done.

+  return MD5DigestToBase16(digest);
+}
+
 }  // namespace
 
 TEST(AcceleratorTableTest, CheckDuplicatedAccelerators) {
@@ -30,10 +58,7 @@ TEST(AcceleratorTableTest, CheckDuplicatedAccelerators) {
   for (size_t i = 0; i < kAcceleratorDataLength; ++i) {
     const AcceleratorData& entry = kAcceleratorData[i];
     EXPECT_TRUE(accelerators.insert(entry).second)
-        << "Duplicated accelerator: " << entry.trigger_on_press << ", "
-        << entry.keycode << ", " << (entry.modifiers & ui::EF_SHIFT_DOWN)
-        << ", " << (entry.modifiers & ui::EF_CONTROL_DOWN) << ", "
-        << (entry.modifiers & ui::EF_ALT_DOWN);
+        << "Duplicated accelerator: " << AcceleratorDataToString(entry);
   }
 }
 
@@ -80,11 +105,8 @@ TEST(AcceleratorTableTest, CheckDeprecatedAccelerators) {
     // A deprecated action can never appear twice in the list.
     const AcceleratorData& entry = kDeprecatedAccelerators[i];
     EXPECT_TRUE(deprecated_actions.insert(entry).second)
-        << "Duplicate deprecated accelerator: " << entry.trigger_on_press
-        << ", " << entry.keycode << ", "
-        << (entry.modifiers & ui::EF_SHIFT_DOWN) << ", "
-        << (entry.modifiers & ui::EF_CONTROL_DOWN) << ", "
-        << (entry.modifiers & ui::EF_ALT_DOWN);
+        << "Duplicate deprecated accelerator: "
+        << AcceleratorDataToString(entry);
   }
 
   std::set<AcceleratorAction> actions;
@@ -101,4 +123,32 @@ TEST(AcceleratorTableTest, CheckDeprecatedAccelerators) {
   }
 }
 
+// All new accelerators should be Search-based and approved by UX.
+TEST(AcceleratorTableTest, CheckSearchBasedAccelerators) {
+  std::vector<AcceleratorData> non_search_accelerators;
+  for (size_t i = 0; i < kAcceleratorDataLength; ++i) {
+    const AcceleratorData& entry = kAcceleratorData[i];
+    if (entry.modifiers & ui::EF_COMMAND_DOWN)
+      continue;
+    non_search_accelerators.emplace_back(entry);
+  }
+
+  const size_t accelerators_number = non_search_accelerators.size();
+  EXPECT_TRUE(accelerators_number <= kNonSearchAcceleratorsNum)

[Daniel Erat, 2017/04/06 18:04:55]

use EXPECT_LE instead

[wutao, 2017/04/06 19:06:11]

Done.

+      << "All new accelerators should be Search-based and approved by UX.";
+
+  std::stable_sort(non_search_accelerators.begin(),
+                   non_search_accelerators.end(), Cmp());
+  const std::string non_search_accelerators_hash =
+      HashAcceleratorData(non_search_accelerators);
+
+  EXPECT_EQ(non_search_accelerators_hash, kNonSearchAcceleratorsHash)
+      << "If you are changing accelerator modifier to non-Search-based, please "
+         "consider using Search-based modifier and get approval from UX.\n"

[Daniel Erat, 2017/04/06 18:04:55]

i'd word this more strongly, e.g.

New accelerators must use the Search key. Please talk to the UX team. If you're
removing a non-Search-based accelerator, please update ...

+         "If you are removing non-Search-based accelerators, please update\n"
+      << "kNonSearchAcceleratorsNum=" << accelerators_number << " and "
+      << "kNonSearchAcceleratorsHash=\"" << non_search_accelerators_hash
+      << "\"";
+}
+
 }  // namespace ash