Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2939283002:
Use wScan instead of wVk to ensure webpage can get correct DOM CodeString (Closed)
DescriptionUse wScan instead of wVk to ensure webpage can get correct DOM CodeString
DOM CodeString comes from scancode, so SendInput should use scancode instead of
virtual-key if the key is associated with a DOM CodeString.
With this change, test cases in https://codereview.chromium.org/2922773002/
should pass.
A KeyboardEventCodeInteractiveTest is also added to ensure KeyboardEvent.code
can be correctly set by using ui_test_utils::SendKeyPressSync().
KeyboardEventCodeInteractiveTest starts a web page to record all the
KeyboardEvent.code it received, sends several key presses through
ui_test_utils::SendKeyPressSync() and verifies whether the record is expected.
BUG=680809
Review-Url: https://codereview.chromium.org/2939283002
Cr-Commit-Position: refs/heads/master@{#481671}
Committed: https://chromium.googlesource.com/chromium/src/+/2ee5f007316b4a7ecf451fce973f581c59de066e
Patch Set 1 #
Total comments: 2
Patch Set 2 : Typo in the comment #Patch Set 3 : Include 2946103002 #
Total comments: 2
Patch Set 4 : Resolve review comments #Patch Set 5 : Undo the test case: it will be covered by 2922773002 #Messages
Total messages: 89 (64 generated)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Use wScan instead of wVK to ensure webpage can get correct DOM CodeString DOM CodeString comes from scancode, so SendInput should use scancode instead of virtual-key. BUG=680809 ========== to ========== Use wScan instead of wVK to ensure webpage can get correct DOM CodeString DOM CodeString comes from scancode, so SendInput should use scancode instead of virtual-key. With this change, test cases in https://codereview.chromium.org/2922773002/ should pass. BUG=680809 ==========
zijiehe@chromium.org changed reviewers: + oshima@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
oshima@chromium.org changed reviewers: + sky@chromium.org
I just moved the code and am not familiar with windows specific stuff. -> sky@ who is more familiar with windows and the owner of this direcotry.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/16 14:52:19, oshima wrote: > I just moved the code and am not familiar with windows specific stuff. -> sky@ > who is more familiar with windows and the owner of this direcotry. Have I accidentally sent this change out for reviewing? I will add you guys back later.
zijiehe@chromium.org changed reviewers: - oshima@chromium.org, sky@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:60001) has been deleted
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Use wScan instead of wVK to ensure webpage can get correct DOM CodeString DOM CodeString comes from scancode, so SendInput should use scancode instead of virtual-key. With this change, test cases in https://codereview.chromium.org/2922773002/ should pass. BUG=680809 ========== to ========== Use wScan instead of wVK to ensure webpage can get correct DOM CodeString DOM CodeString comes from scancode, so SendInput should use scancode instead of virtual-key if the key is associated with a DOM CodeString. With this change, test cases in https://codereview.chromium.org/2922773002/ should pass. BUG=680809 ==========
zijiehe@chromium.org changed reviewers: + sadrul@chromium.org
Sadrul, would you please have a look at this change? Thank you.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Use wScan instead of wVK to ensure webpage can get correct DOM CodeString DOM CodeString comes from scancode, so SendInput should use scancode instead of virtual-key if the key is associated with a DOM CodeString. With this change, test cases in https://codereview.chromium.org/2922773002/ should pass. BUG=680809 ========== to ========== Use wScan instead of wVk to ensure webpage can get correct DOM CodeString DOM CodeString comes from scancode, so SendInput should use scancode instead of virtual-key if the key is associated with a DOM CodeString. With this change, test cases in https://codereview.chromium.org/2922773002/ should pass. BUG=680809 ==========
zijiehe@chromium.org changed reviewers: - sadrul@chromium.org
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #1 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
zijiehe@chromium.org changed reviewers: + sadrul@chromium.org
zijiehe@chromium.org changed reviewers: - sadrul@chromium.org
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #1 (id:80001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
zijiehe@chromium.org changed reviewers: + sadrul@chromium.org
sadrul@chromium.org changed reviewers: + ananta@chromium.org
--> ananta@ for windows change.
lgtm https://codereview.chromium.org/2939283002/diff/100001/ui/base/test/ui_contro... File ui/base/test/ui_controls_internal_win.cc (right): https://codereview.chromium.org/2939283002/diff/100001/ui/base/test/ui_contro... ui/base/test/ui_controls_internal_win.cc:164: // When KEYEVNETF_SCANCODE is used, ki.wVk is ignored, so we do not need to KEYEVENTF_
On 2017/06/20 01:10:54, ananta wrote: > lgtm > > https://codereview.chromium.org/2939283002/diff/100001/ui/base/test/ui_contro... > File ui/base/test/ui_controls_internal_win.cc (right): > > https://codereview.chromium.org/2939283002/diff/100001/ui/base/test/ui_contro... > ui/base/test/ui_controls_internal_win.cc:164: // When KEYEVNETF_SCANCODE is > used, ki.wVk is ignored, so we do not need to > KEYEVENTF_ Is there a way to add unittests for this?
On 2017/06/20 01:11:09, ananta wrote: > On 2017/06/20 01:10:54, ananta wrote: > > lgtm > > > > > https://codereview.chromium.org/2939283002/diff/100001/ui/base/test/ui_contro... > > File ui/base/test/ui_controls_internal_win.cc (right): > > > > > https://codereview.chromium.org/2939283002/diff/100001/ui/base/test/ui_contro... > > ui/base/test/ui_controls_internal_win.cc:164: // When KEYEVNETF_SCANCODE is > > used, ki.wVk is ignored, so we do not need to > > KEYEVENTF_ > > Is there a way to add unittests for this? Yes, it's possible. I am adding one.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
https://codereview.chromium.org/2939283002/diff/100001/ui/base/test/ui_contro... File ui/base/test/ui_controls_internal_win.cc (right): https://codereview.chromium.org/2939283002/diff/100001/ui/base/test/ui_contro... ui/base/test/ui_controls_internal_win.cc:164: // When KEYEVNETF_SCANCODE is used, ki.wVk is ignored, so we do not need to On 2017/06/20 01:10:54, ananta wrote: > KEYEVENTF_ Done.
A test has been added at change https://codereview.chromium.org/2946103002/.
The CQ bit was checked by zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ananta@chromium.org Link to the patchset: https://codereview.chromium.org/2939283002/#ps120001 (title: "Typo in the comment")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
zijiehe@chromium.org changed reviewers: + sky@chromium.org
Maybe Scott can LGTM this change.
On 2017/06/20 23:02:34, Hzj_jie wrote: > A test has been added at change https://codereview.chromium.org/2946103002/. Maybe you could include the test in this CL?
Description was changed from ========== Use wScan instead of wVk to ensure webpage can get correct DOM CodeString DOM CodeString comes from scancode, so SendInput should use scancode instead of virtual-key if the key is associated with a DOM CodeString. With this change, test cases in https://codereview.chromium.org/2922773002/ should pass. BUG=680809 ========== to ========== Use wScan instead of wVk to ensure webpage can get correct DOM CodeString DOM CodeString comes from scancode, so SendInput should use scancode instead of virtual-key if the key is associated with a DOM CodeString. With this change, test cases in https://codereview.chromium.org/2922773002/ should pass. A KeyboardEventCodeInteractiveTest is also added to ensure KeyboardEvent.code can be correctly set by using ui_test_utils::SendKeyPressSync(). KeyboardEventCodeInteractiveTest starts a web page to record all the KeyboardEvent.code it received, sends several key presses through ui_test_utils::SendKeyPressSync() and verifies whether the record is expected. BUG=680809 ==========
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Updated.
A browser test to verify this fix is very heavy. Do you really need a browser test? https://codereview.chromium.org/2939283002/diff/140001/ui/base/test/ui_contro... File ui/base/test/ui_controls_internal_win.cc (right): https://codereview.chromium.org/2939283002/diff/140001/ui/base/test/ui_contro... ui/base/test/ui_controls_internal_win.cc:171: if (key_up) { no {}
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/21 14:56:04, sky wrote: > A browser test to verify this fix is very heavy. Do you really need a browser > test? > > https://codereview.chromium.org/2939283002/diff/140001/ui/base/test/ui_contro... > File ui/base/test/ui_controls_internal_win.cc (right): > > https://codereview.chromium.org/2939283002/diff/140001/ui/base/test/ui_contro... > ui/base/test/ui_controls_internal_win.cc:171: if (key_up) { > no {} Do you have a better suggestion? SendInput() function sends the events to the focused window, so using a browser test is the simplest way I can imagine. Otherwise we can also test in WinWindow or similar layer, but we need more logic and it's not platform independent.
https://codereview.chromium.org/2939283002/diff/140001/ui/base/test/ui_contro... File ui/base/test/ui_controls_internal_win.cc (right): https://codereview.chromium.org/2939283002/diff/140001/ui/base/test/ui_contro... ui/base/test/ui_controls_internal_win.cc:171: if (key_up) { On 2017/06/21 14:56:04, sky wrote: > no {} Done.
A views interactive ui test that calls SendKeyPressSync() and verifies the ui::KeyEvent directly? No need to load a page for this. -Scott On Wed, Jun 21, 2017 at 10:31 AM, <zijiehe@chromium.org> wrote: > On 2017/06/21 14:56:04, sky wrote: > > A browser test to verify this fix is very heavy. Do you really need a > browser > > test? > > > > > https://codereview.chromium.org/2939283002/diff/140001/ui/ > base/test/ui_controls_internal_win.cc > > File ui/base/test/ui_controls_internal_win.cc (right): > > > > > https://codereview.chromium.org/2939283002/diff/140001/ui/ > base/test/ui_controls_internal_win.cc#newcode171 > > ui/base/test/ui_controls_internal_win.cc:171: if (key_up) { > > no {} > > Do you have a better suggestion? SendInput() function sends the events to > the > focused window, so using a browser test is the simplest way I can imagine. > Otherwise we can also test in WinWindow or similar layer, but we need more > logic > and it's not platform independent. > > https://codereview.chromium.org/2939283002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sadrul@chromium.org changed reviewers: - sadrul@chromium.org
This is windows, and sky@ is here. So removing myself from reviewer list.
On 2017/06/21 19:50:04, sky wrote: > A views interactive ui test that calls SendKeyPressSync() and verifies the > ui::KeyEvent directly? No need to load a page for this. > > -Scott > > On Wed, Jun 21, 2017 at 10:31 AM, <mailto:zijiehe@chromium.org> wrote: > > > On 2017/06/21 14:56:04, sky wrote: > > > A browser test to verify this fix is very heavy. Do you really need a > > browser > > > test? > > > > > > > > https://codereview.chromium.org/2939283002/diff/140001/ui/ > > base/test/ui_controls_internal_win.cc > > > File ui/base/test/ui_controls_internal_win.cc (right): > > > > > > > > https://codereview.chromium.org/2939283002/diff/140001/ui/ > > base/test/ui_controls_internal_win.cc#newcode171 > > > ui/base/test/ui_controls_internal_win.cc:171: if (key_up) { > > > no {} > > > > Do you have a better suggestion? SendInput() function sends the events to > > the > > focused window, so using a browser test is the simplest way I can imagine. > > Otherwise we can also test in WinWindow or similar layer, but we need more > > logic > > and it's not platform independent. > > > > https://codereview.chromium.org/2939283002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Sorry, it seems like I cannot work out a solution as you suggested after several attempts. Since I am also working on https://codereview.chromium.org/2922773002/, which already covers the scenario we expected to be tested here. How about let's wait for that change?
SGTM On Wed, Jun 21, 2017 at 4:58 PM, <zijiehe@chromium.org> wrote: > On 2017/06/21 19:50:04, sky wrote: > > A views interactive ui test that calls SendKeyPressSync() and verifies > the > > ui::KeyEvent directly? No need to load a page for this. > > > > -Scott > > > > On Wed, Jun 21, 2017 at 10:31 AM, <mailto:zijiehe@chromium.org> wrote: > > > > > On 2017/06/21 14:56:04, sky wrote: > > > > A browser test to verify this fix is very heavy. Do you really need a > > > browser > > > > test? > > > > > > > > > > > https://codereview.chromium.org/2939283002/diff/140001/ui/ > > > base/test/ui_controls_internal_win.cc > > > > File ui/base/test/ui_controls_internal_win.cc (right): > > > > > > > > > > > https://codereview.chromium.org/2939283002/diff/140001/ui/ > > > base/test/ui_controls_internal_win.cc#newcode171 > > > > ui/base/test/ui_controls_internal_win.cc:171: if (key_up) { > > > > no {} > > > > > > Do you have a better suggestion? SendInput() function sends the events > to > > > the > > > focused window, so using a browser test is the simplest way I can > imagine. > > > Otherwise we can also test in WinWindow or similar layer, but we need > more > > > logic > > > and it's not platform independent. > > > > > > https://codereview.chromium.org/2939283002/ > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > Sorry, it seems like I cannot work out a solution as you suggested after > several > attempts. > Since I am also working on https://codereview.chromium.org/2922773002/, > which > already covers the scenario we expected to be tested here. How about let's > wait > for that change? > > https://codereview.chromium.org/2939283002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Test case has been removed.
LGTM
The CQ bit was checked by zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ananta@chromium.org Link to the patchset: https://codereview.chromium.org/2939283002/#ps180001 (title: "Undo the test case: it will be covered by 2922773002")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1498167201511690,
"parent_rev": "d795cd03fde8b8e23747613e8cc21da840bc4ea8", "commit_rev":
"2ee5f007316b4a7ecf451fce973f581c59de066e"}
Message was sent while issue was closed.
Description was changed from ========== Use wScan instead of wVk to ensure webpage can get correct DOM CodeString DOM CodeString comes from scancode, so SendInput should use scancode instead of virtual-key if the key is associated with a DOM CodeString. With this change, test cases in https://codereview.chromium.org/2922773002/ should pass. A KeyboardEventCodeInteractiveTest is also added to ensure KeyboardEvent.code can be correctly set by using ui_test_utils::SendKeyPressSync(). KeyboardEventCodeInteractiveTest starts a web page to record all the KeyboardEvent.code it received, sends several key presses through ui_test_utils::SendKeyPressSync() and verifies whether the record is expected. BUG=680809 ========== to ========== Use wScan instead of wVk to ensure webpage can get correct DOM CodeString DOM CodeString comes from scancode, so SendInput should use scancode instead of virtual-key if the key is associated with a DOM CodeString. With this change, test cases in https://codereview.chromium.org/2922773002/ should pass. A KeyboardEventCodeInteractiveTest is also added to ensure KeyboardEvent.code can be correctly set by using ui_test_utils::SendKeyPressSync(). KeyboardEventCodeInteractiveTest starts a web page to record all the KeyboardEvent.code it received, sends several key presses through ui_test_utils::SendKeyPressSync() and verifies whether the record is expected. BUG=680809 Review-Url: https://codereview.chromium.org/2939283002 Cr-Commit-Position: refs/heads/master@{#481671} Committed: https://chromium.googlesource.com/chromium/src/+/2ee5f007316b4a7ecf451fce973f... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:180001) as https://chromium.googlesource.com/chromium/src/+/2ee5f007316b4a7ecf451fce973f... |
