Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1739203003:
Add OverlayPanelEventFilter and tests (Closed)
DescriptionAdd OverlayPanelEventFilter
This change introduces the OverlayPanelEventFilter; a single event
filter for any of the actions that can occur on an OverlayPanel. This
change will be used once OverlayPanels are SceneOverlays.
Tests added here: https://codereview.chromium.org/1808653006
BUG=584340
Committed: https://crrev.com/64419fbf062488b08fab71b71f9205245395a882
Cr-Commit-Position: refs/heads/master@{#381556}
Patch Set 1 #
Total comments: 16
Patch Set 2 : address comments #
Total comments: 8
Patch Set 3 : don't propagate event #Patch Set 4 : address comments, fix tests #
Total comments: 4
Patch Set 5 : remove null checks #Patch Set 6 : other comments #Patch Set 7 : move tests to separate CL #
Dependent Patchsets: Messages
Total messages: 30 (9 generated)
mdjones@chromium.org changed reviewers: + pedrosimonetti@chromium.org, twellington@chromium.org
Adding overlay event filter and tests. The ContextualSearchEventFilter and ContextualSearchStaticEventFilter will be removed later. PTAL
lgtm % comments https://codereview.chromium.org/1739203003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java (right): https://codereview.chromium.org/1739203003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java:24: * Content View Core via {@link EventFilterHost}. nit: ContentViewCore https://codereview.chromium.org/1739203003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java:324: if (e == null) return; When would e be null? https://codereview.chromium.org/1739203003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java:333: * Propagates the given {@link MotionEvent} to the Content View. Is "Content View" referring to the ContentViewCore or something else? Maybe the class ContentView, which contains the ContentViewCore? Or when we have the opt out promo, maybe "content view" is all of the content - opt out + cvc? https://codereview.chromium.org/1739203003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java:341: // Ignores multitouch events to prevent the Content View from from scrolling. Remove one of the from's from this line https://codereview.chromium.org/1739203003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java:444: if (e1 == null || e2 == null) return false; Same question here - why would e1 or e2 be null? It looks like this is only called from onScroll(), which I would expect to have valid events. https://codereview.chromium.org/1739203003/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelEventFilterTest.java (right): https://codereview.chromium.org/1739203003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelEventFilterTest.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. Great tests! Looks like they cover all the basics and then some :) https://codereview.chromium.org/1739203003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelEventFilterTest.java:327: public void testUnwantedTapDoesNotHappenInContentView() { This seems more like it's testing to make sure an unwanted scroll doesn't happen? Maybe testUnwantedScrollDoesNotHappen.. is more accurate? Maybe not, tap is checked too.
https://codereview.chromium.org/1739203003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java (right): https://codereview.chromium.org/1739203003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java:24: * Content View Core via {@link EventFilterHost}. On 2016/02/26 23:38:39, Theresa Wellington wrote: > nit: ContentViewCore Done. https://codereview.chromium.org/1739203003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java:324: if (e == null) return; On 2016/02/26 23:38:39, Theresa Wellington wrote: > When would e be null? There is an existing problem where infobars can minimize the panel mid-scroll (at least for reader); in that case the event can be null. I'll handle that and remove this check in a later CL. https://codereview.chromium.org/1739203003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java:333: * Propagates the given {@link MotionEvent} to the Content View. On 2016/02/26 23:38:39, Theresa Wellington wrote: > Is "Content View" referring to the ContentViewCore or something else? Maybe the > class ContentView, which contains the ContentViewCore? Or when we have the opt > out promo, maybe "content view" is all of the content - opt out + cvc? This means the content view core, I updated the doc. Until the panel is maximized, no events actually get passed to the content view core. https://codereview.chromium.org/1739203003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java:341: // Ignores multitouch events to prevent the Content View from from scrolling. On 2016/02/26 23:38:39, Theresa Wellington wrote: > Remove one of the from's from this line Done. https://codereview.chromium.org/1739203003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java:444: if (e1 == null || e2 == null) return false; On 2016/02/26 23:38:39, Theresa Wellington wrote: > Same question here - why would e1 or e2 be null? It looks like this is only > called from onScroll(), which I would expect to have valid events. See above comment. https://codereview.chromium.org/1739203003/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelEventFilterTest.java (right): https://codereview.chromium.org/1739203003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelEventFilterTest.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/02/26 23:38:39, Theresa Wellington wrote: > Great tests! Looks like they cover all the basics and then some :) Thanks, unfortunately I only refactored existing tests to work with the new event filter :\ https://codereview.chromium.org/1739203003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelEventFilterTest.java:327: public void testUnwantedTapDoesNotHappenInContentView() { On 2016/02/26 23:38:39, Theresa Wellington wrote: > This seems more like it's testing to make sure an unwanted scroll doesn't > happen? Maybe testUnwantedScrollDoesNotHappen.. is more accurate? Maybe not, tap > is checked too. Scroll is more appropriate.
mdjones@chromium.org changed reviewers: + dtrainor@chromium.org
+dtrainor for owner approval
https://codereview.chromium.org/1739203003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java (right): https://codereview.chromium.org/1739203003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java:324: if (e == null) return; On 2016/02/27 02:32:19, mdjones wrote: > On 2016/02/26 23:38:39, Theresa Wellington wrote: > > When would e be null? > > There is an existing problem where infobars can minimize the panel mid-scroll > (at least for reader); in that case the event can be null. I'll handle that and > remove this check in a later CL. I think leaving the null check is fine too, I was just curious.
https://codereview.chromium.org/1739203003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java (right): https://codereview.chromium.org/1739203003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java:324: if (e == null) return; On 2016/02/27 02:32:19, mdjones wrote: > On 2016/02/26 23:38:39, Theresa Wellington wrote: > > When would e be null? > > There is an existing problem where infobars can minimize the panel mid-scroll > (at least for reader); in that case the event can be null. I'll handle that and > remove this check in a later CL. Not sure if I understand the problem, but I'd prefer not having those checks in place, and instead, having the underlying infobar issue fixed. We need to be very careful when propagating events because messing the with MotionEvent stream (omitting or changing order of events) might cause chrome to crash. https://codereview.chromium.org/1739203003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java (right): https://codereview.chromium.org/1739203003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java:324: if (e == null) return; Is it possible for the event to be null? Why are we including this check here? See comment in previous patch. I'd prefer not having this check here. https://codereview.chromium.org/1739203003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java:384: ContentViewCore cvc = mPanel.getContentViewCore(); Why are we changing this? Shouldn't the mHost propagate the event to the CVC? https://codereview.chromium.org/1739203003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java:444: if (e1 == null || e2 == null) return false; Same question. Is it possible for any of the events to be null? Why are we including this check here?
https://codereview.chromium.org/1739203003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java (right): https://codereview.chromium.org/1739203003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java:384: ContentViewCore cvc = mPanel.getContentViewCore(); On 2016/02/29 19:43:20, pedrosimonetti wrote: > Why are we changing this? Shouldn't the mHost propagate the event to the CVC? The ContentViewCore is no longer part of the underlying layout that mHost would propagate to.
https://codereview.chromium.org/1739203003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java (right): https://codereview.chromium.org/1739203003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java:384: ContentViewCore cvc = mPanel.getContentViewCore(); On 2016/02/29 21:02:51, mdjones wrote: > On 2016/02/29 19:43:20, pedrosimonetti wrote: > > Why are we changing this? Shouldn't the mHost propagate the event to the CVC? > > The ContentViewCore is no longer part of the underlying layout that mHost would > propagate to. Can we remove the mHost then?
https://codereview.chromium.org/1739203003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java (right): https://codereview.chromium.org/1739203003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java:384: ContentViewCore cvc = mPanel.getContentViewCore(); On 2016/02/29 22:16:08, pedrosimonetti wrote: > On 2016/02/29 21:02:51, mdjones wrote: > > On 2016/02/29 19:43:20, pedrosimonetti wrote: > > > Why are we changing this? Shouldn't the mHost propagate the event to the > CVC? > > > > The ContentViewCore is no longer part of the underlying layout that mHost > would > > propagate to. > > Can we remove the mHost then? Sure, I opted to pass the event along, but I guess that doesn't make too much sense.
https://codereview.chromium.org/1739203003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java (right): https://codereview.chromium.org/1739203003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java:384: ContentViewCore cvc = mPanel.getContentViewCore(); On 2016/03/01 00:59:30, mdjones wrote: > On 2016/02/29 22:16:08, pedrosimonetti wrote: > > On 2016/02/29 21:02:51, mdjones wrote: > > > On 2016/02/29 19:43:20, pedrosimonetti wrote: > > > > Why are we changing this? Shouldn't the mHost propagate the event to the > > CVC? > > > > > > The ContentViewCore is no longer part of the underlying layout that mHost > > would > > > propagate to. > > > > Can we remove the mHost then? > > Sure, I opted to pass the event along, but I guess that doesn't make too much > sense. Nit: Shouldn't we also check that cvc.getContainerView() != null? Nit: Since we're doing the exact same thing below, I suggest creating a new private method named propagateEventToContentViewCoreContainer(event) or something similar.
https://codereview.chromium.org/1739203003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java (right): https://codereview.chromium.org/1739203003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java:384: ContentViewCore cvc = mPanel.getContentViewCore(); On 2016/03/01 01:07:52, pedrosimonetti wrote: > On 2016/03/01 00:59:30, mdjones wrote: > > On 2016/02/29 22:16:08, pedrosimonetti wrote: > > > On 2016/02/29 21:02:51, mdjones wrote: > > > > On 2016/02/29 19:43:20, pedrosimonetti wrote: > > > > > Why are we changing this? Shouldn't the mHost propagate the event to the > > > CVC? > > > > > > > > The ContentViewCore is no longer part of the underlying layout that mHost > > > would > > > > propagate to. > > > > > > Can we remove the mHost then? > > > > Sure, I opted to pass the event along, but I guess that doesn't make too much > > sense. > > Nit: Shouldn't we also check that cvc.getContainerView() != null? > > Nit: Since we're doing the exact same thing below, I suggest creating a new > private method named propagateEventToContentViewCoreContainer(event) or > something similar. Done and structured code differently.
https://codereview.chromium.org/1739203003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java (right): https://codereview.chromium.org/1739203003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java:325: if (e == null) return; You haven't commented about my previous comment about null MotionEvents. Is it possible to address this before landing this change? https://codereview.chromium.org/1739203003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java:396: if (!wasEventCanceled) { Nit: Combine two if blocks?
https://codereview.chromium.org/1739203003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java (right): https://codereview.chromium.org/1739203003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java:325: if (e == null) return; On 2016/03/01 19:13:55, pedrosimonetti wrote: > You haven't commented about my previous comment about null MotionEvents. Is it > possible to address this before landing this change? Removed null checks. The case that causes this is handled in: https://codereview.chromium.org/1785073002/ https://codereview.chromium.org/1739203003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/eventfilter/OverlayPanelEventFilter.java:396: if (!wasEventCanceled) { On 2016/03/01 19:13:55, pedrosimonetti wrote: > Nit: Combine two if blocks? Done.
lgtm Thanks for making the changes!
lgtm
The CQ bit was checked by mdjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from twellington@chromium.org Link to the patchset: https://codereview.chromium.org/1739203003/#ps100001 (title: "other comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1739203003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1739203003/100001
Description was changed from ========== Add OverlayPanelEventFilter and tests This change introduces the OverlayPanelEventFilter; a single event filter for any of the actions that can occur on an OverlayPanel. This change will be used once OverlayPanels are SceneOverlays. BUG=584340 ========== to ========== Add OverlayPanelEventFilter This change introduces the OverlayPanelEventFilter; a single event filter for any of the actions that can occur on an OverlayPanel. This change will be used once OverlayPanels are SceneOverlays. Tests added here: https://codereview.chromium.org/1808653006 BUG=584340 ==========
Proguard removes this class because it is not used yet. See https://codereview.chromium.org/1808653006/ for tests.
The CQ bit was checked by mdjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from twellington@chromium.org, dtrainor@chromium.org, pedrosimonetti@chromium.org Link to the patchset: https://codereview.chromium.org/1739203003/#ps120001 (title: "move tests to separate CL")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1739203003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1739203003/120001
Message was sent while issue was closed.
Description was changed from ========== Add OverlayPanelEventFilter This change introduces the OverlayPanelEventFilter; a single event filter for any of the actions that can occur on an OverlayPanel. This change will be used once OverlayPanels are SceneOverlays. Tests added here: https://codereview.chromium.org/1808653006 BUG=584340 ========== to ========== Add OverlayPanelEventFilter This change introduces the OverlayPanelEventFilter; a single event filter for any of the actions that can occur on an OverlayPanel. This change will be used once OverlayPanels are SceneOverlays. Tests added here: https://codereview.chromium.org/1808653006 BUG=584340 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add OverlayPanelEventFilter This change introduces the OverlayPanelEventFilter; a single event filter for any of the actions that can occur on an OverlayPanel. This change will be used once OverlayPanels are SceneOverlays. Tests added here: https://codereview.chromium.org/1808653006 BUG=584340 ========== to ========== Add OverlayPanelEventFilter This change introduces the OverlayPanelEventFilter; a single event filter for any of the actions that can occur on an OverlayPanel. This change will be used once OverlayPanels are SceneOverlays. Tests added here: https://codereview.chromium.org/1808653006 BUG=584340 Committed: https://crrev.com/64419fbf062488b08fab71b71f9205245395a882 Cr-Commit-Position: refs/heads/master@{#381556} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/64419fbf062488b08fab71b71f9205245395a882 Cr-Commit-Position: refs/heads/master@{#381556} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
