Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23542008:
Add a CodeToNativeKeycode helper that converts UIEvent code to native code. (Closed)
|
Created:
7 years, 3 months ago by weitao Modified:
7 years, 3 months ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, garykac+watch_chromium.org, sanjeevr, yusukes+watch_chromium.org, dcaiafa+watch_chromium.org, wez+watch_chromium.org, amit, hclam+watch_chromium.org, jam, penghuang+watch_chromium.org, joi+watch-content_chromium.org, nona+watch_chromium.org, lambroslambrou+watch_chromium.org, darin-cc_chromium.org, weitaosu+watch_chromium.org, rmsousa+watch_chromium.org, alexeypa+watch_chromium.org, James Su, sergeyu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd a CodeToNativeKeycode helper that converts UIEvent code to native code.
Also add a SimulateKeyPress helper for browser_tests that takes both a UIEvent |code| and a ui::KeyboardCode.
BUG=284754
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222168
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222435
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addressing CR feedback #
Total comments: 30
Patch Set 3 : Addressing CR feedback. #
Total comments: 2
Patch Set 4 : Addressing CR feedback. #
Total comments: 2
Patch Set 5 : Addressing CR feedback. #
Total comments: 8
Patch Set 6 : Addressing CR feedback. #
Total comments: 4
Patch Set 7 : #Patch Set 8 : #Patch Set 9 : Fixing unittests #
Messages
Total messages: 34 (0 generated)
https://codereview.chromium.org/23542008/diff/1/ui/base/keycodes/usb_keycode_... File ui/base/keycodes/usb_keycode_map.h (right): https://codereview.chromium.org/23542008/diff/1/ui/base/keycodes/usb_keycode_... ui/base/keycodes/usb_keycode_map.h:442: inline uint16_t CodeToNativeKeycode(const char* code) { Please move this up next to NativeKeycodeToCode. The functions at the bottom are all USB-related. https://codereview.chromium.org/23542008/diff/1/ui/base/keycodes/usb_keycode_... File ui/base/keycodes/usb_keycode_map_unittest.cc (right): https://codereview.chromium.org/23542008/diff/1/ui/base/keycodes/usb_keycode_... ui/base/keycodes/usb_keycode_map_unittest.cc:52: CodeToNativeKeycode(usb_keycode_map[i].code)); Can you check both directions here? EXPECT_EQ(usb_keycode_map[i].code, NativeKeycodeToCode(usb_keycode_map[i].native_keycode))
LGTM after comments are addressed.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/weitaosu@chromium.org/23542008/7001
Tommi, could you please review the change in browser_test_utils? The chromoting client uses the native key code which is set to 0 by the current SimulateKeyPress method. https://codereview.chromium.org/23542008/diff/1/ui/base/keycodes/usb_keycode_... File ui/base/keycodes/usb_keycode_map.h (right): https://codereview.chromium.org/23542008/diff/1/ui/base/keycodes/usb_keycode_... ui/base/keycodes/usb_keycode_map.h:442: inline uint16_t CodeToNativeKeycode(const char* code) { On 2013/09/04 23:04:03, garykac wrote: > Please move this up next to NativeKeycodeToCode. The functions at the bottom are > all USB-related. Done. https://codereview.chromium.org/23542008/diff/1/ui/base/keycodes/usb_keycode_... File ui/base/keycodes/usb_keycode_map_unittest.cc (right): https://codereview.chromium.org/23542008/diff/1/ui/base/keycodes/usb_keycode_... ui/base/keycodes/usb_keycode_map_unittest.cc:52: CodeToNativeKeycode(usb_keycode_map[i].code)); On 2013/09/04 23:04:03, garykac wrote: > Can you check both directions here? > > EXPECT_EQ(usb_keycode_map[i].code, > NativeKeycodeToCode(usb_keycode_map[i].native_keycode)) Done.
issue= line needs replacing with a BUG= line with the relevant bug numbers (e.g. 283609, 134448, 134210, 284754)
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
not LGTM https://codereview.chromium.org/23542008/diff/7001/content/public/test/browse... File content/public/test/browser_test_utils.cc (right): https://codereview.chromium.org/23542008/diff/7001/content/public/test/browse... content/public/test/browser_test_utils.cc:177: bool command) { Why do you need this extra SimulateKeyPress override rather than putting this logic in the |code| version? If it's required then it shouldn't be an override, as per style guide, instead e.g. SimulateKeyPressWithNativeKeyCode. https://codereview.chromium.org/23542008/diff/7001/content/public/test/browse... content/public/test/browser_test_utils.cc:192: WebKit::WebInputEvent::RawKeyDown, This should be Char. https://codereview.chromium.org/23542008/diff/7001/content/public/test/browse... content/public/test/browser_test_utils.cc:204: WebKit::WebInputEvent::RawKeyDown, This should be KeyUp. https://codereview.chromium.org/23542008/diff/7001/content/public/test/browse... content/public/test/browser_test_utils.cc:331: SimulateKeyPress(web_contents, key, 0, control, shift, alt, command); 0 -> NULL, or better still, InvalidKeyboardEventCode(). https://codereview.chromium.org/23542008/diff/7001/content/public/test/browse... File content/public/test/browser_test_utils.h (right): https://codereview.chromium.org/23542008/diff/7001/content/public/test/browse... content/public/test/browser_test_utils.h:82: // The native code of the key event will be set to 0. This should be "... will be set to InvalidNativeKeycode()." or similar. https://codereview.chromium.org/23542008/diff/7001/content/public/test/browse... content/public/test/browser_test_utils.h:91: // The native code of the key event will be set based on |code|. This should refer to the DOM4 UI Events spec, e.g. "|code| specifies the DOM4 UI Events value for the key." nit: Rename the |key| parameter to |keyCode|, since that's what it actually sets in DOM language, here and above. https://codereview.chromium.org/23542008/diff/7001/content/public/test/browse... content/public/test/browser_test_utils.h:93: // |code| and generates the other one based on US keyboard layout? nit: This isn't really a TODO, so I don't think the comment adds anything. https://codereview.chromium.org/23542008/diff/7001/content/public/test/browse... content/public/test/browser_test_utils.h:94: void SimulateKeyPress(WebContents* web_contents, As per style guide, this shouldn't be an override, but e.g. SimulateKeyPressWithCode(). https://codereview.chromium.org/23542008/diff/7001/remoting/test/remote_deskt... File remoting/test/remote_desktop_browsertest.h (right): https://codereview.chromium.org/23542008/diff/7001/remoting/test/remote_deskt... remoting/test/remote_desktop_browsertest.h:92: bool command); Why do we still need these overrides? https://codereview.chromium.org/23542008/diff/7001/ui/base/keycodes/usb_keyco... File ui/base/keycodes/usb_keycode_map.h (right): https://codereview.chromium.org/23542008/diff/7001/ui/base/keycodes/usb_keyco... ui/base/keycodes/usb_keycode_map.h:403: if (code) { You don't need this if - the loop will exit on the first iteration if |code| is NULL. https://codereview.chromium.org/23542008/diff/7001/ui/base/keycodes/usb_keyco... ui/base/keycodes/usb_keycode_map.h:403: if (code) { You should explicitly check for "Unidentified" here, and return InvalidNativeKeycode() in that case, otherwise you scan the entire array only to find no match. https://codereview.chromium.org/23542008/diff/7001/ui/base/keycodes/usb_keyco... ui/base/keycodes/usb_keycode_map.h:404: // TODO: sort |usb_keycode_map| by |code| so we can use binary search here. This comment assumes that linear search performance won't be sufficient; if you want to research that, please file a bug rather than adding a comment here. :) https://codereview.chromium.org/23542008/diff/7001/ui/base/keycodes/usb_keyco... ui/base/keycodes/usb_keycode_map.h:406: if (usb_keycode_map[i].code && strcmp(usb_keycode_map[i].code, code) == 0) Split this into two if()s for clarity. https://codereview.chromium.org/23542008/diff/7001/ui/base/keycodes/usb_keyco... File ui/base/keycodes/usb_keycode_map_unittest.cc (right): https://codereview.chromium.org/23542008/diff/7001/ui/base/keycodes/usb_keyco... ui/base/keycodes/usb_keycode_map_unittest.cc:35: EXPECT_EQ(InvalidNativeKeycode(), usb_keycode_map[0].native_keycode); Verify the InvalidKeyboardEventCode() is "Unidentified" here. :) Also verify that "Unidentified" maps to InvalidNativeKeycode()
PTAL. https://codereview.chromium.org/23542008/diff/7001/content/public/test/browse... File content/public/test/browser_test_utils.cc (right): https://codereview.chromium.org/23542008/diff/7001/content/public/test/browse... content/public/test/browser_test_utils.cc:177: bool command) { On 2013/09/05 00:16:33, Wez wrote: > Why do you need this extra SimulateKeyPress override rather than putting this > logic in the |code| version? If it's required then it shouldn't be an override, > as per style guide, instead e.g. SimulateKeyPressWithNativeKeyCode. Done. https://codereview.chromium.org/23542008/diff/7001/content/public/test/browse... content/public/test/browser_test_utils.cc:192: WebKit::WebInputEvent::RawKeyDown, On 2013/09/05 00:16:33, Wez wrote: > This should be Char. Done. https://codereview.chromium.org/23542008/diff/7001/content/public/test/browse... content/public/test/browser_test_utils.cc:204: WebKit::WebInputEvent::RawKeyDown, On 2013/09/05 00:16:33, Wez wrote: > This should be KeyUp. Done. https://codereview.chromium.org/23542008/diff/7001/content/public/test/browse... content/public/test/browser_test_utils.cc:331: SimulateKeyPress(web_contents, key, 0, control, shift, alt, command); On 2013/09/05 00:16:33, Wez wrote: > 0 -> NULL, or better still, InvalidKeyboardEventCode(). Done. https://codereview.chromium.org/23542008/diff/7001/content/public/test/browse... File content/public/test/browser_test_utils.h (right): https://codereview.chromium.org/23542008/diff/7001/content/public/test/browse... content/public/test/browser_test_utils.h:82: // The native code of the key event will be set to 0. On 2013/09/05 00:16:33, Wez wrote: > This should be "... will be set to InvalidNativeKeycode()." or similar. Done. https://codereview.chromium.org/23542008/diff/7001/content/public/test/browse... content/public/test/browser_test_utils.h:91: // The native code of the key event will be set based on |code|. On 2013/09/05 00:16:33, Wez wrote: > This should refer to the DOM4 UI Events spec, e.g. "|code| specifies the DOM4 UI > Events value for the key." > > nit: Rename the |key| parameter to |keyCode|, since that's what it actually sets > in DOM language, here and above. Done. https://codereview.chromium.org/23542008/diff/7001/content/public/test/browse... content/public/test/browser_test_utils.h:93: // |code| and generates the other one based on US keyboard layout? On 2013/09/05 00:16:33, Wez wrote: > nit: This isn't really a TODO, so I don't think the comment adds anything. Done. https://codereview.chromium.org/23542008/diff/7001/content/public/test/browse... content/public/test/browser_test_utils.h:94: void SimulateKeyPress(WebContents* web_contents, On 2013/09/05 00:16:33, Wez wrote: > As per style guide, this shouldn't be an override, but e.g. > SimulateKeyPressWithCode(). Done. https://codereview.chromium.org/23542008/diff/7001/remoting/test/remote_deskt... File remoting/test/remote_desktop_browsertest.h (right): https://codereview.chromium.org/23542008/diff/7001/remoting/test/remote_deskt... remoting/test/remote_desktop_browsertest.h:92: bool command); On 2013/09/05 00:16:33, Wez wrote: > Why do we still need these overrides? These helpers provide the active WebContents* so callers don't have to specify one. https://codereview.chromium.org/23542008/diff/7001/ui/base/keycodes/usb_keyco... File ui/base/keycodes/usb_keycode_map.h (right): https://codereview.chromium.org/23542008/diff/7001/ui/base/keycodes/usb_keyco... ui/base/keycodes/usb_keycode_map.h:403: if (code) { On 2013/09/05 00:16:33, Wez wrote: > You don't need this if - the loop will exit on the first iteration if |code| is > NULL. I do need this "if". Otherwise strcmp would crash. https://codereview.chromium.org/23542008/diff/7001/ui/base/keycodes/usb_keyco... ui/base/keycodes/usb_keycode_map.h:404: // TODO: sort |usb_keycode_map| by |code| so we can use binary search here. On 2013/09/05 00:16:33, Wez wrote: > This comment assumes that linear search performance won't be sufficient; if you > want to research that, please file a bug rather than adding a comment here. :) Done. https://codereview.chromium.org/23542008/diff/7001/ui/base/keycodes/usb_keyco... ui/base/keycodes/usb_keycode_map.h:406: if (usb_keycode_map[i].code && strcmp(usb_keycode_map[i].code, code) == 0) On 2013/09/05 00:16:33, Wez wrote: > Split this into two if()s for clarity. I feel the purpose of two conditions connected by && is pretty clear; if the current code is not NULL and it equals the code passed in. The first condition is necessary because strcmp requires non-NULL arguments.
drive-by nit https://codereview.chromium.org/23542008/diff/21001/content/public/test/brows... File content/public/test/browser_test_utils.cc (right): https://codereview.chromium.org/23542008/diff/21001/content/public/test/brows... content/public/test/browser_test_utils.cc:287: SimulateKeyPressWithCode(web_contents, nit: there is no need to put each argument to a separate line like this. IMO it makes this code less readable, and in this case it's also inconsistent with the rest of the code in this file.
https://codereview.chromium.org/23542008/diff/21001/content/public/test/brows... File content/public/test/browser_test_utils.cc (right): https://codereview.chromium.org/23542008/diff/21001/content/public/test/brows... content/public/test/browser_test_utils.cc:287: SimulateKeyPressWithCode(web_contents, On 2013/09/05 16:18:16, Sergey Ulanov wrote: > nit: there is no need to put each argument to a separate line like this. IMO it > makes this code less readable, and in this case it's also inconsistent with the > rest of the code in this file. Done.
LGTM w/ nits https://codereview.chromium.org/23542008/diff/7001/remoting/test/remote_deskt... File remoting/test/remote_desktop_browsertest.h (right): https://codereview.chromium.org/23542008/diff/7001/remoting/test/remote_deskt... remoting/test/remote_desktop_browsertest.h:92: bool command); On 2013/09/05 07:49:17, weitaosu wrote: > On 2013/09/05 00:16:33, Wez wrote: > > Why do we still need these overrides? > > These helpers provide the active WebContents* so callers don't have to specify > one. Ah, OK. The overload should be named SimulateKeyPressWithCode rather than overloading, similarly to the underlying API. https://codereview.chromium.org/23542008/diff/7001/ui/base/keycodes/usb_keyco... File ui/base/keycodes/usb_keycode_map.h (right): https://codereview.chromium.org/23542008/diff/7001/ui/base/keycodes/usb_keyco... ui/base/keycodes/usb_keycode_map.h:403: if (code) { On 2013/09/05 07:49:17, weitaosu wrote: > On 2013/09/05 00:16:33, Wez wrote: > > You don't need this if - the loop will exit on the first iteration if |code| > is > > NULL. > > I do need this "if". Otherwise strcmp would crash. Yes, sorry; missed that you're not comparing the ptr value in the loop. ;) https://codereview.chromium.org/23542008/diff/7001/ui/base/keycodes/usb_keyco... ui/base/keycodes/usb_keycode_map.h:406: if (usb_keycode_map[i].code && strcmp(usb_keycode_map[i].code, code) == 0) On 2013/09/05 07:49:17, weitaosu wrote: > On 2013/09/05 00:16:33, Wez wrote: > > Split this into two if()s for clarity. > > I feel the purpose of two conditions connected by && is pretty clear; if the > current code is not NULL and it equals the code passed in. The first condition > is necessary because strcmp requires non-NULL arguments. Right - I'm suggesting: if (!code) continue; if (strcmp) return stuff; https://codereview.chromium.org/23542008/diff/26001/ui/base/keycodes/usb_keyc... File ui/base/keycodes/usb_keycode_map.h (right): https://codereview.chromium.org/23542008/diff/26001/ui/base/keycodes/usb_keyc... ui/base/keycodes/usb_keycode_map.h:404: strcmp(code, InvalidKeyboardEventCode()) == 0) nit: It's best to treat an if as multi-line, and therefore requiring of braces, even if just the conditional is multi-line.
Thanks Gary and Wez for your CRs. Tommi, could you please review the changes in browser_test_utils.*? Thanks, -Weitao https://codereview.chromium.org/23542008/diff/7001/remoting/test/remote_deskt... File remoting/test/remote_desktop_browsertest.h (right): https://codereview.chromium.org/23542008/diff/7001/remoting/test/remote_deskt... remoting/test/remote_desktop_browsertest.h:92: bool command); On 2013/09/05 19:07:02, Wez wrote: > On 2013/09/05 07:49:17, weitaosu wrote: > > On 2013/09/05 00:16:33, Wez wrote: > > > Why do we still need these overrides? > > > > These helpers provide the active WebContents* so callers don't have to specify > > one. > > Ah, OK. The overload should be named SimulateKeyPressWithCode rather than > overloading, similarly to the underlying API. Done. https://codereview.chromium.org/23542008/diff/26001/ui/base/keycodes/usb_keyc... File ui/base/keycodes/usb_keycode_map.h (right): https://codereview.chromium.org/23542008/diff/26001/ui/base/keycodes/usb_keyc... ui/base/keycodes/usb_keycode_map.h:404: strcmp(code, InvalidKeyboardEventCode()) == 0) On 2013/09/05 19:07:02, Wez wrote: > nit: It's best to treat an if as multi-line, and therefore requiring of braces, > even if just the conditional is multi-line. Done.
Actually I'm not owner for browser_test_utils.cc, so I can't stamp this. Check the owners file for details (I'm guessing we need to update it with the fancy new filters). I do have some style pointers though :) https://codereview.chromium.org/23542008/diff/33001/content/public/test/brows... File content/public/test/browser_test_utils.cc (right): https://codereview.chromium.org/23542008/diff/33001/content/public/test/brows... content/public/test/browser_test_utils.cc:136: ui::KeyboardCode keyCode, key_code https://codereview.chromium.org/23542008/diff/33001/content/public/test/brows... content/public/test/browser_test_utils.cc:137: int nativeKeyCode, native_key_code https://codereview.chromium.org/23542008/diff/33001/content/public/test/brows... File content/public/test/browser_test_utils.h (right): https://codereview.chromium.org/23542008/diff/33001/content/public/test/brows... content/public/test/browser_test_utils.h:84: ui::KeyboardCode keyCode, key_code https://codereview.chromium.org/23542008/diff/33001/content/public/test/brows... content/public/test/browser_test_utils.h:95: ui::KeyboardCode keyCode, key_code. also fix indent please.
Sky, could you please review the changes in browser_tests? Thanks Tommi! For some reason "git cl upload" suggested you as the reviewer. https://codereview.chromium.org/23542008/diff/33001/content/public/test/brows... File content/public/test/browser_test_utils.cc (right): https://codereview.chromium.org/23542008/diff/33001/content/public/test/brows... content/public/test/browser_test_utils.cc:136: ui::KeyboardCode keyCode, On 2013/09/06 06:36:38, tommi wrote: > key_code Done. https://codereview.chromium.org/23542008/diff/33001/content/public/test/brows... content/public/test/browser_test_utils.cc:137: int nativeKeyCode, On 2013/09/06 06:36:38, tommi wrote: > native_key_code Done. https://codereview.chromium.org/23542008/diff/33001/content/public/test/brows... File content/public/test/browser_test_utils.h (right): https://codereview.chromium.org/23542008/diff/33001/content/public/test/brows... content/public/test/browser_test_utils.h:84: ui::KeyboardCode keyCode, On 2013/09/06 06:36:38, tommi wrote: > key_code Done. https://codereview.chromium.org/23542008/diff/33001/content/public/test/brows... content/public/test/browser_test_utils.h:95: ui::KeyboardCode keyCode, On 2013/09/06 06:36:38, tommi wrote: > key_code. > also fix indent please. Done.
I'm not an owner for any of the browser tests.
On 2013/09/06 20:55:38, sky wrote: > I'm not an owner for any of the browser tests. Weitao is referring to content/public/test/*, for which you are an owner.
https://codereview.chromium.org/23542008/diff/12001/content/public/test/brows... File content/public/test/browser_test_utils.h (right): https://codereview.chromium.org/23542008/diff/12001/content/public/test/brows... content/public/test/browser_test_utils.h:94: void SimulateKeyPressWithCode(WebContents* web_contents, Style guide says in general we should not have method with same name and varying number of arguments. Additionally among the chrome team there is a distaste for this style. So, sorry, but you'll need to update all callers. I tend to think making this take a struct with some functions to create said struct is the right long term thing.
https://codereview.chromium.org/23542008/diff/12001/content/public/test/brows... File content/public/test/browser_test_utils.h (right): https://codereview.chromium.org/23542008/diff/12001/content/public/test/brows... content/public/test/browser_test_utils.h:94: void SimulateKeyPressWithCode(WebContents* web_contents, The two methods have different names: SimulateKeyPress vs. SimulateKeyPressWithCode. Or are you referring to some other overloads? On 2013/09/06 22:00:28, sky wrote: > Style guide says in general we should not have method with same name and varying > number of arguments. Additionally among the chrome team there is a distaste for > this style. So, sorry, but you'll need to update all callers. I tend to think > making this take a struct with some functions to create said struct is the right > long term thing.
https://codereview.chromium.org/23542008/diff/12001/content/public/test/brows... File content/public/test/browser_test_utils.h (right): https://codereview.chromium.org/23542008/diff/12001/content/public/test/brows... content/public/test/browser_test_utils.h:94: void SimulateKeyPressWithCode(WebContents* web_contents, Good point. Both take codes though. Is there some better way to disambiguate the two? On 2013/09/06 23:03:51, weitaosu wrote: > The two methods have different names: SimulateKeyPress vs. > SimulateKeyPressWithCode. Or are you referring to some other overloads? > > On 2013/09/06 22:00:28, sky wrote: > > Style guide says in general we should not have method with same name and > varying > > number of arguments. Additionally among the chrome team there is a distaste > for > > this style. So, sorry, but you'll need to update all callers. I tend to think > > making this take a struct with some functions to create said struct is the > right > > long term thing. >
Scott, I added some comments around the two APIs. I don't think I can explain the difference between the different "codes" in the comment but at least I can instruct other devs when to use which API. Let me know if looks good to you. https://codereview.chromium.org/23542008/diff/12001/content/public/test/brows... File content/public/test/browser_test_utils.h (right): https://codereview.chromium.org/23542008/diff/12001/content/public/test/brows... content/public/test/browser_test_utils.h:94: void SimulateKeyPressWithCode(WebContents* web_contents, SimulateKeyPress takes a |key_code| while SimulateKeyPress takes a |code| as well as a |key_code|. I know this is confusing but we simply don't have better names for them. The first parameter was initially named |key| but that is equally confusing, if not more. Wez recommended that we change it to |key_code|. All these different "codes" (pun intended) associated with a key press event are inherently confusing and that is why Gary is drafting a DOM spec for them. The link to the spec is actually included in the comment above. But let me add some more comments on the the usage of these two APIs to make it a little bit easier to choose in most cases. On 2013/09/08 02:29:03, sky wrote: > Good point. Both take codes though. Is there some better way to disambiguate the > two? > > On 2013/09/06 23:03:51, weitaosu wrote: > > The two methods have different names: SimulateKeyPress vs. > > SimulateKeyPressWithCode. Or are you referring to some other overloads? > > > > On 2013/09/06 22:00:28, sky wrote: > > > Style guide says in general we should not have method with same name and > > varying > > > number of arguments. Additionally among the chrome team there is a distaste > > for > > > this style. So, sorry, but you'll need to update all callers. I tend to > think > > > making this take a struct with some functions to create said struct is the > > right > > > long term thing. > > >
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/weitaosu@chromium.org/23542008/54001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/weitaosu@chromium.org/23542008/31001
Message was sent while issue was closed.
Change committed as 222168
Message was sent while issue was closed.
On 2013/09/10 01:18:00, I haz the power (commit-bot) wrote: > Change committed as 222168 This change appears to have broken http://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%28dbg%29%281%... .
Message was sent while issue was closed.
UsbKeycodeMap.Basic: base\keycodes\usb_keycode_map_unittest.cc(36): error: Value of: "Unidentified" Actual: 00B40D98 Expected: InvalidKeyboardEventCode() Which is: 00AA90D4
Message was sent while issue was closed.
On 2013/09/10 02:59:27, Rouslan Solomakhin wrote: > UsbKeycodeMap.Basic: > base\keycodes\usb_keycode_map_unittest.cc(36): error: Value of: "Unidentified" > Actual: 00B40D98 > Expected: InvalidKeyboardEventCode() > Which is: 00AA90D4 Failed twice in a row. Please revert.
Message was sent while issue was closed.
drive-by nits: we use BUG= instead of issue= in the CL description.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/weitaosu@chromium.org/23542008/86001
Retried try job too often on linux_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/weitaosu@chromium.org/23542008/86001
Message was sent while issue was closed.
Change committed as 222435 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
