Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2197113002:
Force U.S. English keyboard layout for TextfieldTest.KeysWithModifiersTest (Closed)
|
Created:
4 years, 4 months ago by Tomasz Moniuszko Modified:
4 years, 3 months ago CC:
chromium-reviews, tfarina, James Su, shuchen+watch_chromium.org, yusukes+watch_chromium.org, nona+watch_chromium.org, chongz Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionForce U.S. English keyboard layout for TextfieldTest.KeysWithModifiersTest
BUG=633136
Committed: https://crrev.com/19cb1e03c46a4df0c0fd54171369152bcb19c237
Cr-Commit-Position: refs/heads/master@{#419454}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Share keyboard layout selection code between tests #
Total comments: 10
Patch Set 3 : Fix review issues #
Total comments: 12
Patch Set 4 : Make ScopedKeyboardLayout compile for other platforms #
Total comments: 1
Patch Set 5 : More review fixes, delete obsolete files #
Total comments: 2
Patch Set 6 : Fix TODO comment #
Total comments: 1
Patch Set 7 : Fix minor review issues #Patch Set 8 : Fix build issues on Mac, iOS and Linux #
Total comments: 19
Patch Set 9 : Fix ui/events review issues #
Total comments: 2
Patch Set 10 : Change MacOSX/iOS preprocessor check as suggested in the review #Patch Set 11 : More Mac and iOS build fixes #
Messages
Total messages: 54 (16 generated)
tmoniuszko@opera.com changed reviewers: + pkasting@chromium.org
https://codereview.chromium.org/2197113002/diff/1/ui/views/controls/textfield... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2197113002/diff/1/ui/views/controls/textfield... ui/views/controls/textfield/textfield_unittest.cc:775: ::LoadKeyboardLayout(L"00000409", KLF_ACTIVATE); Don't you need to call ActivateKeyboardLayout() as well? More generally, can chrome/test/chromedriver/test_util.cc and here share code somehow, since they both want to do the same thing? After that, you probably want to ASSERT that the return value is the locale you wanted, as current callers of SwitchKeyboardLayout() tend to do. Nit: :: is unnecessary
https://codereview.chromium.org/2197113002/diff/1/ui/views/controls/textfield... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2197113002/diff/1/ui/views/controls/textfield... ui/views/controls/textfield/textfield_unittest.cc:775: ::LoadKeyboardLayout(L"00000409", KLF_ACTIVATE); On 2016/08/03 00:13:26, Peter Kasting wrote: > Don't you need to call ActivateKeyboardLayout() as well? More generally, can > chrome/test/chromedriver/test_util.cc and here share code somehow, since they > both want to do the same thing? Done. Code is now shared between chromedriver_test, events_unittests and views_unittests. I also fixed KeyConverter.AllEnglishKeyboardSymbols test which was also failing with non-English keyboard layout.
https://codereview.chromium.org/2197113002/diff/20001/chrome/test/chromedrive... File chrome/test/chromedriver/keycode_text_conversion_unittest.cc (right): https://codereview.chromium.org/2197113002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/keycode_text_conversion_unittest.cc:155: ui::ScopedKeyboardLayout russian_layout(ui::KEYBOARD_LAYOUT_RUSSIAN); Nit: While it will make no functional difference, if you're not actually trying to test what happens when you scoped one layout object inside another, I'd give these separate blocks: { ui::ScopedKeyboardLayout ...; ... } { ui::ScopedKeyboardLayout ...; ... } ...just to make it very clear where your concern for the first layout ends (e.g. it's not going to become relevant again later on in the function after the second layout object goes away). https://codereview.chromium.org/2197113002/diff/20001/ui/events/test/keyboard... File ui/events/test/keyboard_layout.cc (right): https://codereview.chromium.org/2197113002/diff/20001/ui/events/test/keyboard... ui/events/test/keyboard_layout.cc:14: CHECK(ActivateLayout(GetPlatformKeyboardLayout(layout))); Nit: We normally use CHECK only for security-sensitive issues and temporary debugging of DCHECKs that are failing. If you have no reason to expect that ActivateLayout() should fail (which is what this code says), I would just make it return void and remove the CHECK from here. (You can add DCHECKs to the implementation of that function if you want.) https://codereview.chromium.org/2197113002/diff/20001/ui/events/test/keyboard... File ui/events/test/keyboard_layout_mac.cc (right): https://codereview.chromium.org/2197113002/diff/20001/ui/events/test/keyboard... ui/events/test/keyboard_layout_mac.cc:19: return PlatformKeyboardLayout(); Nit: Avoid handling NOTREACHED(). Just do: DCHECK_EQ(KEYBOARD_LAYOUT_GERMAN, layout); const char* input_source_id = "com.apple.keylayout.German"; https://codereview.chromium.org/2197113002/diff/20001/ui/events/test/keyboard... File ui/events/test/keyboard_layout_win.cc (right): https://codereview.chromium.org/2197113002/diff/20001/ui/events/test/keyboard... ui/events/test/keyboard_layout_win.cc:31: // See https://crbug.com/612736#c6 I tried to read the bug, but I don't understand why this causes DrMemory to fail. (This is a a danger of linking to bugs; it's generally worse for a reader than just summarizing the issue inline.) It seems like comment 11 basically says "who knows". I think it would be better to just do the right thing here, and suppress the DrMemory error, than work around it in this fashion. That way we won't run into trouble if other tests later need to actually load these keyboard layouts. https://codereview.chromium.org/2197113002/diff/20001/ui/views/controls/textf... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2197113002/diff/20001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:775: // change the text inserted into a texfield and cause this test to fail. Is this really inherently only an OS_WIN problem, or should we be running this code on all OSes in principle? Why?
https://codereview.chromium.org/2197113002/diff/20001/chrome/test/chromedrive... File chrome/test/chromedriver/keycode_text_conversion_unittest.cc (right): https://codereview.chromium.org/2197113002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/keycode_text_conversion_unittest.cc:155: ui::ScopedKeyboardLayout russian_layout(ui::KEYBOARD_LAYOUT_RUSSIAN); On 2016/08/18 17:48:19, Peter Kasting wrote: > Nit: While it will make no functional difference, if you're not actually trying > to test what happens when you scoped one layout object inside another, I'd give > these separate blocks: > > { > ui::ScopedKeyboardLayout ...; > ... > } > { > ui::ScopedKeyboardLayout ...; > ... > } > > ...just to make it very clear where your concern for the first layout ends (e.g. > it's not going to become relevant again later on in the function after the > second layout object goes away). Done. https://codereview.chromium.org/2197113002/diff/20001/ui/events/test/keyboard... File ui/events/test/keyboard_layout.cc (right): https://codereview.chromium.org/2197113002/diff/20001/ui/events/test/keyboard... ui/events/test/keyboard_layout.cc:14: CHECK(ActivateLayout(GetPlatformKeyboardLayout(layout))); On 2016/08/18 17:48:19, Peter Kasting wrote: > Nit: We normally use CHECK only for security-sensitive issues and temporary > debugging of DCHECKs that are failing. > > If you have no reason to expect that ActivateLayout() should fail (which is what > this code says), I would just make it return void and remove the CHECK from > here. (You can add DCHECKs to the implementation of that function if you want.) Done. https://codereview.chromium.org/2197113002/diff/20001/ui/events/test/keyboard... File ui/events/test/keyboard_layout_mac.cc (right): https://codereview.chromium.org/2197113002/diff/20001/ui/events/test/keyboard... ui/events/test/keyboard_layout_mac.cc:19: return PlatformKeyboardLayout(); On 2016/08/18 17:48:19, Peter Kasting wrote: > Nit: Avoid handling NOTREACHED(). Just do: > > DCHECK_EQ(KEYBOARD_LAYOUT_GERMAN, layout); > const char* input_source_id = "com.apple.keylayout.German"; I added support for English US keyboard so I think it makes sense to leave the switch statement and NOTREACHED() now. https://codereview.chromium.org/2197113002/diff/20001/ui/events/test/keyboard... File ui/events/test/keyboard_layout_win.cc (right): https://codereview.chromium.org/2197113002/diff/20001/ui/events/test/keyboard... ui/events/test/keyboard_layout_win.cc:31: // See https://crbug.com/612736#c6 On 2016/08/18 17:48:19, Peter Kasting wrote: > I tried to read the bug, but I don't understand why this causes DrMemory to > fail. (This is a a danger of linking to bugs; it's generally worse for a reader > than just summarizing the issue inline.) It seems like comment 11 basically > says "who knows". > > I think it would be better to just do the right thing here, and suppress the > DrMemory error, than work around it in this fashion. That way we won't run into > trouble if other tests later need to actually load these keyboard layouts. Done. https://codereview.chromium.org/2197113002/diff/20001/ui/views/controls/textf... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2197113002/diff/20001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:775: // change the text inserted into a texfield and cause this test to fail. On 2016/08/18 17:48:19, Peter Kasting wrote: > Is this really inherently only an OS_WIN problem, or should we be running this > code on all OSes in principle? Why? The problem also exists on other desktop platforms as they also use modifier keys for entering non-standard characters. The same solution will work also for Mac. I'm on Windows so it's hard to me to implement this solution for other platforms. I guess setxkbmap can be used on Linux as this is what ImeKeyboardX11 does. But still it's hard to me to verify. I added a TODO instead. Is it acceptable?
https://codereview.chromium.org/2197113002/diff/40001/chrome/test/chromedrive... File chrome/test/chromedriver/keycode_text_conversion_unittest.cc (right): https://codereview.chromium.org/2197113002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/keycode_text_conversion_unittest.cc:162: } In principle, is there a reason why these tests would be OS-specific? Like, if we supported the Greek and Russian layouts on Mac, would it make sense to test this functionality there as well? Because if so maybe the right answer is to expand the supported layouts on these platforms and then reduce the #ifdefing in the test files. https://codereview.chromium.org/2197113002/diff/40001/ui/events/test/keyboard... File ui/events/test/keyboard_layout.h (right): https://codereview.chromium.org/2197113002/diff/40001/ui/events/test/keyboard... ui/events/test/keyboard_layout.h:18: #if defined(OS_WIN) || defined(OS_MACOSX) If this header only defines the contents below for Mac and Win, how are test files that reference these without a guard ifdef going to compile on Linux? Even if we disable the tests, that's a runtime thing, not a compile-time one, I believe. https://codereview.chromium.org/2197113002/diff/40001/ui/events/test/keyboard... File ui/events/test/keyboard_layout_mac.cc (right): https://codereview.chromium.org/2197113002/diff/40001/ui/events/test/keyboard... ui/events/test/keyboard_layout_mac.cc:22: return PlatformKeyboardLayout(); Nit: I still probably would have tried to avoid handling NOTREACHED, e.g. by doing DCHECK((layout == KEYBOARD_LAYOUT_ENGLISH_US) || (layout == KEYBOARD_LAYOUT_GERMAN)); const char[] const input_source_id = (layout == KEYBOARD_LAYOUT_ENGLISH_US ) ? "com.apple.keylayout.US" : "com.apple.keylayout.German"; ...which is also noticeably shorter, but it also wouldn't continue to scale up well if you think there will be more layouts added later. So up to you. https://codereview.chromium.org/2197113002/diff/40001/ui/views/controls/textf... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2197113002/diff/40001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:779: // TODO(crbug.com/633136): Switch keyboard layout on other platforms. Does this mean that for now, the test is potentially flaky/failing on other platforms? If so we should probably be disabling the test on those platforms.
https://codereview.chromium.org/2197113002/diff/40001/chrome/test/chromedrive... File chrome/test/chromedriver/keycode_text_conversion_unittest.cc (right): https://codereview.chromium.org/2197113002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/keycode_text_conversion_unittest.cc:162: } On 2016/08/26 18:54:33, Peter Kasting wrote: > In principle, is there a reason why these tests would be OS-specific? Like, if > we supported the Greek and Russian layouts on Mac, would it make sense to test > this functionality there as well? > > Because if so maybe the right answer is to expand the supported layouts on these > platforms and then reduce the #ifdefing in the test files. I tested Greek and Russian keyboard layouts on Mac. Pressing Q key gives ';' character with Greek and pressing B key gives 'и' character with Russian just like on Windows. Similarly pressing Y key on Windows with German keyboard layout gives 'z' character. I did both tests using physical US QWERTY keyboard. Unfortunately Y key test here fails on Windows when I remove #ifdefs. I'm not sure why. I guess Y key on QWERTZ keyboard sends Y virtual key. https://codereview.chromium.org/2197113002/diff/40001/ui/events/test/keyboard... File ui/events/test/keyboard_layout.h (right): https://codereview.chromium.org/2197113002/diff/40001/ui/events/test/keyboard... ui/events/test/keyboard_layout.h:18: #if defined(OS_WIN) || defined(OS_MACOSX) On 2016/08/26 18:54:33, Peter Kasting wrote: > If this header only defines the contents below for Mac and Win, how are test > files that reference these without a guard ifdef going to compile on Linux? > Even if we disable the tests, that's a runtime thing, not a compile-time one, I > believe. Right, it won't work for tests which are disabled with DISABLED_ prefix. I added dummy implementation for other platforms. https://codereview.chromium.org/2197113002/diff/40001/ui/events/test/keyboard... File ui/events/test/keyboard_layout_mac.cc (right): https://codereview.chromium.org/2197113002/diff/40001/ui/events/test/keyboard... ui/events/test/keyboard_layout_mac.cc:22: return PlatformKeyboardLayout(); On 2016/08/26 18:54:33, Peter Kasting wrote: > Nit: I still probably would have tried to avoid handling NOTREACHED, e.g. by > doing > > DCHECK((layout == KEYBOARD_LAYOUT_ENGLISH_US) || > (layout == KEYBOARD_LAYOUT_GERMAN)); > const char[] const input_source_id = (layout == KEYBOARD_LAYOUT_ENGLISH_US ) > ? "com.apple.keylayout.US" > : "com.apple.keylayout.German"; > > ...which is also noticeably shorter, but it also wouldn't continue to scale up > well if you think there will be more layouts added later. So up to you. Actually, it seems that tests which need German keyboard layout are disabled on Mac. Comments say it's because they fail if German keyboard layout is not installed in system. I removed support for German layout then. https://codereview.chromium.org/2197113002/diff/40001/ui/views/controls/textf... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2197113002/diff/40001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:779: // TODO(crbug.com/633136): Switch keyboard layout on other platforms. On 2016/08/26 18:54:33, Peter Kasting wrote: > Does this mean that for now, the test is potentially flaky/failing on other > platforms? If so we should probably be disabling the test on those platforms. Test fails only on machines with keyboard layouts that uses modifier keys for generating special characters. Most test machines seem to use US English keyboard layout by default. Test is fine on these machines. I think it shouldn't be disabled.
https://codereview.chromium.org/2197113002/diff/40001/chrome/test/chromedrive... File chrome/test/chromedriver/keycode_text_conversion_unittest.cc (right): https://codereview.chromium.org/2197113002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/keycode_text_conversion_unittest.cc:162: } On 2016/08/31 15:29:42, Tomasz Moniuszko wrote: > On 2016/08/26 18:54:33, Peter Kasting wrote: > > In principle, is there a reason why these tests would be OS-specific? Like, > if > > we supported the Greek and Russian layouts on Mac, would it make sense to test > > this functionality there as well? > > > > Because if so maybe the right answer is to expand the supported layouts on > these > > platforms and then reduce the #ifdefing in the test files. > > I tested Greek and Russian keyboard layouts on Mac. Pressing Q key gives ';' > character with Greek and pressing B key gives 'и' character with Russian just > like on Windows. That sounds like those tests should be enabled on Mac then? > Similarly pressing Y key on Windows with German keyboard layout > gives 'z' character. I did both tests using physical US QWERTY keyboard. > > Unfortunately Y key test here fails on Windows when I remove #ifdefs. I'm not > sure why. I guess Y key on QWERTZ keyboard sends Y virtual key. I'd like to understand why the test fails if the behavior it's trying to test is the behavior you see in actual usage. This makes me worry that we're missing something here. https://codereview.chromium.org/2197113002/diff/40001/ui/views/controls/textf... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2197113002/diff/40001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:779: // TODO(crbug.com/633136): Switch keyboard layout on other platforms. On 2016/08/31 15:29:42, Tomasz Moniuszko wrote: > On 2016/08/26 18:54:33, Peter Kasting wrote: > > Does this mean that for now, the test is potentially flaky/failing on other > > platforms? If so we should probably be disabling the test on those platforms. > > Test fails only on machines with keyboard layouts that uses modifier keys for > generating special characters. Most test machines seem to use US English > keyboard layout by default. Test is fine on these machines. I think it shouldn't > be disabled. I don't know that that answers my question. I read you as saying that, as long as a test machine is either using Win/Mac or else using US English, it's fine, and most machines seem to fall into one of those groups. But that doesn't mean being outside them is impossible. It seems like we need to do one of: * Implement layout switching for other platforms. * Disable this test on other platforms. * Implement something to read the current keyboard layout on other platforms and don't run the test unless the layout is US English We don't want to check in something that we know can fail just because it looks like it's going to work on most test machines. https://codereview.chromium.org/2197113002/diff/60001/ui/events/test/keyboard... File ui/events/test/keyboard_layout_mac.cc (right): https://codereview.chromium.org/2197113002/diff/60001/ui/events/test/keyboard... ui/events/test/keyboard_layout_mac.cc:12: DCHECK_EQ(layout, KEYBOARD_LAYOUT_ENGLISH_US); Nit: (expected, actual) You might want a comment that says something like "Right now tests only need US English. If other layouts need to be supported in the future this code should be extended."
On 2016/08/31 21:16:15, Peter Kasting wrote: > That sounds like those tests should be enabled on Mac then? I think so. But please note that it doesn't change much as this test is disabled on Mac anyway. > I'd like to understand why the test fails if the behavior it's trying to test is > the behavior you see in actual usage. This makes me worry that we're missing > something here. We get WM_KEYDOWN with wParam=89 on Y key press using German QWERTZ keyboard. 89 is 'Y' character so no additional translation is needed on Windows. (Tested using On-Screen Keyboard on Windows 7) > I don't know that that answers my question. I read you as saying that, as long > as a test machine is either using Win/Mac or else using US English, it's fine, > and most machines seem to fall into one of those groups. Yes, it was exactly what I meant. > But that doesn't mean > being outside them is impossible. It seems like we need to do one of: > > * Implement layout switching for other platforms. > * Disable this test on other platforms. > * Implement something to read the current keyboard layout on other platforms and > don't run the test unless the layout is US English I disabled this test on other platforms. I'm on Windows so it's hard to me to implement keyboard layout change for other platforms. > > We don't want to check in something that we know can fail just because it looks > like it's going to work on most test machines. Well, this test is enabled for all platforms for some time already. I think fixing only Windows + Mac is still an improvement. Leaving this test enabled for other platforms won't introduce any new issues on these platforms. But if you think the test should pass on all test machines (with all keyboard layouts) then yes, test should be disabled for other platforms.
(Note: Please try to continue replying on the codereview comments as it makes it easier to keep discussion threads together later) On 2016/09/01 14:21:16, Tomasz Moniuszko wrote: > > I'd like to understand why the test fails if the behavior it's trying to test > is > > the behavior you see in actual usage. This makes me worry that we're missing > > something here. > > We get WM_KEYDOWN with wParam=89 on Y key press using German QWERTZ keyboard. 89 > is 'Y' character so no additional translation is needed on Windows. (Tested > using On-Screen Keyboard on Windows 7) I'm still confused. Why were you getting a 'z' on the physical keyboard? What about what this key in this layout is testing is different than the keys in the previous two tests? And if this difference is really intentional and well-understood, shouldn't the ifdef here be "!defined(OS_WIN)" (because we're excluding a Windows-specific behavior instead of including a Mac-specific behavior) and have a comment explaining it? https://codereview.chromium.org/2197113002/diff/80001/ui/views/controls/textf... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2197113002/diff/80001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:774: // U.S. English layout cannot be forced). Nit: Hmm, this TODO says what's currently happening rather than what's TODO. Seems like it should be more like TODO(...): Implement keyboard layout changing for other platforms. And are you going to leave bug 633136 as the bug on doing this implementation? Or will that bug effectively be "fixed" because either the problem is solved or the test is disabled, so we'll end up with a different bug just about implementing this on other platforms?
On 2016/09/01 20:16:29, Peter Kasting wrote:
> (Note: Please try to continue replying on the codereview comments as it makes
it
> easier to keep discussion threads together later)
>
> On 2016/09/01 14:21:16, Tomasz Moniuszko wrote:
> > > I'd like to understand why the test fails if the behavior it's trying to
> test
> > is
> > > the behavior you see in actual usage. This makes me worry that we're
> missing
> > > something here.
> >
> > We get WM_KEYDOWN with wParam=89 on Y key press using German QWERTZ
keyboard.
> 89
> > is 'Y' character so no additional translation is needed on Windows. (Tested
> > using On-Screen Keyboard on Windows 7)
>
> I'm still confused. Why were you getting a 'z' on the physical keyboard?
What
> about what this key in this layout is testing is different than the keys in
the
> previous two tests?
I was using QWERTY physical keyboard but On-Screen keyboard was QWERTZ.
Positions of alpha keys are the same for these keyboards with the exception that
positions of Z and Y keys are swapped.
After switching to German keyboard layout in Windows:
- On QWERTY keyboard, pressing Y key generates WM_KEYDOWN with wParam=90 ('z'
character).
- On QWERTZ keyboard, pressing Y key generates WM_KEYDOWN with wParam=89 ('y'
character).
> And if this difference is really intentional and
> well-understood, shouldn't the ifdef here be "!defined(OS_WIN)" (because we're
> excluding a Windows-specific behavior instead of including a Mac-specific
> behavior) and have a comment explaining it?
https://msdn.microsoft.com/en-us/library/windows/desktop/ms646267(v=vs.85).as...
says:
"A keyboard layout not only specifies the physical position of the keys on the
keyboard but also determines the character values generated by pressing those
keys. Each layout identifies the current input language and determines which
character values are generated by which keys and key combinations."
Physical keyboard have some layout that cannot be changed. If keyboard layout
selected in system settings match the physical keyboard's layout then everything
is fine (pressing keyboard key produces expected character). But if we change
the keyboard layout in system settings, it no longer matches physical keyboard's
layout. Pressing keyboard key produces character according to the layout
selected in system settings. Produced character doesn't match the label on
physical key (unless you use overlay for keyboard - see
http://www.howtogeek.com/189270/alternative-keyboard-layouts-explained-dvorak...).
If the test for Mac is valid, then I guess Mac behaves uncommon here.
>
>
https://codereview.chromium.org/2197113002/diff/80001/ui/views/controls/textf...
> File ui/views/controls/textfield/textfield_unittest.cc (right):
>
>
https://codereview.chromium.org/2197113002/diff/80001/ui/views/controls/textf...
> ui/views/controls/textfield/textfield_unittest.cc:774: // U.S. English layout
> cannot be forced).
> Nit: Hmm, this TODO says what's currently happening rather than what's TODO.
> Seems like it should be more like
>
> TODO(...): Implement keyboard layout changing for other platforms.
Done.
>
> And are you going to leave bug 633136 as the bug on doing this implementation?
> Or will that bug effectively be "fixed" because either the problem is solved
or
> the test is disabled, so we'll end up with a different bug just about
> implementing this on other platforms?
I reported this bug for Windows originally. But I think I can change "OS" field
to "All" and leave this bug unresolved after landing this CL to avoid creating a
bug duplicate. Unless you think closing this bug is better.
Calling this out as a separate email since I'm repeating myself: if at all possible, please reply using the comment reply tools here, not by replying to the email summary of them. The latter does not put your replies on the comments on individual patch versions, so they are hard to find later while comparing different versions and seeing what changed in each patch and why, what the discussion was, etc. It forces the other person to reply to the whole message as well.
On 2016/09/02 09:30:19, Tomasz Moniuszko wrote:
> On 2016/09/01 20:16:29, Peter Kasting wrote:
> > (Note: Please try to continue replying on the codereview comments as it
makes
> it
> > easier to keep discussion threads together later)
> >
> > On 2016/09/01 14:21:16, Tomasz Moniuszko wrote:
> > > > I'd like to understand why the test fails if the behavior it's trying to
> > test
> > > is
> > > > the behavior you see in actual usage. This makes me worry that we're
> > missing
> > > > something here.
> > >
> > > We get WM_KEYDOWN with wParam=89 on Y key press using German QWERTZ
> keyboard.
> > 89
> > > is 'Y' character so no additional translation is needed on Windows.
(Tested
> > > using On-Screen Keyboard on Windows 7)
> >
> > I'm still confused. Why were you getting a 'z' on the physical keyboard?
> What
> > about what this key in this layout is testing is different than the keys in
> the
> > previous two tests?
>
> I was using QWERTY physical keyboard but On-Screen keyboard was QWERTZ.
> Positions of alpha keys are the same for these keyboards with the exception
that
> positions of Z and Y keys are swapped.
> After switching to German keyboard layout in Windows:
> - On QWERTY keyboard, pressing Y key generates WM_KEYDOWN with wParam=90 ('z'
> character).
> - On QWERTZ keyboard, pressing Y key generates WM_KEYDOWN with wParam=89 ('y'
> character).
>
> > And if this difference is really intentional and
> > well-understood, shouldn't the ifdef here be "!defined(OS_WIN)" (because
we're
> > excluding a Windows-specific behavior instead of including a Mac-specific
> > behavior) and have a comment explaining it?
>
>
https://msdn.microsoft.com/en-us/library/windows/desktop/ms646267(v=vs.85).as...
> says:
>
> "A keyboard layout not only specifies the physical position of the keys on the
> keyboard but also determines the character values generated by pressing those
> keys. Each layout identifies the current input language and determines which
> character values are generated by which keys and key combinations."
>
> Physical keyboard have some layout that cannot be changed. If keyboard layout
> selected in system settings match the physical keyboard's layout then
everything
> is fine (pressing keyboard key produces expected character). But if we change
> the keyboard layout in system settings, it no longer matches physical
keyboard's
> layout. Pressing keyboard key produces character according to the layout
> selected in system settings. Produced character doesn't match the label on
> physical key (unless you use overlay for keyboard - see
>
http://www.howtogeek.com/189270/alternative-keyboard-layouts-explained-dvorak...).
>
> If the test for Mac is valid, then I guess Mac behaves uncommon here.
Perhaps I'm just dumb, but I'm sorry, I still don't understand what's going on.
There are three tests here that one character value and a different VKEY value
map to each other.
Two of them work on Windows. One does not. Therefore, something is
fundamentally different about what the third is trying to test, or else Windows
is buggy. I am asking what the difference is.
You are explaining things like what keyboard layouts are in general. But that
doesn't answer the question of what is different about this character in this
keyboard layout compared to the other two characters in the Greek and Russian
cases.
Your description of the German-specific case above, where pressing a 'y' key on
a QWERTY keyboard generates a 'z' character, seems like it would be true of the
Greek and Russian cases as well. So I don't understand how that explains why
two cases work and one doesn't.
> > And are you going to leave bug 633136 as the bug on doing this
implementation?
> > Or will that bug effectively be "fixed" because either the problem is solved
> or
> > the test is disabled, so we'll end up with a different bug just about
> > implementing this on other platforms?
>
> I reported this bug for Windows originally. But I think I can change "OS"
field
> to "All" and leave this bug unresolved after landing this CL to avoid creating
a
> bug duplicate. Unless you think closing this bug is better.
Yes, I think closing the bug as Fixed is correct, and filing a different bug,
before landing this, for adding layout switching for non-Win/Mac, is clearer.
chongz@chromium.org changed reviewers: + chongz@chromium.org
https://codereview.chromium.org/2197113002/diff/40001/chrome/test/chromedrive... File chrome/test/chromedriver/keycode_text_conversion_unittest.cc (right): https://codereview.chromium.org/2197113002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/keycode_text_conversion_unittest.cc:162: } On 2016/08/31 21:16:15, Peter Kasting wrote: > On 2016/08/31 15:29:42, Tomasz Moniuszko wrote: > > On 2016/08/26 18:54:33, Peter Kasting wrote: > > > In principle, is there a reason why these tests would be OS-specific? Like, > > if > > > we supported the Greek and Russian layouts on Mac, would it make sense to > test > > > this functionality there as well? > > > > > > Because if so maybe the right answer is to expand the supported layouts on > > these > > > platforms and then reduce the #ifdefing in the test files. > > > > I tested Greek and Russian keyboard layouts on Mac. Pressing Q key gives ';' > > character with Greek and pressing B key gives 'и' character with Russian just > > like on Windows. > > That sounds like those tests should be enabled on Mac then? > > > Similarly pressing Y key on Windows with German keyboard layout > > gives 'z' character. I did both tests using physical US QWERTY keyboard. > > > > Unfortunately Y key test here fails on Windows when I remove #ifdefs. I'm not > > sure why. I guess Y key on QWERTZ keyboard sends Y virtual key. > > I'd like to understand why the test fails if the behavior it's trying to test is > the behavior you see in actual usage. This makes me worry that we're missing > something here. The following comments are not verified, please remind me if I'm wrong. Q1. Why is the last test case different from the first 2? I think the reason is German layout is different from Greek and Russian, where: * Greek and Russian layout's character set is very different from English letters, so they are just using QWERTY layout for VKEYs instead of trying to match characters. (I don't have access to Windows right now and cannot verify) * On German layout most English letters are available, so just VKEY_Y and VKEY_Z was swapped. Q2. How to make the last test case work on Windows? I believe |CheckCharToKeyCode('z', ui::VKEY_Z, 0);| would work. Q3. Why the last test case would work on mac OS? * I believe it only works on QWERTY physical keyboard with German system layout. It seems to me that the Mac's version of |CheckCharToKeyCode()| is actually using scan code rather than VKEY to do the mapping. e.g. On QWERTY physical keyboard with German system layout: * The scan code for key labeled as 'Y' is still 'Y' * The VKEY for key labeled as 'Y' becomes 'Z' * The character produced by scan code 'Y' would be 'z'. Also see how we are using [NSEvent keyCode] to get DomCode (which is essentially scan code): https://cs.chromium.org/chromium/src/ui/events/keycodes/keyboard_code_convers...
On 2016/09/03 00:44:17, chongz wrote: > https://codereview.chromium.org/2197113002/diff/40001/chrome/test/chromedrive... > File chrome/test/chromedriver/keycode_text_conversion_unittest.cc (right): > > https://codereview.chromium.org/2197113002/diff/40001/chrome/test/chromedrive... > chrome/test/chromedriver/keycode_text_conversion_unittest.cc:162: } > On 2016/08/31 21:16:15, Peter Kasting wrote: > > On 2016/08/31 15:29:42, Tomasz Moniuszko wrote: > > > On 2016/08/26 18:54:33, Peter Kasting wrote: > > > > In principle, is there a reason why these tests would be OS-specific? > Like, > > > if > > > > we supported the Greek and Russian layouts on Mac, would it make sense to > > test > > > > this functionality there as well? > > > > > > > > Because if so maybe the right answer is to expand the supported layouts on > > > these > > > > platforms and then reduce the #ifdefing in the test files. > > > > > > I tested Greek and Russian keyboard layouts on Mac. Pressing Q key gives ';' > > > character with Greek and pressing B key gives 'и' character with Russian > just > > > like on Windows. > > > > That sounds like those tests should be enabled on Mac then? > > > > > Similarly pressing Y key on Windows with German keyboard layout > > > gives 'z' character. I did both tests using physical US QWERTY keyboard. > > > > > > Unfortunately Y key test here fails on Windows when I remove #ifdefs. I'm > not > > > sure why. I guess Y key on QWERTZ keyboard sends Y virtual key. > > > > I'd like to understand why the test fails if the behavior it's trying to test > is > > the behavior you see in actual usage. This makes me worry that we're missing > > something here. > > The following comments are not verified, please remind me if I'm wrong. > > Q1. Why is the last test case different from the first 2? > I think the reason is German layout is different from Greek and Russian, where: > * Greek and Russian layout's character set is very different from English > letters, so they are just using QWERTY layout for VKEYs instead of trying to > match characters. (I don't have access to Windows right now and cannot verify) > * On German layout most English letters are available, so just VKEY_Y and > VKEY_Z was swapped. Thanks, but when I'm already confused about precisely how the low-level details on Windows work, I'm concerned untested speculation is just adding confusion rather than reducing it. > Q2. How to make the last test case work on Windows? > I believe |CheckCharToKeyCode('z', ui::VKEY_Z, 0);| would work. That doesn't check anything, since that also works on a US keyboard layout.
On 2016/09/02 19:36:53, Peter Kasting wrote: > Calling this out as a separate email since I'm repeating myself: if at all > possible, please reply using the comment reply tools here, not by replying to > the email summary of them. The latter does not put your replies on the comments > on individual patch versions, so they are hard to find later while comparing > different versions and seeing what changed in each patch and why, what the > discussion was, etc. It forces the other person to reply to the whole message > as well. I didn't know it works this way. I'm going to use reply tools in the future. Sorry for the mess.
https://codereview.chromium.org/2197113002/diff/40001/chrome/test/chromedrive... File chrome/test/chromedriver/keycode_text_conversion_unittest.cc (right): https://codereview.chromium.org/2197113002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/keycode_text_conversion_unittest.cc:162: } On 2016/09/03 00:44:17, chongz wrote: > On 2016/08/31 21:16:15, Peter Kasting wrote: > > On 2016/08/31 15:29:42, Tomasz Moniuszko wrote: > > > On 2016/08/26 18:54:33, Peter Kasting wrote: > > > > In principle, is there a reason why these tests would be OS-specific? > Like, > > > if > > > > we supported the Greek and Russian layouts on Mac, would it make sense to > > test > > > > this functionality there as well? > > > > > > > > Because if so maybe the right answer is to expand the supported layouts on > > > these > > > > platforms and then reduce the #ifdefing in the test files. > > > > > > I tested Greek and Russian keyboard layouts on Mac. Pressing Q key gives ';' > > > character with Greek and pressing B key gives 'и' character with Russian > just > > > like on Windows. > > > > That sounds like those tests should be enabled on Mac then? > > > > > Similarly pressing Y key on Windows with German keyboard layout > > > gives 'z' character. I did both tests using physical US QWERTY keyboard. > > > > > > Unfortunately Y key test here fails on Windows when I remove #ifdefs. I'm > not > > > sure why. I guess Y key on QWERTZ keyboard sends Y virtual key. > > > > I'd like to understand why the test fails if the behavior it's trying to test > is > > the behavior you see in actual usage. This makes me worry that we're missing > > something here. > > The following comments are not verified, please remind me if I'm wrong. > > Q1. Why is the last test case different from the first 2? > I think the reason is German layout is different from Greek and Russian, where: > * Greek and Russian layout's character set is very different from English > letters, so they are just using QWERTY layout for VKEYs instead of trying to > match characters. (I don't have access to Windows right now and cannot verify) > * On German layout most English letters are available, so just VKEY_Y and > VKEY_Z was swapped. I think this is the right answer. Windows system uses a predefined set of virtual-key codes: https://msdn.microsoft.com/en-us/library/windows/desktop/dd375731(v=vs.85).aspx Only codes for characters from Latin script are available. There are no codes for Greek or Cyrillic characters. I tested Greek and Russian keyboards: virtual-key codes match QWERTY layout. I also tested US Dvorak keyboard: virtual-key codes match key labels here (just like in case of QWERTZ keyboard).
LGTM https://codereview.chromium.org/2197113002/diff/100001/chrome/test/chromedriv... File chrome/test/chromedriver/keycode_text_conversion_unittest.cc (right): https://codereview.chromium.org/2197113002/diff/100001/chrome/test/chromedriv... chrome/test/chromedriver/keycode_text_conversion_unittest.cc:162: #if defined(OS_MACOSX) OK, since this test is disabled for Mac anyway, and unlikely to be enabled in the future, let's just remove this block for now. If necessary, we could add a comment in this test like: For Greek and Russian keyboard layouts, which are very different from QWERTY, Windows just uses virtual key codes that match the QWERTY layout, and translates them to other characters. If we wanted to test something like German, whose layout is very similar to QWERTY, we'd need to be careful, as in this case Windows maps the keyboard scan codes to the appropriate (different) VKEYs instead of mapping the VKEYs to different characters.
https://codereview.chromium.org/2197113002/diff/80001/ui/views/controls/textf... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2197113002/diff/80001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:774: // U.S. English layout cannot be forced). On 2016/09/01 20:16:29, Peter Kasting wrote: > Nit: Hmm, this TODO says what's currently happening rather than what's TODO. > Seems like it should be more like > > TODO(...): Implement keyboard layout changing for other platforms. > > And are you going to leave bug 633136 as the bug on doing this implementation? > Or will that bug effectively be "fixed" because either the problem is solved or > the test is disabled, so we'll end up with a different bug just about > implementing this on other platforms? I reported https://bugs.chromium.org/p/chromium/issues/detail?id=633136 for other platforms.
The CQ bit was checked by tmoniuszko@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2197113002/#ps120001 (title: "Fix minor review issues")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Description was changed from ========== Force U.S. English keyboard layout for TextfieldTest.KeysWithModifiersTest BUG=633136 ========== to ========== Force U.S. English keyboard layout for TextfieldTest.KeysWithModifiersTest BUG=633136 ==========
tmoniuszko@opera.com changed reviewers: + frankf@chromium.org, sadrul@chromium.org
sadrul@, frankf@: It seems //chrome/test/chromedriver/* and //ui/events/* still need a review from file owners. Could you please take a look?
sadrul@chromium.org changed reviewers: + wez@chromium.org
+wez@ as key event owner https://codereview.chromium.org/2197113002/diff/140001/ui/views/controls/text... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2197113002/diff/140001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:771: TEST_F(TextfieldTest, MAYBE_KeysWithModifiersTest) { This is a bit weird. From what I can tell, this test is now running (and passing) on linux and chromeos too. With this change, it looks like you are disabling the test on these platforms? Why do we want to do that?
https://codereview.chromium.org/2197113002/diff/140001/ui/views/controls/text... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2197113002/diff/140001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:771: TEST_F(TextfieldTest, MAYBE_KeysWithModifiersTest) { On 2016/09/13 15:16:32, sadrul wrote: > This is a bit weird. From what I can tell, this test is now running (and > passing) on linux and chromeos too. With this change, it looks like you are > disabling the test on these platforms? Why do we want to do that? See conversation that started in https://codereview.chromium.org/2197113002/diff/40001/ui/views/controls/textf... . (It continued later in direct replies for which you'll need to just go searching through the replies to this CL.) The short summary is that this usually passes, but could fail, and I don't want us to enable a known-potentially-flaky test.
https://codereview.chromium.org/2197113002/diff/140001/ui/events/test/keyboar... File ui/events/test/keyboard_layout.h (right): https://codereview.chromium.org/2197113002/diff/140001/ui/events/test/keyboar... ui/events/test/keyboard_layout.h:13: #elif defined(OS_MACOSX) && !defined(OS_IOS) It's strange to have this conditional on OS_MACOSX but !OS_IOS, but then to have a _mac.cc - won't that therefore get build on iOS, still? https://codereview.chromium.org/2197113002/diff/140001/ui/events/test/keyboar... ui/events/test/keyboard_layout.h:35: // Dummy type for unsupported platforms. Do we need dummy types, or should we simply not define the class at all unless the platform is a supported one? https://codereview.chromium.org/2197113002/diff/140001/ui/events/test/keyboar... File ui/events/test/keyboard_layout_mac.cc (right): https://codereview.chromium.org/2197113002/diff/140001/ui/events/test/keyboar... ui/events/test/keyboard_layout_mac.cc:14: DCHECK_EQ(KEYBOARD_LAYOUT_ENGLISH_US, layout); nit: I generally recommend leaving a blank-line between logical blocks in code - in this case the DCHECK is not related to the input_source_id definition. https://codereview.chromium.org/2197113002/diff/140001/ui/events/test/keyboar... ui/events/test/keyboard_layout_mac.cc:15: const char input_source_id[] = "com.apple.keylayout.US"; kInputSourceId? Or kUsInputSourceId? https://codereview.chromium.org/2197113002/diff/140001/ui/events/test/keyboar... ui/events/test/keyboard_layout_mac.cc:17: base::ScopedCFTypeRef<CFMutableDictionaryRef> filter_dict( Can we give the variables in this section more helpful names, e.g. this is the input-source-list filter, right? Similarly the id_ref below is really the input-source-id-ref, for example. https://codereview.chromium.org/2197113002/diff/140001/ui/events/test/keyboar... File ui/events/test/keyboard_layout_win.cc (right): https://codereview.chromium.org/2197113002/diff/140001/ui/events/test/keyboar... ui/events/test/keyboard_layout_win.cc:31: return LoadKeyboardLayout(L"00000412", KLF_ACTIVATE); We no longer care about DrMemory failures? https://codereview.chromium.org/2197113002/diff/140001/ui/events/test/keyboar... ui/events/test/keyboard_layout_win.cc:34: default: You're handling all 7 possibilities; do you need a default case here?
https://codereview.chromium.org/2197113002/diff/140001/ui/events/test/keyboar... File ui/events/test/keyboard_layout.h (right): https://codereview.chromium.org/2197113002/diff/140001/ui/events/test/keyboar... ui/events/test/keyboard_layout.h:13: #elif defined(OS_MACOSX) && !defined(OS_IOS) On 2016/09/13 20:03:16, Wez wrote: > It's strange to have this conditional on OS_MACOSX but !OS_IOS, but then to have > a _mac.cc - won't that therefore get build on iOS, still? I haven't tried to compile on iOS after this change. You are right. I'll exclude _mac.cc file on iOS. https://codereview.chromium.org/2197113002/diff/140001/ui/events/test/keyboar... ui/events/test/keyboard_layout.h:35: // Dummy type for unsupported platforms. On 2016/09/13 20:03:16, Wez wrote: > Do we need dummy types, or should we simply not define the class at all unless > the platform is a supported one? Tests disabled with DISABLED_ prefix still require PlatformKeyboardLayout and ScopedKeyboardLayout to compile. https://codereview.chromium.org/2197113002/diff/140001/ui/events/test/keyboar... File ui/events/test/keyboard_layout_mac.cc (right): https://codereview.chromium.org/2197113002/diff/140001/ui/events/test/keyboar... ui/events/test/keyboard_layout_mac.cc:14: DCHECK_EQ(KEYBOARD_LAYOUT_ENGLISH_US, layout); On 2016/09/13 20:03:16, Wez wrote: > nit: I generally recommend leaving a blank-line between logical blocks in code - > in this case the DCHECK is not related to the input_source_id definition. Done. https://codereview.chromium.org/2197113002/diff/140001/ui/events/test/keyboar... ui/events/test/keyboard_layout_mac.cc:15: const char input_source_id[] = "com.apple.keylayout.US"; On 2016/09/13 20:03:16, Wez wrote: > kInputSourceId? Or kUsInputSourceId? Done. https://codereview.chromium.org/2197113002/diff/140001/ui/events/test/keyboar... ui/events/test/keyboard_layout_mac.cc:17: base::ScopedCFTypeRef<CFMutableDictionaryRef> filter_dict( On 2016/09/13 20:03:16, Wez wrote: > Can we give the variables in this section more helpful names, e.g. this is the > input-source-list filter, right? Similarly the id_ref below is really the > input-source-id-ref, for example. I moved this code from chrome/test/chromedriver/test_util.cc. Sure, I can rename variables. https://codereview.chromium.org/2197113002/diff/140001/ui/events/test/keyboar... File ui/events/test/keyboard_layout_win.cc (right): https://codereview.chromium.org/2197113002/diff/140001/ui/events/test/keyboar... ui/events/test/keyboard_layout_win.cc:31: return LoadKeyboardLayout(L"00000412", KLF_ACTIVATE); On 2016/09/13 20:03:16, Wez wrote: > We no longer care about DrMemory failures? Peter Kasting suggested this change here: https://codereview.chromium.org/2197113002/diff/20001/ui/events/test/keyboard... https://codereview.chromium.org/2197113002/diff/140001/ui/events/test/keyboar... ui/events/test/keyboard_layout_win.cc:34: default: On 2016/09/13 20:03:16, Wez wrote: > You're handling all 7 possibilities; do you need a default case here? I'll move NOTREACHED() outside of switch statement.
lgtm https://codereview.chromium.org/2197113002/diff/140001/ui/events/test/keyboar... File ui/events/test/keyboard_layout_mac.cc (right): https://codereview.chromium.org/2197113002/diff/140001/ui/events/test/keyboar... ui/events/test/keyboard_layout_mac.cc:17: base::ScopedCFTypeRef<CFMutableDictionaryRef> filter_dict( On 2016/09/14 13:56:06, Tomasz Moniuszko wrote: > On 2016/09/13 20:03:16, Wez wrote: > > Can we give the variables in this section more helpful names, e.g. this is the > > input-source-list filter, right? Similarly the id_ref below is really the > > input-source-id-ref, for example. > > I moved this code from chrome/test/chromedriver/test_util.cc. Sure, I can rename > variables. Thanks! https://codereview.chromium.org/2197113002/diff/140001/ui/events/test/keyboar... File ui/events/test/keyboard_layout_win.cc (right): https://codereview.chromium.org/2197113002/diff/140001/ui/events/test/keyboar... ui/events/test/keyboard_layout_win.cc:31: return LoadKeyboardLayout(L"00000412", KLF_ACTIVATE); On 2016/09/14 13:56:06, Tomasz Moniuszko wrote: > On 2016/09/13 20:03:16, Wez wrote: > > We no longer care about DrMemory failures? > > Peter Kasting suggested this change here: > https://codereview.chromium.org/2197113002/diff/20001/ui/events/test/keyboard... Peter's approach seems reasonable, but let's comment on the bug to make clear that we're switching the implementation to require the DrMemory exception, or perhaps just include that original bug in the BUG= line so this CL will appear there automagically. https://codereview.chromium.org/2197113002/diff/140001/ui/events/test/keyboar... ui/events/test/keyboard_layout_win.cc:34: default: On 2016/09/14 13:56:06, Tomasz Moniuszko wrote: > On 2016/09/13 20:03:16, Wez wrote: > > You're handling all 7 possibilities; do you need a default case here? > > I'll move NOTREACHED() outside of switch statement. Acknowledged. https://codereview.chromium.org/2197113002/diff/160001/ui/events/test/keyboar... File ui/events/test/keyboard_layout.cc (right): https://codereview.chromium.org/2197113002/diff/160001/ui/events/test/keyboar... ui/events/test/keyboard_layout.cc:20: #if !defined(OS_WIN) && (!defined(OS_MACOSX) || defined(OS_IOS)) nit: It may actually be clearer to write: !(defined(OS_MACOSX) && !defined(OS_IOS)) so that the MACOSX/IOS term is directly recognizable as identical (albeit then inverted) to the preprocessor checks used elsewhere. Up to you, though.
https://codereview.chromium.org/2197113002/diff/160001/ui/events/test/keyboar... File ui/events/test/keyboard_layout.cc (right): https://codereview.chromium.org/2197113002/diff/160001/ui/events/test/keyboar... ui/events/test/keyboard_layout.cc:20: #if !defined(OS_WIN) && (!defined(OS_MACOSX) || defined(OS_IOS)) On 2016/09/16 00:57:18, Wez wrote: > nit: It may actually be clearer to write: > > !(defined(OS_MACOSX) && !defined(OS_IOS)) > > so that the MACOSX/IOS term is directly recognizable as identical (albeit then > inverted) to the preprocessor checks used elsewhere. Up to you, though. Done.
On 2016/09/16 00:57:18, Wez wrote: > https://codereview.chromium.org/2197113002/diff/140001/ui/events/test/keyboar... > File ui/events/test/keyboard_layout_win.cc (right): > > https://codereview.chromium.org/2197113002/diff/140001/ui/events/test/keyboar... > ui/events/test/keyboard_layout_win.cc:31: return LoadKeyboardLayout(L"00000412", > KLF_ACTIVATE); > On 2016/09/14 13:56:06, Tomasz Moniuszko wrote: > > On 2016/09/13 20:03:16, Wez wrote: > > > We no longer care about DrMemory failures? > > > > Peter Kasting suggested this change here: > > > https://codereview.chromium.org/2197113002/diff/20001/ui/events/test/keyboard... > > Peter's approach seems reasonable, but let's comment on the bug to make clear > that we're switching the implementation to require the DrMemory exception, or > perhaps just include that original bug in the BUG= line so this CL will appear > there automagically. I left a comment on the bug: https://bugs.chromium.org/p/chromium/issues/detail?id=624527#c1
tmoniuszko@opera.com changed reviewers: + samuong@chromium.org
samuong@, there's no reply from frankf@. Could you maybe take a look at files in //chrome/test/chromedriver?
lgtm, thanks for doing this frank doesn't work on chrome anymore
lgtm
The CQ bit was checked by tmoniuszko@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/2197113002/#ps180001 (title: "Change MacOSX/iOS preprocessor check as suggested in the review")
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
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by tmoniuszko@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, wez@chromium.org, samuong@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2197113002/#ps200001 (title: "More Mac and iOS build fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Force U.S. English keyboard layout for TextfieldTest.KeysWithModifiersTest BUG=633136 ========== to ========== Force U.S. English keyboard layout for TextfieldTest.KeysWithModifiersTest BUG=633136 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Force U.S. English keyboard layout for TextfieldTest.KeysWithModifiersTest BUG=633136 ========== to ========== Force U.S. English keyboard layout for TextfieldTest.KeysWithModifiersTest BUG=633136 Committed: https://crrev.com/19cb1e03c46a4df0c0fd54171369152bcb19c237 Cr-Commit-Position: refs/heads/master@{#419454} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/19cb1e03c46a4df0c0fd54171369152bcb19c237 Cr-Commit-Position: refs/heads/master@{#419454}
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:200001) has been created in https://codereview.chromium.org/2349253002/ by iclelland@chromium.org. The reason for reverting is: Tests are failing on Mac ASAN builder. See details here: https://uberchromegw.corp.google.com/i/chromium.memory/builders/Mac%20ASan%20... First failed build: https://uberchromegw.corp.google.com/i/chromium.memory/builders/Mac%20ASan%20.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
