Add test to check search key based accelerators.

ash/common/accelerators/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 "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)
       return lhs.trigger_on_press < rhs.trigger_on_press;
     if (lhs.keycode != rhs.keycode)
       return lhs.keycode < rhs.keycode;
     return lhs.modifiers < rhs.modifiers;
     // Do not check |action|.
   }
 };
 
+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) {
   std::set<AcceleratorData, Cmp> accelerators;
   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);
   }
 }
 
 TEST(AcceleratorTableTest, CheckDuplicatedReservedActions) {
   std::set<AcceleratorAction> actions;
   for (size_t i = 0; i < kReservedActionsLength; ++i) {
     EXPECT_TRUE(actions.insert(kReservedActions[i]).second)
         << "Duplicated action: " << kReservedActions[i];
   }
 }
@@ skipping 26 matching lines @@
         << "Duplicated action: " << kRepeatableActions[i] << " at index: " << i;
   }
 }
 
 TEST(AcceleratorTableTest, CheckDeprecatedAccelerators) {
   std::set<AcceleratorData, Cmp> deprecated_actions;
   for (size_t i = 0; i < kDeprecatedAcceleratorsLength; ++i) {
     // 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;
   for (size_t i = 0; i < kDeprecatedAcceleratorsDataLength; ++i) {
     // There must never be any duplicated actions.
     const DeprecatedAcceleratorData& data = kDeprecatedAcceleratorsData[i];
     EXPECT_TRUE(actions.insert(data.action).second) << "Deprecated action: "
                                                     << data.action;
 
     // The UMA histogram name must be of the format "Ash.Accelerators.*"
     std::string uma_histogram(data.uma_histogram_name);
     EXPECT_TRUE(base::StartsWith(uma_histogram, "Ash.Accelerators.",
                                  base::CompareCase::SENSITIVE));
   }
 }
 
+// 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