Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2802583002:
Add test to check search key based accelerators. (Closed)
DescriptionAdd test to check search key based accelerators.
For new added accelerators, we want to ensure it is search key based by
adding the test.
BUG=706018, 706462
TEST=AcceleratorTableTest.CheckSearchBasedAccelerators
Review-Url: https://codereview.chromium.org/2802583002
Cr-Commit-Position: refs/heads/master@{#462753}
Committed: https://chromium.googlesource.com/chromium/src/+/77744550580e6f83fa1ada19e8f96722b911640f
Patch Set 1 #
Total comments: 8
Patch Set 2 : Changed based on comments in patch 1. #
Total comments: 6
Patch Set 3 : Fixed nits in patch 2. #
Total comments: 4
Patch Set 4 : Change test method to number and hash based. #
Total comments: 19
Patch Set 5 : Update the hash method. #
Total comments: 1
Messages
Total messages: 44 (18 generated)
wutao@chromium.org changed reviewers: + afakhry@chromium.org, oshima@chromium.org
Hi Ahmed, Could you please have a look. Thanks. Tao
https://codereview.chromium.org/2802583002/diff/1/ash/common/accelerators/acc... File ash/common/accelerators/accelerator_table_unittest.cc (right): https://codereview.chromium.org/2802583002/diff/1/ash/common/accelerators/acc... ash/common/accelerators/accelerator_table_unittest.cc:29: const AcceleratorData kAcceleratorDataAchived20170405[] = { Please add a comment explaining what this snapshot is, why it was added here, and what engineers should do if a new shortcut was added. https://codereview.chromium.org/2802583002/diff/1/ash/common/accelerators/acc... ash/common/accelerators/accelerator_table_unittest.cc:29: const AcceleratorData kAcceleratorDataAchived20170405[] = { We can keep this snapshot of only the non-Search-based accelerators. Consider it as a white listed accelerators that are allowed to have no Search in their modifiers. https://codereview.chromium.org/2802583002/diff/1/ash/common/accelerators/acc... ash/common/accelerators/accelerator_table_unittest.cc:32: // Shortcuts for Japanese IME. You don't need to include these comments here. Please remove them all. https://codereview.chromium.org/2802583002/diff/1/ash/common/accelerators/acc... ash/common/accelerators/accelerator_table_unittest.cc:276: << "Non-command key based accelerator: " << entry.trigger_on_press Use Search instead of command. Search-based accelerator.
Description was changed from ========== Add test to check search key based accelerators. For new added accelerators, we want to ensure it is search key based by adding the test. BUG=706462 TEST=AcceleratorTableTest.CheckSearchKeyBasedAccelerators ========== to ========== Add test to check search key based accelerators. For new added accelerators, we want to ensure it is search key based by adding the test. BUG=706462 TEST=AcceleratorTableTest.CheckSearchBasedAccelerators ==========
wutao@chromium.org changed reviewers: + jamescook@chromium.org - oshima@chromium.org
Hi Ahmed, Please have another look. Thanks, Tao https://codereview.chromium.org/2802583002/diff/1/ash/common/accelerators/acc... File ash/common/accelerators/accelerator_table_unittest.cc (right): https://codereview.chromium.org/2802583002/diff/1/ash/common/accelerators/acc... ash/common/accelerators/accelerator_table_unittest.cc:29: const AcceleratorData kAcceleratorDataAchived20170405[] = { On 2017/04/05 16:01:32, afakhry wrote: > We can keep this snapshot of only the non-Search-based accelerators. Consider it > as a white listed accelerators that are allowed to have no Search in their > modifiers. Done. https://codereview.chromium.org/2802583002/diff/1/ash/common/accelerators/acc... ash/common/accelerators/accelerator_table_unittest.cc:29: const AcceleratorData kAcceleratorDataAchived20170405[] = { On 2017/04/05 16:01:32, afakhry wrote: > Please add a comment explaining what this snapshot is, why it was added here, > and what engineers should do if a new shortcut was added. Done. https://codereview.chromium.org/2802583002/diff/1/ash/common/accelerators/acc... ash/common/accelerators/accelerator_table_unittest.cc:32: // Shortcuts for Japanese IME. On 2017/04/05 16:01:32, afakhry wrote: > You don't need to include these comments here. Please remove them all. Done. https://codereview.chromium.org/2802583002/diff/1/ash/common/accelerators/acc... ash/common/accelerators/accelerator_table_unittest.cc:276: << "Non-command key based accelerator: " << entry.trigger_on_press On 2017/04/05 16:01:32, afakhry wrote: > Use Search instead of command. > Search-based accelerator. Done.
derat@chromium.org changed reviewers: + derat@chromium.org
high-level comment: engineers shouldn't be adding search-based accelerators either without UX approval. https://codereview.chromium.org/2802583002/diff/20001/ash/common/accelerators... File ash/common/accelerators/accelerator_table_unittest.cc (right): https://codereview.chromium.org/2802583002/diff/20001/ash/common/accelerators... ash/common/accelerators/accelerator_table_unittest.cc:29: // This table snapshot was added on Apr. 05, 2017 to white list these nit: use iso 8601 dates, i.e. 2017-04-05 https://codereview.chromium.org/2802583002/diff/20001/ash/common/accelerators... ash/common/accelerators/accelerator_table_unittest.cc:33: // to include the Search key. nit: all new accelerators should be approved by UX, so i'd mention that here as well https://codereview.chromium.org/2802583002/diff/20001/ash/common/accelerators... ash/common/accelerators/accelerator_table_unittest.cc:245: << entry.keycode << ", " << (entry.modifiers & ui::EF_SHIFT_DOWN) i'd label all of these fields since the message seems like it'll be hard to interpret otherwise, e.g. << " keycode=" << entry.keycode << " shift=" << (entry.modifiers & ui::EF_SHIFT_DOWN) << ... (i don't think you need to log trigger_on_press)
wutao@chromium.org changed reviewers: - jamescook@chromium.org
Hi Daniel, Please have another look at the changes. Thank you, Tao https://codereview.chromium.org/2802583002/diff/20001/ash/common/accelerators... File ash/common/accelerators/accelerator_table_unittest.cc (right): https://codereview.chromium.org/2802583002/diff/20001/ash/common/accelerators... ash/common/accelerators/accelerator_table_unittest.cc:29: // This table snapshot was added on Apr. 05, 2017 to white list these On 2017/04/05 18:08:31, Daniel Erat wrote: > nit: use iso 8601 dates, i.e. 2017-04-05 Done. https://codereview.chromium.org/2802583002/diff/20001/ash/common/accelerators... ash/common/accelerators/accelerator_table_unittest.cc:33: // to include the Search key. On 2017/04/05 18:08:31, Daniel Erat wrote: > nit: all new accelerators should be approved by UX, so i'd mention that here as > well Done. https://codereview.chromium.org/2802583002/diff/20001/ash/common/accelerators... ash/common/accelerators/accelerator_table_unittest.cc:245: << entry.keycode << ", " << (entry.modifiers & ui::EF_SHIFT_DOWN) On 2017/04/05 18:08:31, Daniel Erat wrote: > i'd label all of these fields since the message seems like it'll be hard to > interpret otherwise, e.g. > > << " keycode=" << entry.keycode > << " shift=" << (entry.modifiers & ui::EF_SHIFT_DOWN) > << ... > > (i don't think you need to log trigger_on_press) Changed this and other old test as well.
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
derat@chromium.org changed reviewers: + jamescook@chromium.org, oshima@chromium.org, sky@chromium.org
[adding other ash owners] i'm still not convinced about this approach. how will this be more effective than a comment above the accelerator table stating that new chrome os accelerators must use the search key? would it be better to ensure that ash owners (who should be reviewing all accelerator changes) are aware of the policy? https://codereview.chromium.org/2802583002/diff/40001/ash/common/accelerators... File ash/common/accelerators/accelerator_table_unittest.cc (right): https://codereview.chromium.org/2802583002/diff/40001/ash/common/accelerators... ash/common/accelerators/accelerator_table_unittest.cc:26: const int kDebugModifier = this looks like it's only used in one place. since the table explicitly shouldn't be updated, would it be cleaner to remove this and just inline the values for the single entry that needs them? https://codereview.chromium.org/2802583002/diff/40001/ash/common/accelerators... ash/common/accelerators/accelerator_table_unittest.cc:162: << " trigger_on_press=" << entry.trigger_on_press since this logging code appears multiple times, how about adding an AcceleratorDataToString() helper method in the anon namespace at the top of the file that formats and returns a string?
Description was changed from ========== Add test to check search key based accelerators. For new added accelerators, we want to ensure it is search key based by adding the test. BUG=706462 TEST=AcceleratorTableTest.CheckSearchBasedAccelerators ========== to ========== Add test to check search key based accelerators. For new added accelerators, we want to ensure it is search key based by adding the test. BUG=706018,706462 TEST=AcceleratorTableTest.CheckSearchBasedAccelerators ==========
On 2017/04/05 23:32:51, Daniel Erat wrote: > [adding other ash owners] > > i'm still not convinced about this approach. how will this be more effective > than a comment above the accelerator table stating that new chrome os > accelerators must use the search key? I don't trust human being :p The real answer is that the cost of failing to catch this is very high (it takes several MSs to replace, confuses users), so I think it worth it. That's being said, this test is way more complicated than necessary. Why don't we just check the number of accelerator that does NOT use search key and make sure it's not added? > would it be better to ensure that ash owners (who should be reviewing all > accelerator changes) are aware of the policy? > > https://codereview.chromium.org/2802583002/diff/40001/ash/common/accelerators... > File ash/common/accelerators/accelerator_table_unittest.cc (right): > > https://codereview.chromium.org/2802583002/diff/40001/ash/common/accelerators... > ash/common/accelerators/accelerator_table_unittest.cc:26: const int > kDebugModifier = > this looks like it's only used in one place. since the table explicitly > shouldn't be updated, would it be cleaner to remove this and just inline the > values for the single entry that needs them? > > https://codereview.chromium.org/2802583002/diff/40001/ash/common/accelerators... > ash/common/accelerators/accelerator_table_unittest.cc:162: << " > trigger_on_press=" << entry.trigger_on_press > since this logging code appears multiple times, how about adding an > AcceleratorDataToString() helper method in the anon namespace at the top of the > file that formats and returns a string?
On 2017/04/05 23:32:51, Daniel Erat wrote: > [adding other ash owners] > > i'm still not convinced about this approach. how will this be more effective > than a comment above the accelerator table stating that new chrome os > accelerators must use the search key? > > would it be better to ensure that ash owners (who should be reviewing all > accelerator changes) are aware of the policy? > As discussed in BUG=706018, 706462, tests are suggested for reliability.
On 2017/04/05 23:51:32, oshima wrote: > On 2017/04/05 23:32:51, Daniel Erat wrote: > > [adding other ash owners] > > > > i'm still not convinced about this approach. how will this be more effective > > than a comment above the accelerator table stating that new chrome os > > accelerators must use the search key? > > I don't trust human being :p > > The real answer is that the cost of failing to catch this is very high (it takes > several MSs to replace, > confuses users), so I think it worth it. > > That's being said, this test is way more complicated than necessary. Why don't > we just check > the number of accelerator that does NOT use search key and make sure it's not > added? This method will be much easier. But one of the corner cases is that, developers remove and add one non-search-based the accelerator, the total number of accelerators will not change.
On 2017/04/05 23:52:55, wutao wrote: > On 2017/04/05 23:32:51, Daniel Erat wrote: > > [adding other ash owners] > > > > i'm still not convinced about this approach. how will this be more effective > > than a comment above the accelerator table stating that new chrome os > > accelerators must use the search key? > > > > would it be better to ensure that ash owners (who should be reviewing all > > accelerator changes) are aware of the policy? > > > As discussed in BUG=706018, 706462, tests are suggested for reliability. i'm a fan of tests too! i just don't like ones that are testing that data in one place matches data in another place. :-) i agree with oshima that checking that the count doesn't increase would be a better approach (with the downside that it won't catch cases where someone changes one non-search accelerator to be a different non-search accelerator in a single change).
On 2017/04/06 00:04:16, Daniel Erat wrote: > On 2017/04/05 23:52:55, wutao wrote: > > On 2017/04/05 23:32:51, Daniel Erat wrote: > > > [adding other ash owners] > > > > > > i'm still not convinced about this approach. how will this be more effective > > > than a comment above the accelerator table stating that new chrome os > > > accelerators must use the search key? > > > > > > would it be better to ensure that ash owners (who should be reviewing all > > > accelerator changes) are aware of the policy? > > > > > As discussed in BUG=706018, 706462, tests are suggested for reliability. > > i'm a fan of tests too! i just don't like ones that are testing that data in one > place matches data in another place. :-) > > i agree with oshima that checking that the count doesn't increase would be a > better approach (with the downside that it won't catch cases where someone > changes one non-search accelerator to be a different non-search accelerator in a > single change). +1 for automated test. I defer to you guys on choosing the approach.
On 2017/04/05 23:59:22, wutao wrote: > On 2017/04/05 23:51:32, oshima wrote: > > On 2017/04/05 23:32:51, Daniel Erat wrote: > > > [adding other ash owners] > > > > > > i'm still not convinced about this approach. how will this be more effective > > > than a comment above the accelerator table stating that new chrome os > > > accelerators must use the search key? > > > > I don't trust human being :p > > > > The real answer is that the cost of failing to catch this is very high (it > takes > > several MSs to replace, > > confuses users), so I think it worth it. > > > > That's being said, this test is way more complicated than necessary. Why don't > > we just check > > the number of accelerator that does NOT use search key and make sure it's not > > added? > > This method will be much easier. But one of the corner cases is that, developers > remove and add one non-search-based the accelerator, the total number of > accelerators will not change. Get the list of non search key accelerators, sort it (just have to be stable), then compute the fingerprint of them, and compare. That should be able to catch such case. You can print different message depending on the type of change. If # of such accelerator decreased, you can just suggest to update the fingerprint.
Hi Oshima and Daniel, I changed the testing method to number and hash based. Please check the method again. Thanks, Tao https://codereview.chromium.org/2802583002/diff/40001/ash/common/accelerators... File ash/common/accelerators/accelerator_table_unittest.cc (right): https://codereview.chromium.org/2802583002/diff/40001/ash/common/accelerators... ash/common/accelerators/accelerator_table_unittest.cc:26: const int kDebugModifier = On 2017/04/05 23:32:51, Daniel Erat wrote: > this looks like it's only used in one place. since the table explicitly > shouldn't be updated, would it be cleaner to remove this and just inline the > values for the single entry that needs them? Changed method to number and hash based, so do not need this any more. https://codereview.chromium.org/2802583002/diff/40001/ash/common/accelerators... ash/common/accelerators/accelerator_table_unittest.cc:162: << " trigger_on_press=" << entry.trigger_on_press On 2017/04/05 23:32:51, Daniel Erat wrote: > since this logging code appears multiple times, how about adding an > AcceleratorDataToString() helper method in the anon namespace at the top of the > file that formats and returns a string? Done.
On 2017/04/06 02:00:55, oshima wrote: > On 2017/04/05 23:59:22, wutao wrote: > > On 2017/04/05 23:51:32, oshima wrote: > > > On 2017/04/05 23:32:51, Daniel Erat wrote: > > > > [adding other ash owners] > > > > > > > > i'm still not convinced about this approach. how will this be more > effective > > > > than a comment above the accelerator table stating that new chrome os > > > > accelerators must use the search key? > > > > > > I don't trust human being :p > > > > > > The real answer is that the cost of failing to catch this is very high (it > > takes > > > several MSs to replace, > > > confuses users), so I think it worth it. > > > > > > That's being said, this test is way more complicated than necessary. Why > don't > > > we just check > > > the number of accelerator that does NOT use search key and make sure it's > not > > > added? > > > > This method will be much easier. But one of the corner cases is that, > developers > > remove and add one non-search-based the accelerator, the total number of > > accelerators will not change. > > Get the list of non search key accelerators, sort it (just have to be stable), > then compute the fingerprint of them, and compare. That should be able to catch > such case. > > You can print different message depending on the type of change. If # of such > accelerator > decreased, you can just suggest to update the fingerprint. Implemented as suggested.
Nice! https://codereview.chromium.org/2802583002/diff/60001/ash/common/accelerators... File ash/common/accelerators/accelerator_table_unittest.cc (right): https://codereview.chromium.org/2802583002/diff/60001/ash/common/accelerators... ash/common/accelerators/accelerator_table_unittest.cc:8: #include "base/bit_cast.h" You don't need this header, do you? https://codereview.chromium.org/2802583002/diff/60001/ash/common/accelerators... ash/common/accelerators/accelerator_table_unittest.cc:20: const int kNonSearchAcceleratorsNum = 92; constexpr https://codereview.chromium.org/2802583002/diff/60001/ash/common/accelerators... ash/common/accelerators/accelerator_table_unittest.cc:22: const std::string kNonSearchAcceleratorsHash = 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...
https://codereview.chromium.org/2802583002/diff/60001/ash/common/accelerators... File ash/common/accelerators/accelerator_table_unittest.cc (right): https://codereview.chromium.org/2802583002/diff/60001/ash/common/accelerators... ash/common/accelerators/accelerator_table_unittest.cc:19: // The snapshot of non-Search-based acclerators number. nit: s/acclerators/accelerators/, but something like this may be clearer: // The number of non-Search-based accelerators as of 2017-MM-DD. https://codereview.chromium.org/2802583002/diff/60001/ash/common/accelerators... ash/common/accelerators/accelerator_table_unittest.cc:21: // The snapshot of non-Search-based acclerators hash. also change this to something like: // The hash of non-Search-based accelerators as of 2017-MM-DD. // See HashAcceleratorData(). https://codereview.chromium.org/2802583002/diff/60001/ash/common/accelerators... ash/common/accelerators/accelerator_table_unittest.cc:22: const std::string kNonSearchAcceleratorsHash = On 2017/04/06 17:50:29, afakhry wrote: > 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... 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. https://codereview.chromium.org/2802583002/diff/60001/ash/common/accelerators... ash/common/accelerators/accelerator_table_unittest.cc:38: "trigger_on_press=%s keycode=%d SHIFT=%s CONTROL=%s ALT=%s SEARCH=%s", 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. https://codereview.chromium.org/2802583002/diff/60001/ash/common/accelerators... ash/common/accelerators/accelerator_table_unittest.cc:50: sizeof(accelerators[0]) * accelerators.size(), &digest); 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. https://codereview.chromium.org/2802583002/diff/60001/ash/common/accelerators... ash/common/accelerators/accelerator_table_unittest.cc:137: EXPECT_TRUE(accelerators_number <= kNonSearchAcceleratorsNum) use EXPECT_LE instead https://codereview.chromium.org/2802583002/diff/60001/ash/common/accelerators... ash/common/accelerators/accelerator_table_unittest.cc:147: "consider using Search-based modifier and get approval from UX.\n" 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 ...
Hi derat@ and afakhry@ Please look again about the new hash method. Thanks. wutao@ https://codereview.chromium.org/2802583002/diff/60001/ash/common/accelerators... File ash/common/accelerators/accelerator_table_unittest.cc (right): https://codereview.chromium.org/2802583002/diff/60001/ash/common/accelerators... ash/common/accelerators/accelerator_table_unittest.cc:8: #include "base/bit_cast.h" On 2017/04/06 17:50:29, afakhry wrote: > You don't need this header, do you? I do not need it, removed. https://codereview.chromium.org/2802583002/diff/60001/ash/common/accelerators... ash/common/accelerators/accelerator_table_unittest.cc:19: // The snapshot of non-Search-based acclerators number. On 2017/04/06 18:04:55, Daniel Erat wrote: > nit: s/acclerators/accelerators/, but something like this may be clearer: > > // The number of non-Search-based accelerators as of 2017-MM-DD. Done. https://codereview.chromium.org/2802583002/diff/60001/ash/common/accelerators... ash/common/accelerators/accelerator_table_unittest.cc:20: const int kNonSearchAcceleratorsNum = 92; On 2017/04/06 17:50:29, afakhry wrote: > constexpr Done. https://codereview.chromium.org/2802583002/diff/60001/ash/common/accelerators... ash/common/accelerators/accelerator_table_unittest.cc:21: // The snapshot of non-Search-based acclerators hash. On 2017/04/06 18:04:56, Daniel Erat wrote: > also change this to something like: > > // The hash of non-Search-based accelerators as of 2017-MM-DD. > // See HashAcceleratorData(). Done. https://codereview.chromium.org/2802583002/diff/60001/ash/common/accelerators... ash/common/accelerators/accelerator_table_unittest.cc:22: const std::string kNonSearchAcceleratorsHash = On 2017/04/06 17:50:29, afakhry wrote: > 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... Done. https://codereview.chromium.org/2802583002/diff/60001/ash/common/accelerators... ash/common/accelerators/accelerator_table_unittest.cc:22: const std::string kNonSearchAcceleratorsHash = On 2017/04/06 18:04:56, Daniel Erat wrote: > On 2017/04/06 17:50:29, afakhry wrote: > > 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... > > 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. Acknowledged. https://codereview.chromium.org/2802583002/diff/60001/ash/common/accelerators... ash/common/accelerators/accelerator_table_unittest.cc:38: "trigger_on_press=%s keycode=%d SHIFT=%s CONTROL=%s ALT=%s SEARCH=%s", On 2017/04/06 18:04:55, Daniel Erat wrote: > 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. 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. https://codereview.chromium.org/2802583002/diff/60001/ash/common/accelerators... ash/common/accelerators/accelerator_table_unittest.cc:50: sizeof(accelerators[0]) * accelerators.size(), &digest); On 2017/04/06 18:04:55, Daniel Erat wrote: > 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. Nice! done. https://codereview.chromium.org/2802583002/diff/60001/ash/common/accelerators... ash/common/accelerators/accelerator_table_unittest.cc:137: EXPECT_TRUE(accelerators_number <= kNonSearchAcceleratorsNum) On 2017/04/06 18:04:55, Daniel Erat wrote: > use EXPECT_LE instead Done.
lgtm thanks! https://codereview.chromium.org/2802583002/diff/80001/ash/common/accelerators... File ash/common/accelerators/accelerator_table_unittest.cc (right): https://codereview.chromium.org/2802583002/diff/80001/ash/common/accelerators... ash/common/accelerators/accelerator_table_unittest.cc:39: "action=%d", just using the integer value here like this is fine
lgtm. Thank you for handling this! :)
Hi Oshima, Could you please have a look. Thanks. Tao
On 2017/04/07 00:34:21, wutao wrote: > Hi Oshima, > > Could you please have a look. > > Thanks. > Tao i'm guessing that oshima is fine with this since you implemented what he asked for, but you may want to still consider sorting the accelerators (if they're comparable), which he also recommended. (i'm not sure how necessary that is, though; it's probably fine to make people update the hash if they decide to reorder the entries.)
On 2017/04/07 00:45:48, Daniel Erat wrote: > On 2017/04/07 00:34:21, wutao wrote: > > Hi Oshima, > > > > Could you please have a look. > > > > Thanks. > > Tao > > i'm guessing that oshima is fine with this since you implemented what he asked > for, but you may want to still consider sorting the accelerators (if they're > comparable), which he also recommended. (i'm not sure how necessary that is, > though; it's probably fine to make people update the hash if they decide to > reorder the entries.) I have sorted in the test :) std::stable_sort(non_search_accelerators.begin(), non_search_accelerators.end(), Cmp());
oh, sorry. i remember seeing the sorting earlier now. :-) i think you should go ahead and send this to the CQ.
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/07 01:17:16, Daniel Erat wrote: > oh, sorry. i remember seeing the sorting earlier now. :-) > > i think you should go ahead and send this to the CQ. yep, please go ahead. you don't need my review.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by wutao@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491537380984890,
"parent_rev": "a5023a0e9ba7b9f0c3137d90a3a830ecfb362414", "commit_rev":
"77744550580e6f83fa1ada19e8f96722b911640f"}
Message was sent while issue was closed.
Description was changed from ========== Add test to check search key based accelerators. For new added accelerators, we want to ensure it is search key based by adding the test. BUG=706018,706462 TEST=AcceleratorTableTest.CheckSearchBasedAccelerators ========== to ========== Add test to check search key based accelerators. For new added accelerators, we want to ensure it is search key based by adding the test. BUG=706018,706462 TEST=AcceleratorTableTest.CheckSearchBasedAccelerators Review-Url: https://codereview.chromium.org/2802583002 Cr-Commit-Position: refs/heads/master@{#462753} Committed: https://chromium.googlesource.com/chromium/src/+/77744550580e6f83fa1ada19e8f9... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/77744550580e6f83fa1ada19e8f9... |
