Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2125663002:
mus: Add watcher for all touch events. (Closed)
Descriptionmus: Add watcher for all touch events.
BUG=588311
TEST=views_mus_unittests
Committed: https://crrev.com/aa9394e02706b89d95c3676396ebed708a6a2ea7
Cr-Commit-Position: refs/heads/master@{#405297}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Support multiple event observers. #Patch Set 3 : Erase id when removing watcher. #Patch Set 4 : Change to receive array of event observer ids. #Patch Set 5 : Change back a test case. #Patch Set 6 : Move Change struct. #Patch Set 7 : Overload copy assignment operator for Change struct. #
Total comments: 3
Patch Set 8 : Change back to support only one event matcher with TODOs and DCHECKs. #
Total comments: 12
Patch Set 9 : Use std::find; check if has PointerWatcher/TouchEventWatcher. #
Total comments: 7
Patch Set 10 : DCHECK #Patch Set 11 : DCHECK #
Total comments: 13
Patch Set 12 : Early return; etc #Patch Set 13 : Add touch_event_watcher.h to includes #Patch Set 14 : Change back to ifs #
Total comments: 2
Patch Set 15 : Fix a comment #
Messages
Total messages: 40 (16 generated)
riajiang@chromium.org changed reviewers: + sadrul@chromium.org, sky@chromium.org
Can you outline what functionality you need? https://codereview.chromium.org/2125663002/diff/1/ui/views/views.gyp File ui/views/views.gyp (right): https://codereview.chromium.org/2125663002/diff/1/ui/views/views.gyp#newcode302 ui/views/views.gyp:302: 'touch_event_watcher.h', I think you forgot to add this file.
On 2016/07/06 16:23:14, sky wrote: > Can you outline what functionality you need? > > https://codereview.chromium.org/2125663002/diff/1/ui/views/views.gyp > File ui/views/views.gyp (right): > > https://codereview.chromium.org/2125663002/diff/1/ui/views/views.gyp#newcode302 > ui/views/views.gyp:302: 'touch_event_watcher.h', > I think you forgot to add this file. TouchEventWatcher would watch for all PointerKind::TOUCH events. When an event IsTouchPointerEvent(), WindowManagerConnection would pass it to OnTouchEventObserved in the touch hud app which would draw out touch points. touch_event_watcher.h is in ui/views.
https://codereview.chromium.org/2125663002/diff/1/services/ui/ws/window_tree.cc File services/ui/ws/window_tree.cc (right): https://codereview.chromium.org/2125663002/diff/1/services/ui/ws/window_tree.... services/ui/ws/window_tree.cc:1182: } else if (matcher->pointer_kind_matcher) { Shouldn't this conditional be a standalone if? That is, if a type_matcher is specified you validate it, and if a pointer_kind_matcher is specified you validate it too? https://codereview.chromium.org/2125663002/diff/1/ui/views/mus/window_manager... File ui/views/mus/window_manager_connection.cc (right): https://codereview.chromium.org/2125663002/diff/1/ui/views/mus/window_manager... ui/views/mus/window_manager_connection.cc:111: client_->SetEventObserver(std::move(matcher)); Notice how this now conflicts with 92. In an effort to not have a ton of observers we made the client library only support one. It looks like we need to support more than once at a particular time, so you'll need to update the client library to handle that. https://codereview.chromium.org/2125663002/diff/1/ui/views/touch_event_watcher.h File ui/views/touch_event_watcher.h (right): https://codereview.chromium.org/2125663002/diff/1/ui/views/touch_event_watche... ui/views/touch_event_watcher.h:21: class VIEWS_EXPORT TouchEventWatcher { Add description. https://codereview.chromium.org/2125663002/diff/1/ui/views/touch_event_watche... ui/views/touch_event_watcher.h:23: virtual ~TouchEventWatcher() {} move destructor to protected section to reinforce not owned.
https://codereview.chromium.org/2125663002/diff/1/services/ui/ws/window_tree.cc File services/ui/ws/window_tree.cc (right): https://codereview.chromium.org/2125663002/diff/1/services/ui/ws/window_tree.... services/ui/ws/window_tree.cc:1182: } else if (matcher->pointer_kind_matcher) { On 2016/07/06 20:22:02, sky wrote: > Shouldn't this conditional be a standalone if? That is, if a type_matcher is > specified you validate it, and if a pointer_kind_matcher is specified you > validate it too? Yes, changed it to a standalone if. https://codereview.chromium.org/2125663002/diff/1/ui/views/mus/window_manager... File ui/views/mus/window_manager_connection.cc (right): https://codereview.chromium.org/2125663002/diff/1/ui/views/mus/window_manager... ui/views/mus/window_manager_connection.cc:111: client_->SetEventObserver(std::move(matcher)); On 2016/07/06 20:22:02, sky wrote: > Notice how this now conflicts with 92. In an effort to not have a ton of > observers we made the client library only support one. It looks like we need to > support more than once at a particular time, so you'll need to update the client > library to handle that. Done. https://codereview.chromium.org/2125663002/diff/1/ui/views/touch_event_watcher.h File ui/views/touch_event_watcher.h (right): https://codereview.chromium.org/2125663002/diff/1/ui/views/touch_event_watche... ui/views/touch_event_watcher.h:21: class VIEWS_EXPORT TouchEventWatcher { On 2016/07/06 20:22:02, sky wrote: > Add description. Done. https://codereview.chromium.org/2125663002/diff/1/ui/views/touch_event_watche... ui/views/touch_event_watcher.h:23: virtual ~TouchEventWatcher() {} On 2016/07/06 20:22:02, sky wrote: > move destructor to protected section to reinforce not owned. Done. Also changed this in PointerWatcher.
https://codereview.chromium.org/2125663002/diff/120001/services/ui/public/int... File services/ui/public/interfaces/window_tree.mojom (right): https://codereview.chromium.org/2125663002/diff/120001/services/ui/public/int... services/ui/public/interfaces/window_tree.mojom:76: AddEventObserver(EventMatcher? matcher, uint32 observer_id); In thinking about this a bit could we we implement > 1 matchers in the client library? The client library would maintain the set of matchers and build a matcher that matches all the supplied matchers. This way we minimize interaction with the window server and keep the complexity in the client. Is that possible?
https://codereview.chromium.org/2125663002/diff/120001/services/ui/public/int... File services/ui/public/interfaces/window_tree.mojom (right): https://codereview.chromium.org/2125663002/diff/120001/services/ui/public/int... services/ui/public/interfaces/window_tree.mojom:76: AddEventObserver(EventMatcher? matcher, uint32 observer_id); On 2016/07/08 15:41:11, sky wrote: > In thinking about this a bit could we we implement > 1 matchers in the client > library? The client library would maintain the set of matchers and build a > matcher that matches all the supplied matchers. This way we minimize interaction > with the window server and keep the complexity in the client. Is that possible? I think this would be possible (and how I was thinking initially as well). But it's not clear if we are going to need this. The touch-hud is going to be a separate app, and it would need just the TouchEventWatcher. ash currently needs just PointerWatcher. For some accessibility features (which I think should be a separate app), we are going to need some event-watcher that match all touch and mouse events. So with the use-cases we have right now, every client needs just one type of watcher. So it would make sense to not introduce this yet, either on the client or the server side, until we have a use-case where the client needs more than one type of watcher. Maybe just leave a TODO for now?
I'm ok with a TODO and DCHECK that we don't attempt to register both types at the same time. On Fri, Jul 8, 2016 at 8:51 AM, <sadrul@chromium.org> wrote: > > https://codereview.chromium.org/2125663002/diff/120001/services/ui/public/int... > File services/ui/public/interfaces/window_tree.mojom (right): > > https://codereview.chromium.org/2125663002/diff/120001/services/ui/public/int... > services/ui/public/interfaces/window_tree.mojom:76: > AddEventObserver(EventMatcher? matcher, uint32 observer_id); > On 2016/07/08 15:41:11, sky wrote: >> In thinking about this a bit could we we implement > 1 matchers in the > client >> library? The client library would maintain the set of matchers and > build a >> matcher that matches all the supplied matchers. This way we minimize > interaction >> with the window server and keep the complexity in the client. Is that > possible? > > I think this would be possible (and how I was thinking initially as > well). But it's not clear if we are going to need this. The touch-hud is > going to be a separate app, and it would need just the > TouchEventWatcher. ash currently needs just PointerWatcher. For some > accessibility features (which I think should be a separate app), we are > going to need some event-watcher that match all touch and mouse events. > So with the use-cases we have right now, every client needs just one > type of watcher. So it would make sense to not introduce this yet, > either on the client or the server side, until we have a use-case where > the client needs more than one type of watcher. Maybe just leave a TODO > for now? > > https://codereview.chromium.org/2125663002/ -- 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.
riajiang@chromium.org changed reviewers: + msw@chromium.org
+msw, since sky is OOO https://codereview.chromium.org/2125663002/diff/120001/services/ui/public/int... File services/ui/public/interfaces/window_tree.mojom (right): https://codereview.chromium.org/2125663002/diff/120001/services/ui/public/int... services/ui/public/interfaces/window_tree.mojom:76: AddEventObserver(EventMatcher? matcher, uint32 observer_id); On 2016/07/08 15:51:30, sadrul wrote: > On 2016/07/08 15:41:11, sky wrote: > > In thinking about this a bit could we we implement > 1 matchers in the client > > library? The client library would maintain the set of matchers and build a > > matcher that matches all the supplied matchers. This way we minimize > interaction > > with the window server and keep the complexity in the client. Is that > possible? > > I think this would be possible (and how I was thinking initially as well). But > it's not clear if we are going to need this. The touch-hud is going to be a > separate app, and it would need just the TouchEventWatcher. ash currently needs > just PointerWatcher. For some accessibility features (which I think should be a > separate app), we are going to need some event-watcher that match all touch and > mouse events. So with the use-cases we have right now, every client needs just > one type of watcher. So it would make sense to not introduce this yet, either on > the client or the server side, until we have a use-case where the client needs > more than one type of watcher. Maybe just leave a TODO for now? Changed back to support only one event matcher with TODOs and DCHECKs.
https://codereview.chromium.org/2125663002/diff/140001/services/ui/ws/window_... File services/ui/ws/window_tree.cc (right): https://codereview.chromium.org/2125663002/diff/140001/services/ui/ws/window_... services/ui/ws/window_tree.cc:1184: for (ui::mojom::EventType event_type : event_type_whitelist) { Can you use std::find instead? if (matcher->type_matcher) { auto iter = std::find(std::begin(event_type_whitelist), std::end(event_type_whitelist), matcher->type_matcher->type); allowed = iter != std::end(event_type_whitelist); } https://codereview.chromium.org/2125663002/diff/140001/services/ui/ws/window_... services/ui/ws/window_tree.cc:1197: } ditto https://codereview.chromium.org/2125663002/diff/140001/ui/views/mus/window_ma... File ui/views/mus/window_manager_connection.cc (right): https://codereview.chromium.org/2125663002/diff/140001/ui/views/mus/window_ma... ui/views/mus/window_manager_connection.cc:85: // TODO(riajiang): Support multiple event matchers (issue 627146). s#issue 627146#crbug.com/627146# https://codereview.chromium.org/2125663002/diff/140001/ui/views/mus/window_ma... ui/views/mus/window_manager_connection.cc:227: } Change to: if (HasPointerWatcher()) { ... trigger PointerWatchers } else { DCHECK(HasTouchEventWatcher()); ... trigger TouchEventWatchers } https://codereview.chromium.org/2125663002/diff/140001/ui/views/touch_event_w... File ui/views/touch_event_watcher.h (right): https://codereview.chromium.org/2125663002/diff/140001/ui/views/touch_event_w... ui/views/touch_event_watcher.h:29: virtual void OnTouchEventObserved(const ui::LocatedEvent& event, Change to ui::TouchEvent https://codereview.chromium.org/2125663002/diff/140001/ui/views/touch_event_w... ui/views/touch_event_watcher.h:30: const gfx::Point& location_in_screen, The location should not be necessary, since TouchEvent already has that information.
https://codereview.chromium.org/2125663002/diff/140001/services/ui/ws/window_... File services/ui/ws/window_tree.cc (right): https://codereview.chromium.org/2125663002/diff/140001/services/ui/ws/window_... services/ui/ws/window_tree.cc:1184: for (ui::mojom::EventType event_type : event_type_whitelist) { On 2016/07/11 20:47:51, sadrul wrote: > Can you use std::find instead? > > if (matcher->type_matcher) { > auto iter = std::find(std::begin(event_type_whitelist), > std::end(event_type_whitelist), > matcher->type_matcher->type); > allowed = iter != std::end(event_type_whitelist); > } Done. https://codereview.chromium.org/2125663002/diff/140001/services/ui/ws/window_... services/ui/ws/window_tree.cc:1197: } On 2016/07/11 20:47:52, sadrul wrote: > ditto Done. https://codereview.chromium.org/2125663002/diff/140001/ui/views/mus/window_ma... File ui/views/mus/window_manager_connection.cc (right): https://codereview.chromium.org/2125663002/diff/140001/ui/views/mus/window_ma... ui/views/mus/window_manager_connection.cc:85: // TODO(riajiang): Support multiple event matchers (issue 627146). On 2016/07/11 20:47:52, sadrul wrote: > s#issue 627146#crbug.com/627146# Done. https://codereview.chromium.org/2125663002/diff/140001/ui/views/mus/window_ma... ui/views/mus/window_manager_connection.cc:227: } On 2016/07/11 20:47:52, sadrul wrote: > Change to: > > if (HasPointerWatcher()) { > ... trigger PointerWatchers > } else { > DCHECK(HasTouchEventWatcher()); > ... trigger TouchEventWatchers > } Done. https://codereview.chromium.org/2125663002/diff/140001/ui/views/touch_event_w... File ui/views/touch_event_watcher.h (right): https://codereview.chromium.org/2125663002/diff/140001/ui/views/touch_event_w... ui/views/touch_event_watcher.h:29: virtual void OnTouchEventObserved(const ui::LocatedEvent& event, On 2016/07/11 20:47:52, sadrul wrote: > Change to ui::TouchEvent But we are receiving both TouchEvent and TouchPointerEvent which is a PointerEvent not TouchEvent? https://codereview.chromium.org/2125663002/diff/140001/ui/views/touch_event_w... ui/views/touch_event_watcher.h:30: const gfx::Point& location_in_screen, On 2016/07/11 20:47:52, sadrul wrote: > The location should not be necessary, since TouchEvent already has that > information. Done.
lgtm with the comments addressed https://codereview.chromium.org/2125663002/diff/160001/services/ui/ws/window_... File services/ui/ws/window_tree.cc (right): https://codereview.chromium.org/2125663002/diff/160001/services/ui/ws/window_... services/ui/ws/window_tree.cc:1193: allowed = iter != std::end(pointer_kind_whitelist); Actually, let's get rid of this whitelist. Just do: if (matcher->type_matcher) { .. } if (matcher->pointer_kind_matcher) { allowed = pointer_kind == MOUSE || pointer_kind == TOUCH || pointer_kind == PEN; } https://codereview.chromium.org/2125663002/diff/160001/ui/views/mus/window_ma... File ui/views/mus/window_manager_connection.cc (right): https://codereview.chromium.org/2125663002/diff/160001/ui/views/mus/window_ma... ui/views/mus/window_manager_connection.cc:225: if (event.IsTouchEvent() || event.IsTouchPointerEvent()) { DCHECK(IsTouchEvent() || IsTouchPointerEvent()) https://codereview.chromium.org/2125663002/diff/160001/ui/views/mus/window_ma... File ui/views/mus/window_manager_connection.h (right): https://codereview.chromium.org/2125663002/diff/160001/ui/views/mus/window_ma... ui/views/mus/window_manager_connection.h:82: // Returns true if there is one or more pointer watchers for this client. s/pointer//
https://codereview.chromium.org/2125663002/diff/160001/services/ui/ws/window_... File services/ui/ws/window_tree.cc (right): https://codereview.chromium.org/2125663002/diff/160001/services/ui/ws/window_... services/ui/ws/window_tree.cc:1193: allowed = iter != std::end(pointer_kind_whitelist); On 2016/07/12 19:21:05, sadrul wrote: > Actually, let's get rid of this whitelist. Just do: > > if (matcher->type_matcher) { > .. > } > > if (matcher->pointer_kind_matcher) { > allowed = pointer_kind == MOUSE || pointer_kind == TOUCH || pointer_kind == > PEN; > } Done. https://codereview.chromium.org/2125663002/diff/160001/ui/views/mus/window_ma... File ui/views/mus/window_manager_connection.cc (right): https://codereview.chromium.org/2125663002/diff/160001/ui/views/mus/window_ma... ui/views/mus/window_manager_connection.cc:225: if (event.IsTouchEvent() || event.IsTouchPointerEvent()) { On 2016/07/12 19:21:05, sadrul wrote: > DCHECK(IsTouchEvent() || IsTouchPointerEvent()) Done. https://codereview.chromium.org/2125663002/diff/160001/ui/views/mus/window_ma... File ui/views/mus/window_manager_connection.h (right): https://codereview.chromium.org/2125663002/diff/160001/ui/views/mus/window_ma... ui/views/mus/window_manager_connection.h:82: // Returns true if there is one or more pointer watchers for this client. On 2016/07/12 19:21:05, sadrul wrote: > s/pointer// Done.
Description was changed from ========== mus: Add watcher for all touch events. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). ========== to ========== mus: Add watcher for all touch events. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). BUG=588311 TEST=views_mus_unittests ==========
lgtm with nits and a q; +CC jamescook for FYI. https://codereview.chromium.org/2125663002/diff/200001/services/ui/ws/window_... File services/ui/ws/window_tree.cc (right): https://codereview.chromium.org/2125663002/diff/200001/services/ui/ws/window_... services/ui/ws/window_tree.cc:1189: allowed = pointer_kind == ui::mojom::PointerKind::MOUSE || q: Will we imminently add other PointerKind types that shouldn't be allowed? It seems odd to check this if all PointerKind types are allowed. https://codereview.chromium.org/2125663002/diff/200001/ui/views/mus/window_ma... File ui/views/mus/window_manager_connection.cc (right): https://codereview.chromium.org/2125663002/diff/200001/ui/views/mus/window_ma... ui/views/mus/window_manager_connection.cc:224: // Receives TouchEvent and TouchPointerEvent nit: add a trailing period; optionally expand or remove comment. https://codereview.chromium.org/2125663002/diff/200001/ui/views/mus/window_ma... File ui/views/mus/window_manager_connection_unittest.cc (right): https://codereview.chromium.org/2125663002/diff/200001/ui/views/mus/window_ma... ui/views/mus/window_manager_connection_unittest.cc:162: ui::MouseEvent mouse_event(kMouseType[i], gfx::Point(0, 0), nit: use default gfx::Point ctor here and elsewhere. https://codereview.chromium.org/2125663002/diff/200001/ui/views/pointer_watch... File ui/views/pointer_watcher.h (right): https://codereview.chromium.org/2125663002/diff/200001/ui/views/pointer_watch... ui/views/pointer_watcher.h:39: }; nit: private: DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/2125663002/diff/200001/ui/views/touch_event_w... File ui/views/touch_event_watcher.h (right): https://codereview.chromium.org/2125663002/diff/200001/ui/views/touch_event_w... ui/views/touch_event_watcher.h:34: }; nit: private: DISALLOW_COPY_AND_ASSIGN?
jamescook@chromium.org changed reviewers: + jamescook@chromium.org
It feels weird to me to have two "watchers" that can both listen for touch-down events but I guess it's OK. https://codereview.chromium.org/2125663002/diff/200001/services/ui/ws/window_... File services/ui/ws/window_tree.cc (right): https://codereview.chromium.org/2125663002/diff/200001/services/ui/ws/window_... services/ui/ws/window_tree.cc:1184: allowed = iter != std::end(event_type_whitelist); nit: I think this would be clearer with an early return if the item isn't found on the whitelist. Ditto for an early return in the if() block below. For example, if I'm a malicious client and I set matcher->type_matcher->type to KEY_PRESSED and matcher->pointer_kind_matcher->pointer_kind to PointerKind::MOUSE then "allowed" will be true at line 1193, even though I should not be allowed to look for keystrokes. EventMatcher::MatchesEvent() will still do the right thing, and won't match the events, but it would be nice not to have to look at the EventMatcher implementation to know that things are safe.
On 2016/07/12 23:22:59, James Cook wrote: > It feels weird to me to have two "watchers" that can both listen for touch-down > events but I guess it's OK. We do not actually allow installing both types of watcher from the same client right now. We may need to change the API a bit if/when there's a need for it (crbug.com/627146) https://codereview.chromium.org/2125663002/diff/200001/services/ui/ws/window_... File services/ui/ws/window_tree.cc (right): https://codereview.chromium.org/2125663002/diff/200001/services/ui/ws/window_... services/ui/ws/window_tree.cc:1184: allowed = iter != std::end(event_type_whitelist); On 2016/07/12 23:22:59, James Cook wrote: > nit: I think this would be clearer with an early return if the item isn't found > on the whitelist. Ditto for an early return in the if() block below. > > For example, if I'm a malicious client and I set matcher->type_matcher->type to > KEY_PRESSED and matcher->pointer_kind_matcher->pointer_kind to > PointerKind::MOUSE then "allowed" will be true at line 1193, even though I > should not be allowed to look for keystrokes. > > EventMatcher::MatchesEvent() will still do the right thing, and won't match the > events, but it would be nice not to have to look at the EventMatcher > implementation to know that things are safe. Good catch! +1 https://codereview.chromium.org/2125663002/diff/200001/services/ui/ws/window_... services/ui/ws/window_tree.cc:1189: allowed = pointer_kind == ui::mojom::PointerKind::MOUSE || On 2016/07/12 22:23:11, msw wrote: > q: Will we imminently add other PointerKind types that shouldn't be allowed? It > seems odd to check this if all PointerKind types are allowed. I don't really expect new pointer-kinds (I think stylus would just be PEN?). This explicit check is to make sure external clients aren't using some invalid pointer-kind.
https://codereview.chromium.org/2125663002/diff/200001/services/ui/ws/window_... File services/ui/ws/window_tree.cc (right): https://codereview.chromium.org/2125663002/diff/200001/services/ui/ws/window_... services/ui/ws/window_tree.cc:1184: allowed = iter != std::end(event_type_whitelist); On 2016/07/12 23:22:59, James Cook wrote: > nit: I think this would be clearer with an early return if the item isn't found > on the whitelist. Ditto for an early return in the if() block below. > > For example, if I'm a malicious client and I set matcher->type_matcher->type to > KEY_PRESSED and matcher->pointer_kind_matcher->pointer_kind to > PointerKind::MOUSE then "allowed" will be true at line 1193, even though I > should not be allowed to look for keystrokes. > > EventMatcher::MatchesEvent() will still do the right thing, and won't match the > events, but it would be nice not to have to look at the EventMatcher > implementation to know that things are safe. Done. https://codereview.chromium.org/2125663002/diff/200001/ui/views/mus/window_ma... File ui/views/mus/window_manager_connection.cc (right): https://codereview.chromium.org/2125663002/diff/200001/ui/views/mus/window_ma... ui/views/mus/window_manager_connection.cc:224: // Receives TouchEvent and TouchPointerEvent On 2016/07/12 22:23:11, msw wrote: > nit: add a trailing period; optionally expand or remove comment. Removed. The DCHECK after this comment should be enough https://codereview.chromium.org/2125663002/diff/200001/ui/views/mus/window_ma... File ui/views/mus/window_manager_connection_unittest.cc (right): https://codereview.chromium.org/2125663002/diff/200001/ui/views/mus/window_ma... ui/views/mus/window_manager_connection_unittest.cc:162: ui::MouseEvent mouse_event(kMouseType[i], gfx::Point(0, 0), On 2016/07/12 22:23:11, msw wrote: > nit: use default gfx::Point ctor here and elsewhere. Done. https://codereview.chromium.org/2125663002/diff/200001/ui/views/pointer_watch... File ui/views/pointer_watcher.h (right): https://codereview.chromium.org/2125663002/diff/200001/ui/views/pointer_watch... ui/views/pointer_watcher.h:39: }; On 2016/07/12 22:23:11, msw wrote: > nit: private: DISALLOW_COPY_AND_ASSIGN? Done. https://codereview.chromium.org/2125663002/diff/200001/ui/views/touch_event_w... File ui/views/touch_event_watcher.h (right): https://codereview.chromium.org/2125663002/diff/200001/ui/views/touch_event_w... ui/views/touch_event_watcher.h:34: }; On 2016/07/12 22:23:11, msw wrote: > nit: private: DISALLOW_COPY_AND_ASSIGN? Done.
The CQ bit was checked by riajiang@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: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
PTAL https://codereview.chromium.org/2125663002/diff/160001/ui/views/mus/window_ma... File ui/views/mus/window_manager_connection.cc (right): https://codereview.chromium.org/2125663002/diff/160001/ui/views/mus/window_ma... ui/views/mus/window_manager_connection.cc:225: if (event.IsTouchEvent() || event.IsTouchPointerEvent()) { On 2016/07/12 20:35:47, riajiang wrote: > On 2016/07/12 19:21:05, sadrul wrote: > > DCHECK(IsTouchEvent() || IsTouchPointerEvent()) > > Done. Changed both DCHECKs for (HasTouchEventWatcher()) and (event.IsTouchEvent() || event.IsTouchPointerEvent()) back to ifs. If we remove the watcher just after receiving the event but observer is not updated on the server yet, the first DCHECK would fail. For the second one, we want to just not send any events to watchers when it's a non-supported event (e.g. MouseEvent) instead of failing the DCHECK.
still lgtm https://codereview.chromium.org/2125663002/diff/260001/ui/views/mus/window_ma... File ui/views/mus/window_manager_connection.cc (right): https://codereview.chromium.org/2125663002/diff/260001/ui/views/mus/window_ma... ui/views/mus/window_manager_connection.cc:124: // Last PointerWatcher removed, stop the event observer. *TouchEventWatcher
The CQ bit was checked by riajiang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/2125663002/#ps260001 (title: "Change back to ifs")
The CQ bit was unchecked by riajiang@chromium.org
https://codereview.chromium.org/2125663002/diff/260001/ui/views/mus/window_ma... File ui/views/mus/window_manager_connection.cc (right): https://codereview.chromium.org/2125663002/diff/260001/ui/views/mus/window_ma... ui/views/mus/window_manager_connection.cc:124: // Last PointerWatcher removed, stop the event observer. On 2016/07/13 20:05:50, sadrul wrote: > *TouchEventWatcher Done.
The CQ bit was checked by riajiang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2125663002/#ps280001 (title: "Fix a 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 ========== mus: Add watcher for all touch events. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). BUG=588311 TEST=views_mus_unittests ========== to ========== mus: Add watcher for all touch events. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). BUG=588311 TEST=views_mus_unittests ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== mus: Add watcher for all touch events. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). BUG=588311 TEST=views_mus_unittests ========== to ========== mus: Add watcher for all touch events. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). BUG=588311 TEST=views_mus_unittests Committed: https://crrev.com/aa9394e02706b89d95c3676396ebed708a6a2ea7 Cr-Commit-Position: refs/heads/master@{#405297} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/aa9394e02706b89d95c3676396ebed708a6a2ea7 Cr-Commit-Position: refs/heads/master@{#405297}
Message was sent while issue was closed.
Description was changed from ========== mus: Add watcher for all touch events. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). BUG=588311 TEST=views_mus_unittests Committed: https://crrev.com/aa9394e02706b89d95c3676396ebed708a6a2ea7 Cr-Commit-Position: refs/heads/master@{#405297} ========== to ========== mus: Add watcher for all touch events. BUG=588311 TEST=views_mus_unittests Committed: https://crrev.com/aa9394e02706b89d95c3676396ebed708a6a2ea7 Cr-Commit-Position: refs/heads/master@{#405297} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
