Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1288893002:
Ctrl-Enter on Bookmark bar link should open in the New Tab. (Closed)
DescriptionCtrl-Enter on Bookmark bar link should open in the New Tab.
While conversion from keyboard event to the mouse event meta keys was
not considered. mouse event should consider meta keys also.
BUG=519630
Committed: https://crrev.com/944c8f763537da751829e0db548a86df7b968ccf
Cr-Commit-Position: refs/heads/master@{#344469}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Changes as per review comments. #
Total comments: 2
Patch Set 3 : Changes as per review comments. #
Total comments: 2
Patch Set 4 : Changes as per review comments. #Messages
Total messages: 20 (5 generated)
deepak.m1@samsung.com changed reviewers: + sadrul@chromium.org
PTAL
deepak.m1@samsung.com changed reviewers: + ajith.v@samsung.com
Adding ajith for peer review.
Please check my comments inline. https://codereview.chromium.org/1288893002/diff/1/ui/views/controls/button/cu... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/1288893002/diff/1/ui/views/controls/button/cu... ui/views/controls/button/custom_button.cc:206: flags, Can we pass event.flags() | ui::EF_LEFT_MOUSE_BUTTON here, so that all meta flags will be passed as info to next layer.
PTAL https://codereview.chromium.org/1288893002/diff/1/ui/views/controls/button/cu... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/1288893002/diff/1/ui/views/controls/button/cu... ui/views/controls/button/custom_button.cc:206: flags, On 2015/08/13 15:11:49, AKVT wrote: > Can we pass event.flags() | ui::EF_LEFT_MOUSE_BUTTON here, so that all meta > flags will be passed as info to next layer. Done.
peer review looks good to me!
https://codereview.chromium.org/1288893002/diff/20001/ui/views/controls/butto... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/1288893002/diff/20001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:203: ui::EF_LEFT_MOUSE_BUTTON | event.flags(), KeyEvent can have KeyEventFlags, which can mean different things for a MouseEvent (e.g. KeyEventFlags::EF_IME_FABRICATED_KEY == MouseEventFlags::EF_IS_DOUBLE_CLICK). So we should set only a white-listed set of flags from event.flags(). Also, please add a test.
I referred custom_button_unittest.cc file for the unit test case for this change but in all the test cases only button state is getting checked. As We are just updating the mouse event flag while creating it from the keyboard event. To check this one way is to locally store the created mouse event and then check the flags but it will be overkill. PTAL https://codereview.chromium.org/1288893002/diff/20001/ui/views/controls/butto... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/1288893002/diff/20001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:203: ui::EF_LEFT_MOUSE_BUTTON | event.flags(), On 2015/08/14 14:52:46, sadrul wrote: > KeyEvent can have KeyEventFlags, which can mean different things for a > MouseEvent (e.g. KeyEventFlags::EF_IME_FABRICATED_KEY == > MouseEventFlags::EF_IS_DOUBLE_CLICK). So we should set only a white-listed set > of flags from event.flags(). > > Also, please add a test. Done.
https://codereview.chromium.org/1288893002/diff/40001/ui/views/controls/butto... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/1288893002/diff/40001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:203: // TODO(beng): remove once NotifyClick takes ui::Event. NotifyClick() now takes a ui::Event. Can we address this TODO here, and just send event, instead of synthesizing the mouse-release event?
Thanks for review and suggestion. Changes done. PTAL https://codereview.chromium.org/1288893002/diff/40001/ui/views/controls/butto... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/1288893002/diff/40001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:203: // TODO(beng): remove once NotifyClick takes ui::Event. On 2015/08/17 14:40:11, sadrul wrote: > NotifyClick() now takes a ui::Event. Can we address this TODO here, and just > send event, instead of synthesizing the mouse-release event? Done.
lgtm
The CQ bit was checked by deepak.m1@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288893002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288893002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, no build URL)
The CQ bit was checked by deepak.m1@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288893002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288893002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/944c8f763537da751829e0db548a86df7b968ccf Cr-Commit-Position: refs/heads/master@{#344469} |
