Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2095283002:
MacViews: Don't handle character events in textfield_unittest's MockInputMethod::DispatchKeyEvent. (Closed)
|
Created:
4 years, 5 months ago by karandeepb Modified:
4 years, 5 months ago CC:
chromium-reviews, tfarina, James Su, shuchen+watch_chromium.org, yusukes+watch_chromium.org, nona+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews: Don't handle character events in textfield_unittest's MockInputMethod::DispatchKeyEvent.
MockInputMethod::DispatchKeyEvent in textfield_unittest.cc currently has a lot
of logic to handle key events. While this is appropriate for other platforms,
on Mac this is incorrect since key events don't pass through InputMethod on Mac.
This causes the test pipeline to be different from the production one on Mac. As
a result some tests may incorrectly pass on MacViews.
This CL fixes the issue by changing MockInputMethod::DispatchKeyEvent to behave
similarly to InputMethodMac::DispatchKeyEvent for character events. Composition
still needs to be mocked, since its not possible to generate test events which
trigger the appropriate NSResponder action messages for composition. This also
necessitates modifying SendKeyEvent(base::char16 ch) in textfield_unittest.cc to
dispatch all character events regularly on Mac, rather than sending them
directly to the InputMethod.
This is a precursor to crrev.com/2096363002 which adds a test
TextfieldTest.TextInputType_InsertionTest, which should have failed on MacViews,
but doesn't currently due to this issue.
BUG=623036
Committed: https://crrev.com/32409d8df5d505a66ed1710f102c84b90456f696
Cr-Commit-Position: refs/heads/master@{#403110}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address review comments. Rebase. #Patch Set 3 : Fix trybots. #
Total comments: 2
Patch Set 4 : Modify comment. #
Dependent Patchsets: Messages
Total messages: 30 (18 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== --- BUG= ========== to ========== --- BUG= ==========
Patchset #1 (id:20001) has been deleted
Description was changed from ========== --- BUG= ========== to ========== MacViews: Make MockInputMethod::DispatchKeyEvent behavior similar to InputMethodMac. In textfield_unittest.cc currently, MockInputMethod::DispatchKeyEvent has a lot of logic to handle key events. While this is appropriate for other platforms, on Mac this is incorrect since key events don't pass through InputMethod on Mac. This causes the test pipeline to be different from the production one on Mac. As a result some tests may incorrectly pass on MacViews. This CL fixes the issue by changing MockInputMethod::DispatchKeyEvent to behave similarly to InputMethodMac::DispatchKeyEvent. This is a precursor to crrev.com/2096363002 which adds a test TextfieldTest.TextInputType_InsertionTest, which should have failed on MacViews, but doesn't currently due to this issue. BUG=623036 ==========
Description was changed from ========== MacViews: Make MockInputMethod::DispatchKeyEvent behavior similar to InputMethodMac. In textfield_unittest.cc currently, MockInputMethod::DispatchKeyEvent has a lot of logic to handle key events. While this is appropriate for other platforms, on Mac this is incorrect since key events don't pass through InputMethod on Mac. This causes the test pipeline to be different from the production one on Mac. As a result some tests may incorrectly pass on MacViews. This CL fixes the issue by changing MockInputMethod::DispatchKeyEvent to behave similarly to InputMethodMac::DispatchKeyEvent. This is a precursor to crrev.com/2096363002 which adds a test TextfieldTest.TextInputType_InsertionTest, which should have failed on MacViews, but doesn't currently due to this issue. BUG=623036 ========== to ========== MacViews: Make MockInputMethod::DispatchKeyEvent behavior similar to InputMethodMac. In textfield_unittest.cc currently, MockInputMethod::DispatchKeyEvent has a lot of logic to handle key events. While this is appropriate for other platforms, on Mac this is incorrect since key events don't pass through InputMethod on Mac. This causes the test pipeline to be different from the production one on Mac. As a result some tests may incorrectly pass on MacViews. This CL fixes the issue by changing MockInputMethod::DispatchKeyEvent to behave similarly to InputMethodMac::DispatchKeyEvent. This is a precursor to crrev.com/2096363002 which adds a test TextfieldTest.TextInputType_InsertionTest, which should have failed on MacViews, but doesn't currently due to this issue. BUG=623036 ==========
Description was changed from ========== MacViews: Make MockInputMethod::DispatchKeyEvent behavior similar to InputMethodMac. In textfield_unittest.cc currently, MockInputMethod::DispatchKeyEvent has a lot of logic to handle key events. While this is appropriate for other platforms, on Mac this is incorrect since key events don't pass through InputMethod on Mac. This causes the test pipeline to be different from the production one on Mac. As a result some tests may incorrectly pass on MacViews. This CL fixes the issue by changing MockInputMethod::DispatchKeyEvent to behave similarly to InputMethodMac::DispatchKeyEvent. This is a precursor to crrev.com/2096363002 which adds a test TextfieldTest.TextInputType_InsertionTest, which should have failed on MacViews, but doesn't currently due to this issue. BUG=623036 ========== to ========== MacViews: Make MockInputMethod::DispatchKeyEvent behavior similar to InputMethodMac. MockInputMethod::DispatchKeyEvent currently has a lot of logic to handle key events. While this is appropriate for other platforms, on Mac this is incorrect since key events don't pass through InputMethod on Mac. This causes the test pipeline to be different from the production one on Mac. As a result some tests may incorrectly pass on MacViews. This CL fixes the issue by changing MockInputMethod::DispatchKeyEvent to behave similarly to InputMethodMac::DispatchKeyEvent. This is a precursor to crrev.com/2096363002 which adds a test TextfieldTest.TextInputType_InsertionTest, which should have failed on MacViews, but doesn't currently due to this issue. BUG=623036 ==========
karandeepb@chromium.org changed reviewers: + tapted@chromium.org
PTAL Trent. https://codereview.chromium.org/2095283002/diff/40001/ui/views/controls/textf... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2095283002/diff/40001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:526: input_method_->DispatchKeyEvent(&event); Not sure if the event needs to be dispatched directly to |input_method_| on other platforms. The tests also work with a regular dispatch.
https://codereview.chromium.org/2095283002/diff/40001/ui/events/test/cocoa_te... File ui/events/test/cocoa_test_event_utils.mm (right): https://codereview.chromium.org/2095283002/diff/40001/ui/events/test/cocoa_te... ui/events/test/cocoa_test_event_utils.mm:1: // Copyright 2014 The Chromium Authors. All rights reserved. Note there's also ui/base/ime/mock_input_method.h, but this is just modifying textfield_unittests's mock input method, so the CL description is a bit confusing. I'd replace "MockInputMethod" with "textfield_unittest's mocked InputMethod" or something like that https://codereview.chromium.org/2095283002/diff/40001/ui/events/test/cocoa_te... ui/events/test/cocoa_test_event_utils.mm:208: // each unicode character, we use a dummy keycode corresponding to key 'A'. (also this probably only really matters for things with a NSCommandKeyMask since |shifted_character| uses dom_key.ToCharacter() just below.) https://codereview.chromium.org/2095283002/diff/40001/ui/views/controls/textf... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2095283002/diff/40001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:524: event_generator_->Dispatch(&event); can you explain (in the CL description perhaps) why this is necessary in addition to the short-circuit in MockInputMethod? https://codereview.chromium.org/2095283002/diff/40001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:526: input_method_->DispatchKeyEvent(&event); On 2016/06/29 01:42:41, karandeepb wrote: > Not sure if the event needs to be dispatched directly to |input_method_| on > other platforms. The tests also work with a regular dispatch. This looks ok.. Event dispatch can be a bit crazy, so if the test coverage is still good with a shorter path, that typically makes them easier to debug.
also lgtm
Description was changed from ========== MacViews: Make MockInputMethod::DispatchKeyEvent behavior similar to InputMethodMac. MockInputMethod::DispatchKeyEvent currently has a lot of logic to handle key events. While this is appropriate for other platforms, on Mac this is incorrect since key events don't pass through InputMethod on Mac. This causes the test pipeline to be different from the production one on Mac. As a result some tests may incorrectly pass on MacViews. This CL fixes the issue by changing MockInputMethod::DispatchKeyEvent to behave similarly to InputMethodMac::DispatchKeyEvent. This is a precursor to crrev.com/2096363002 which adds a test TextfieldTest.TextInputType_InsertionTest, which should have failed on MacViews, but doesn't currently due to this issue. BUG=623036 ========== to ========== Make textfield_unittest's mocked InputMethod::DispatchKeyEvent behave similarly to InputMethodMac. MockInputMethod::DispatchKeyEvent in textfield_unittest.cc currently has a lot of logic to handle key events. While this is appropriate for other platforms, on Mac this is incorrect since key events don't pass through InputMethod on Mac. This causes the test pipeline to be different from the production one on Mac. As a result some tests may incorrectly pass on MacViews. This CL fixes the issue by changing MockInputMethod::DispatchKeyEvent to behave similarly to InputMethodMac::DispatchKeyEvent. This is a precursor to crrev.com/2096363002 which adds a test TextfieldTest.TextInputType_InsertionTest, which should have failed on MacViews, but doesn't currently due to this issue. BUG=623036 ==========
Description was changed from ========== Make textfield_unittest's mocked InputMethod::DispatchKeyEvent behave similarly to InputMethodMac. MockInputMethod::DispatchKeyEvent in textfield_unittest.cc currently has a lot of logic to handle key events. While this is appropriate for other platforms, on Mac this is incorrect since key events don't pass through InputMethod on Mac. This causes the test pipeline to be different from the production one on Mac. As a result some tests may incorrectly pass on MacViews. This CL fixes the issue by changing MockInputMethod::DispatchKeyEvent to behave similarly to InputMethodMac::DispatchKeyEvent. This is a precursor to crrev.com/2096363002 which adds a test TextfieldTest.TextInputType_InsertionTest, which should have failed on MacViews, but doesn't currently due to this issue. BUG=623036 ========== to ========== Make textfield_unittest's mocked InputMethod::DispatchKeyEvent behave similarly to InputMethodMac. MockInputMethod::DispatchKeyEvent in textfield_unittest.cc currently has a lot of logic to handle key events. While this is appropriate for other platforms, on Mac this is incorrect since key events don't pass through InputMethod on Mac. This causes the test pipeline to be different from the production one on Mac. As a result some tests may incorrectly pass on MacViews. This CL fixes the issue by changing MockInputMethod::DispatchKeyEvent to behave similarly to InputMethodMac::DispatchKeyEvent. This also necessitates modifying SendKeyEvent(base::char16 ch) in textfield_unittest.cc to dispatch all character events regularly on Mac, rather than sending them directly to the InputMethod. This is a precursor to crrev.com/2096363002 which adds a test TextfieldTest.TextInputType_InsertionTest, which should have failed on MacViews, but doesn't currently due to this issue. BUG=623036 ==========
https://codereview.chromium.org/2095283002/diff/40001/ui/events/test/cocoa_te... File ui/events/test/cocoa_test_event_utils.mm (right): https://codereview.chromium.org/2095283002/diff/40001/ui/events/test/cocoa_te... ui/events/test/cocoa_test_event_utils.mm:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2016/06/29 07:21:39, tapted wrote: > Note there's also ui/base/ime/mock_input_method.h, but this is just modifying > textfield_unittests's mock input method, so the CL description is a bit > confusing. I'd replace "MockInputMethod" with "textfield_unittest's mocked > InputMethod" or something like that Done. https://codereview.chromium.org/2095283002/diff/40001/ui/events/test/cocoa_te... ui/events/test/cocoa_test_event_utils.mm:208: // each unicode character, we use a dummy keycode corresponding to key 'A'. On 2016/06/29 07:21:39, tapted wrote: > (also this probably only really matters for things with a NSCommandKeyMask since > |shifted_character| uses dom_key.ToCharacter() just below.) I didn't get the bit about NSCommandKeyMask. This is needed for character events with an undefined keycode. https://codereview.chromium.org/2095283002/diff/40001/ui/views/controls/textf... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2095283002/diff/40001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:524: event_generator_->Dispatch(&event); On 2016/06/29 07:21:39, tapted wrote: > can you explain (in the CL description perhaps) why this is necessary in > addition to the short-circuit in MockInputMethod? Done.
karandeepb@chromium.org changed reviewers: + sky@chromium.org
+sky for ui/ review.
LGTM
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Description was changed from ========== Make textfield_unittest's mocked InputMethod::DispatchKeyEvent behave similarly to InputMethodMac. MockInputMethod::DispatchKeyEvent in textfield_unittest.cc currently has a lot of logic to handle key events. While this is appropriate for other platforms, on Mac this is incorrect since key events don't pass through InputMethod on Mac. This causes the test pipeline to be different from the production one on Mac. As a result some tests may incorrectly pass on MacViews. This CL fixes the issue by changing MockInputMethod::DispatchKeyEvent to behave similarly to InputMethodMac::DispatchKeyEvent. This also necessitates modifying SendKeyEvent(base::char16 ch) in textfield_unittest.cc to dispatch all character events regularly on Mac, rather than sending them directly to the InputMethod. This is a precursor to crrev.com/2096363002 which adds a test TextfieldTest.TextInputType_InsertionTest, which should have failed on MacViews, but doesn't currently due to this issue. BUG=623036 ========== to ========== MacViews: Don't handle character events in textfield_unittest's MockInputMethod::DispatchKeyEvent. MockInputMethod::DispatchKeyEvent in textfield_unittest.cc currently has a lot of logic to handle key events. While this is appropriate for other platforms, on Mac this is incorrect since key events don't pass through InputMethod on Mac. This causes the test pipeline to be different from the production one on Mac. As a result some tests may incorrectly pass on MacViews. This CL fixes the issue by changing MockInputMethod::DispatchKeyEvent to behave similarly to InputMethodMac::DispatchKeyEvent for character events. Composition still needs to be mocked, since its not possible to generate test events which trigger the appropriate NSResponder action messages for composition. This also necessitates modifying SendKeyEvent(base::char16 ch) in textfield_unittest.cc to dispatch all character events regularly on Mac, rather than sending them directly to the InputMethod. This is a precursor to crrev.com/2096363002 which adds a test TextfieldTest.TextInputType_InsertionTest, which should have failed on MacViews, but doesn't currently due to this issue. BUG=623036 ==========
PTAL Trent. Rebasing caused TextfieldTest.TextInputClientTest (which was disabled earlier) to fail, since composition still needs to be mocked in MockInputMethod::DispatchKeyEvent.
yup makes sense - still lgtm (please re-wrap CL description) https://codereview.chromium.org/2095283002/diff/120001/ui/views/controls/text... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2095283002/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:151: // still needs to be mocked, since its not possible to generate test events nit: its -> it's
Description was changed from ========== MacViews: Don't handle character events in textfield_unittest's MockInputMethod::DispatchKeyEvent. MockInputMethod::DispatchKeyEvent in textfield_unittest.cc currently has a lot of logic to handle key events. While this is appropriate for other platforms, on Mac this is incorrect since key events don't pass through InputMethod on Mac. This causes the test pipeline to be different from the production one on Mac. As a result some tests may incorrectly pass on MacViews. This CL fixes the issue by changing MockInputMethod::DispatchKeyEvent to behave similarly to InputMethodMac::DispatchKeyEvent for character events. Composition still needs to be mocked, since its not possible to generate test events which trigger the appropriate NSResponder action messages for composition. This also necessitates modifying SendKeyEvent(base::char16 ch) in textfield_unittest.cc to dispatch all character events regularly on Mac, rather than sending them directly to the InputMethod. This is a precursor to crrev.com/2096363002 which adds a test TextfieldTest.TextInputType_InsertionTest, which should have failed on MacViews, but doesn't currently due to this issue. BUG=623036 ========== to ========== MacViews: Don't handle character events in textfield_unittest's MockInputMethod::DispatchKeyEvent. MockInputMethod::DispatchKeyEvent in textfield_unittest.cc currently has a lot of logic to handle key events. While this is appropriate for other platforms, on Mac this is incorrect since key events don't pass through InputMethod on Mac. This causes the test pipeline to be different from the production one on Mac. As a result some tests may incorrectly pass on MacViews. This CL fixes the issue by changing MockInputMethod::DispatchKeyEvent to behave similarly to InputMethodMac::DispatchKeyEvent for character events. Composition still needs to be mocked, since its not possible to generate test events which trigger the appropriate NSResponder action messages for composition. This also necessitates modifying SendKeyEvent(base::char16 ch) in textfield_unittest.cc to dispatch all character events regularly on Mac, rather than sending them directly to the InputMethod. This is a precursor to crrev.com/2096363002 which adds a test TextfieldTest.TextInputType_InsertionTest, which should have failed on MacViews, but doesn't currently due to this issue. BUG=623036 ==========
https://codereview.chromium.org/2095283002/diff/120001/ui/views/controls/text... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2095283002/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:151: // still needs to be mocked, since its not possible to generate test events On 2016/06/30 05:33:53, tapted wrote: > nit: its -> it's Done.
The CQ bit was checked by karandeepb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, tapted@chromium.org Link to the patchset: https://codereview.chromium.org/2095283002/#ps140001 (title: "Modify comment.")
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 ========== MacViews: Don't handle character events in textfield_unittest's MockInputMethod::DispatchKeyEvent. MockInputMethod::DispatchKeyEvent in textfield_unittest.cc currently has a lot of logic to handle key events. While this is appropriate for other platforms, on Mac this is incorrect since key events don't pass through InputMethod on Mac. This causes the test pipeline to be different from the production one on Mac. As a result some tests may incorrectly pass on MacViews. This CL fixes the issue by changing MockInputMethod::DispatchKeyEvent to behave similarly to InputMethodMac::DispatchKeyEvent for character events. Composition still needs to be mocked, since its not possible to generate test events which trigger the appropriate NSResponder action messages for composition. This also necessitates modifying SendKeyEvent(base::char16 ch) in textfield_unittest.cc to dispatch all character events regularly on Mac, rather than sending them directly to the InputMethod. This is a precursor to crrev.com/2096363002 which adds a test TextfieldTest.TextInputType_InsertionTest, which should have failed on MacViews, but doesn't currently due to this issue. BUG=623036 ========== to ========== MacViews: Don't handle character events in textfield_unittest's MockInputMethod::DispatchKeyEvent. MockInputMethod::DispatchKeyEvent in textfield_unittest.cc currently has a lot of logic to handle key events. While this is appropriate for other platforms, on Mac this is incorrect since key events don't pass through InputMethod on Mac. This causes the test pipeline to be different from the production one on Mac. As a result some tests may incorrectly pass on MacViews. This CL fixes the issue by changing MockInputMethod::DispatchKeyEvent to behave similarly to InputMethodMac::DispatchKeyEvent for character events. Composition still needs to be mocked, since its not possible to generate test events which trigger the appropriate NSResponder action messages for composition. This also necessitates modifying SendKeyEvent(base::char16 ch) in textfield_unittest.cc to dispatch all character events regularly on Mac, rather than sending them directly to the InputMethod. This is a precursor to crrev.com/2096363002 which adds a test TextfieldTest.TextInputType_InsertionTest, which should have failed on MacViews, but doesn't currently due to this issue. BUG=623036 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== MacViews: Don't handle character events in textfield_unittest's MockInputMethod::DispatchKeyEvent. MockInputMethod::DispatchKeyEvent in textfield_unittest.cc currently has a lot of logic to handle key events. While this is appropriate for other platforms, on Mac this is incorrect since key events don't pass through InputMethod on Mac. This causes the test pipeline to be different from the production one on Mac. As a result some tests may incorrectly pass on MacViews. This CL fixes the issue by changing MockInputMethod::DispatchKeyEvent to behave similarly to InputMethodMac::DispatchKeyEvent for character events. Composition still needs to be mocked, since its not possible to generate test events which trigger the appropriate NSResponder action messages for composition. This also necessitates modifying SendKeyEvent(base::char16 ch) in textfield_unittest.cc to dispatch all character events regularly on Mac, rather than sending them directly to the InputMethod. This is a precursor to crrev.com/2096363002 which adds a test TextfieldTest.TextInputType_InsertionTest, which should have failed on MacViews, but doesn't currently due to this issue. BUG=623036 ========== to ========== MacViews: Don't handle character events in textfield_unittest's MockInputMethod::DispatchKeyEvent. MockInputMethod::DispatchKeyEvent in textfield_unittest.cc currently has a lot of logic to handle key events. While this is appropriate for other platforms, on Mac this is incorrect since key events don't pass through InputMethod on Mac. This causes the test pipeline to be different from the production one on Mac. As a result some tests may incorrectly pass on MacViews. This CL fixes the issue by changing MockInputMethod::DispatchKeyEvent to behave similarly to InputMethodMac::DispatchKeyEvent for character events. Composition still needs to be mocked, since its not possible to generate test events which trigger the appropriate NSResponder action messages for composition. This also necessitates modifying SendKeyEvent(base::char16 ch) in textfield_unittest.cc to dispatch all character events regularly on Mac, rather than sending them directly to the InputMethod. This is a precursor to crrev.com/2096363002 which adds a test TextfieldTest.TextInputType_InsertionTest, which should have failed on MacViews, but doesn't currently due to this issue. BUG=623036 Committed: https://crrev.com/32409d8df5d505a66ed1710f102c84b90456f696 Cr-Commit-Position: refs/heads/master@{#403110} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/32409d8df5d505a66ed1710f102c84b90456f696 Cr-Commit-Position: refs/heads/master@{#403110} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
