Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
435023003:
Add pretargethandler to exit overview mode on touch/click. (Closed)
|
Created:
6 years, 4 months ago by rsadam Modified:
6 years, 4 months ago CC:
chromium-reviews, kalyank, sadrul, ben+ash_chromium.org, tdanderson Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionAdd pretargethandler to exit overview mode on touch/click.
Touching or clicking on the background page while in overview mode will now
dismiss it.
BUG=364507
TEST=TBA
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287601
Patch Set 1 #Patch Set 2 : Add touch to exit. #
Total comments: 18
Patch Set 3 : #
Total comments: 8
Patch Set 4 : Addressed feedback. #Patch Set 5 : Fix diamond inheritance. #
Messages
Total messages: 19 (0 generated)
Hi Biao, PTAL! I haven't written the unittests yet, but will follow up with them.
Hey Raheem, please see my unsolicited drive-by review comments below! https://codereview.chromium.org/435023003/diff/20001/ash/desktop_background/d... File ash/desktop_background/desktop_background_view.cc (right): https://codereview.chromium.org/435023003/diff/20001/ash/desktop_background/d... ash/desktop_background/desktop_background_view.cc:76: namespace internal { newline after line 76 https://codereview.chromium.org/435023003/diff/20001/ash/desktop_background/d... ash/desktop_background/desktop_background_view.cc:88: virtual void OnMouseEvent(ui::MouseEvent* event) OVERRIDE { I'd add a CHECK_EQ to verify that the phase is actually EP_PRETARGET (here and also at the top of the other method). https://codereview.chromium.org/435023003/diff/20001/ash/desktop_background/d... ash/desktop_background/desktop_background_view.cc:91: if (event->type() == ui::ET_MOUSE_RELEASED && controller->IsSelecting()) { Since conditional is only one line, omit {} around the body. https://codereview.chromium.org/435023003/diff/20001/ash/desktop_background/d... ash/desktop_background/desktop_background_view.cc:93: } Mark the event as handled (here and also in the other method). https://codereview.chromium.org/435023003/diff/20001/ash/desktop_background/d... ash/desktop_background/desktop_background_view.cc:96: virtual void OnTouchEvent(ui::TouchEvent* event) OVERRIDE { We should instead listen for ET_GESTURE_TAP instead of raw touch events. https://codereview.chromium.org/435023003/diff/20001/ash/desktop_background/d... ash/desktop_background/desktop_background_view.cc:99: if (event->type() == ui::ET_TOUCH_RELEASED && controller->IsSelecting()) { Same as above, no {} https://codereview.chromium.org/435023003/diff/20001/ash/desktop_background/d... ash/desktop_background/desktop_background_view.cc:105: }; newline after line 105 https://codereview.chromium.org/435023003/diff/20001/ash/desktop_background/d... File ash/desktop_background/desktop_background_view.h (right): https://codereview.chromium.org/435023003/diff/20001/ash/desktop_background/d... ash/desktop_background/desktop_background_view.h:15: class PreEventDispatchHandler; This looks pretty crowded. Does the style guide or 'git cl format' have anything to say about this?
Thanks for the driveby comments! Sorry, in hindsight, I should have added you as a reviewer instead of just cc'ing you before asking bshe for OWNERS. https://codereview.chromium.org/435023003/diff/20001/ash/desktop_background/d... File ash/desktop_background/desktop_background_view.cc (right): https://codereview.chromium.org/435023003/diff/20001/ash/desktop_background/d... ash/desktop_background/desktop_background_view.cc:76: namespace internal { On 2014/08/01 19:32:14, tdanderson wrote: > newline after line 76 Done. https://codereview.chromium.org/435023003/diff/20001/ash/desktop_background/d... ash/desktop_background/desktop_background_view.cc:88: virtual void OnMouseEvent(ui::MouseEvent* event) OVERRIDE { On 2014/08/01 19:32:14, tdanderson wrote: > I'd add a CHECK_EQ to verify that the phase is actually EP_PRETARGET (here and > also at the top of the other method). Done. https://codereview.chromium.org/435023003/diff/20001/ash/desktop_background/d... ash/desktop_background/desktop_background_view.cc:91: if (event->type() == ui::ET_MOUSE_RELEASED && controller->IsSelecting()) { On 2014/08/01 19:32:14, tdanderson wrote: > Since conditional is only one line, omit {} around the body. Acknowledged! Readded the braces since I moved the stopPropagation in here. https://codereview.chromium.org/435023003/diff/20001/ash/desktop_background/d... ash/desktop_background/desktop_background_view.cc:93: } On 2014/08/01 19:32:14, tdanderson wrote: > Mark the event as handled (here and also in the other method). Based on my understanding we should only do this if we use the event, so it should be within the conditional, right? https://codereview.chromium.org/435023003/diff/20001/ash/desktop_background/d... ash/desktop_background/desktop_background_view.cc:96: virtual void OnTouchEvent(ui::TouchEvent* event) OVERRIDE { On 2014/08/01 19:32:14, tdanderson wrote: > We should instead listen for ET_GESTURE_TAP instead of raw touch events. Done. https://codereview.chromium.org/435023003/diff/20001/ash/desktop_background/d... ash/desktop_background/desktop_background_view.cc:99: if (event->type() == ui::ET_TOUCH_RELEASED && controller->IsSelecting()) { On 2014/08/01 19:32:14, tdanderson wrote: > Same as above, no {} Acknowledged. https://codereview.chromium.org/435023003/diff/20001/ash/desktop_background/d... ash/desktop_background/desktop_background_view.cc:105: }; On 2014/08/01 19:32:14, tdanderson wrote: > newline after line 105 Done. https://codereview.chromium.org/435023003/diff/20001/ash/desktop_background/d... File ash/desktop_background/desktop_background_view.h (right): https://codereview.chromium.org/435023003/diff/20001/ash/desktop_background/d... ash/desktop_background/desktop_background_view.h:15: class PreEventDispatchHandler; On 2014/08/01 19:32:15, tdanderson wrote: > This looks pretty crowded. Does the style guide or 'git cl format' have anything > to say about this? git cl format didn't - and i don't see anything in the style guide. Judging by other classes in the code base, there seems to be a line after the namespace and a line before closing it.
I've left a few more comments below. Also, git logs are much easier to read if you format your CL description as: <title> <blank line> <description of change> <blank line> BUG=... TEST=... Also make sure you are respecting the 80 character line length limit even in the CL description. https://codereview.chromium.org/435023003/diff/20001/ash/desktop_background/d... File ash/desktop_background/desktop_background_view.cc (right): https://codereview.chromium.org/435023003/diff/20001/ash/desktop_background/d... ash/desktop_background/desktop_background_view.cc:93: } On 2014/08/01 19:57:09, rsadam wrote: > On 2014/08/01 19:32:14, tdanderson wrote: > > Mark the event as handled (here and also in the other method). > > Based on my understanding we should only do this if we use the event, so it > should be within the conditional, right? Yep, that's correct. https://codereview.chromium.org/435023003/diff/20001/ash/desktop_background/d... File ash/desktop_background/desktop_background_view.h (right): https://codereview.chromium.org/435023003/diff/20001/ash/desktop_background/d... ash/desktop_background/desktop_background_view.h:15: class PreEventDispatchHandler; On 2014/08/01 19:57:10, rsadam wrote: > On 2014/08/01 19:32:15, tdanderson wrote: > > This looks pretty crowded. Does the style guide or 'git cl format' have > anything > > to say about this? > git cl format didn't - and i don't see anything in the style guide. Judging by > other classes in the code base, there seems to be a line after the namespace and > a line before closing it. The way you have changed it in the latest patch set looks reasonable to me. https://codereview.chromium.org/435023003/diff/40001/ash/desktop_background/d... File ash/desktop_background/desktop_background_view.cc (right): https://codereview.chromium.org/435023003/diff/40001/ash/desktop_background/d... ash/desktop_background/desktop_background_view.cc:91: if (event->handled()) Is there a particular reason you added this check in here since the last patch set? https://codereview.chromium.org/435023003/diff/40001/ash/desktop_background/d... ash/desktop_background/desktop_background_view.cc:101: virtual void OnTouchEvent(ui::TouchEvent* event) OVERRIDE { Since you've changed to listening for gestures, this needs to be OnGestureEvent(ui::GestureEvent* event) instead of OnTouchEvent().
https://codereview.chromium.org/435023003/diff/40001/ash/desktop_background/d... File ash/desktop_background/desktop_background_view.cc (right): https://codereview.chromium.org/435023003/diff/40001/ash/desktop_background/d... ash/desktop_background/desktop_background_view.cc:127: DesktopBackgroundView::~DesktopBackgroundView() { nit: do you need to remove the handler here? https://codereview.chromium.org/435023003/diff/40001/ash/desktop_background/d... File ash/desktop_background/desktop_background_view.h (right): https://codereview.chromium.org/435023003/diff/40001/ash/desktop_background/d... ash/desktop_background/desktop_background_view.h:14: namespace internal { namespace internal is discouraged, see: https://codereview.chromium.org/224113005 you can probably just remove the internal namespace.
https://codereview.chromium.org/435023003/diff/40001/ash/desktop_background/d... File ash/desktop_background/desktop_background_view.cc (right): https://codereview.chromium.org/435023003/diff/40001/ash/desktop_background/d... ash/desktop_background/desktop_background_view.cc:91: if (event->handled()) On 2014/08/01 20:09:27, tdanderson wrote: > Is there a particular reason you added this check in here since the last patch > set? I went back and looked at root_view.cc and this check is there:https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/root_view.cc&l=68 I thought this meant that events could be sent to pretarget handlers, even if an ancestor pretarget handler had dealt with the event. https://codereview.chromium.org/435023003/diff/40001/ash/desktop_background/d... ash/desktop_background/desktop_background_view.cc:101: virtual void OnTouchEvent(ui::TouchEvent* event) OVERRIDE { On 2014/08/01 20:09:27, tdanderson wrote: > Since you've changed to listening for gestures, this needs to be > OnGestureEvent(ui::GestureEvent* event) instead of OnTouchEvent(). Done. https://codereview.chromium.org/435023003/diff/40001/ash/desktop_background/d... ash/desktop_background/desktop_background_view.cc:127: DesktopBackgroundView::~DesktopBackgroundView() { On 2014/08/05 11:38:28, bshe wrote: > nit: do you need to remove the handler here? Done. https://codereview.chromium.org/435023003/diff/40001/ash/desktop_background/d... File ash/desktop_background/desktop_background_view.h (right): https://codereview.chromium.org/435023003/diff/40001/ash/desktop_background/d... ash/desktop_background/desktop_background_view.h:14: namespace internal { On 2014/08/05 11:38:29, bshe wrote: > namespace internal is discouraged, see: > https://codereview.chromium.org/224113005 > > you can probably just remove the internal namespace. Done.
On 2014/08/05 14:52:55, rsadam wrote: > https://codereview.chromium.org/435023003/diff/40001/ash/desktop_background/d... > File ash/desktop_background/desktop_background_view.cc (right): > > https://codereview.chromium.org/435023003/diff/40001/ash/desktop_background/d... > ash/desktop_background/desktop_background_view.cc:91: if (event->handled()) > On 2014/08/01 20:09:27, tdanderson wrote: > > Is there a particular reason you added this check in here since the last patch > > set? > > I went back and looked at root_view.cc and this check is > there:https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/root_view.cc&l=68 > > I thought this meant that events could be sent to pretarget handlers, even if an > ancestor pretarget handler had dealt with the event. > > https://codereview.chromium.org/435023003/diff/40001/ash/desktop_background/d... > ash/desktop_background/desktop_background_view.cc:101: virtual void > OnTouchEvent(ui::TouchEvent* event) OVERRIDE { > On 2014/08/01 20:09:27, tdanderson wrote: > > Since you've changed to listening for gestures, this needs to be > > OnGestureEvent(ui::GestureEvent* event) instead of OnTouchEvent(). > > Done. > > https://codereview.chromium.org/435023003/diff/40001/ash/desktop_background/d... > ash/desktop_background/desktop_background_view.cc:127: > DesktopBackgroundView::~DesktopBackgroundView() { > On 2014/08/05 11:38:28, bshe wrote: > > nit: do you need to remove the handler here? > > Done. > > https://codereview.chromium.org/435023003/diff/40001/ash/desktop_background/d... > File ash/desktop_background/desktop_background_view.h (right): > > https://codereview.chromium.org/435023003/diff/40001/ash/desktop_background/d... > ash/desktop_background/desktop_background_view.h:14: namespace internal { > On 2014/08/05 11:38:29, bshe wrote: > > namespace internal is discouraged, see: > > https://codereview.chromium.org/224113005 > > > > you can probably just remove the internal namespace. > > Done. lgtm
lgtm2
The CQ bit was checked by rsadam@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rsadam@chromium.org/435023003/60001
Fix inheritance compile error on window. Views::View already extends EventHandler.
The CQ bit was checked by rsadam@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rsadam@chromium.org/435023003/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/42704)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/42766)
The CQ bit was checked by rsadam@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rsadam@chromium.org/435023003/80001
Message was sent while issue was closed.
Change committed as 287601 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
