chrome/android: Push EventFilters into the Layout implementations.
EventFilters are currently built by the LayoutManager and all gestures
are routed through it to the active layout. This indirection is
unnecessary since only the StackLayout requires gesture handling. This
also ends up exposing public APIs on the Layouts making it difficult to
follow the path for events from different sources (CompositorView vs
the Toolbar).
This change pushes EventFilters deeper into the Layout implementations,
so the Layouts directly consume touch events and generate gestures if
necessary. Layout still has an API for swipe gestures coming from the
toolbar.
BUG=
Review-Url: https://codereview.chromium.org/2774443002
Cr-Original-Commit-Position: refs/heads/master@{#459980}
Committed: https://chromium.googlesource.com/chromium/src/+/ef25f0df9d626028752de7d945d89abe15d39dfb
Review-Url: https://codereview.chromium.org/2774443002
Cr-Commit-Position: refs/heads/master@{#460525}
Committed: https://chromium.googlesource.com/chromium/src/+/2d3c8ab9800e2eebc6a379a78f5afba38bd4f640
Description was changed from ========== chrome/android: Push EventFilters into the Layout implementations. BUG= ========== to ...
3 years, 9 months ago
(2017-03-22 21:19:16 UTC)
#1
Description was changed from
==========
chrome/android: Push EventFilters into the Layout implementations.
BUG=
==========
to
==========
chrome/android: Push EventFilters into the Layout implementations.
EventFilters are currently built by the LayoutManager and all gestures
are routed through it to the active layout. This indirection is
unnecessary since only the StackLayout requires gesture handling. This
also ends up exposing public APIs on the Layouts making it difficult to
follow the path for events from different sources (CompositorView vs
the Toolbar).
This change pushes EventFilters deeper into the Layout implementations,
so the Layouts directly consume touch events and generate gestures if
necessary. Layout still has an API for swipe gestures coming from the
toolbar.
BUG=
==========
Thanks for the clean up! This mostly looks good, just wanted to discuss a few ...
3 years, 9 months ago
(2017-03-23 17:18:55 UTC)
#4
Thanks for the clean up! This mostly looks good, just wanted to discuss a few
things.
https://codereview.chromium.org/2774443002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java
(left):
https://codereview.chromium.org/2774443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java:1132:
public boolean isTabStripEventFilterEnabled() {
On 2017/03/22 21:20:09, Khushal wrote:
> This would always return true, so not necessary.
Also this had no place in this class and apparently was never overridden.
https://codereview.chromium.org/2774443002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java
(right):
https://codereview.chromium.org/2774443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java:102:
protected EventFilter mEventFilter;
On 2017/03/22 21:20:09, Khushal wrote:
> I was thinking about just removing this and adding a getEventFilter() which
lets
> the sub-class provide the EventFilter. May be not worth it.
I think this is a pretty good idea; it makes the event filter feel more like a
requirement rather than an implementation detail. I guess it depends on how you
look at it though. It seems like each layout should be responsible for its own.
David Trainor- moved to gerrit
lgtm, but just mmake sure the expected behavior still works for cross-layout interaction and events. ...
3 years, 9 months ago
(2017-03-24 17:06:43 UTC)
#5
lgtm, but just mmake sure the expected behavior still works for cross-layout
interaction and events.
https://codereview.chromium.org/2774443002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java
(right):
https://codereview.chromium.org/2774443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java:102:
protected EventFilter mEventFilter;
On 2017/03/23 17:18:55, mdjones wrote:
> On 2017/03/22 21:20:09, Khushal wrote:
> > I was thinking about just removing this and adding a getEventFilter() which
> lets
> > the sub-class provide the EventFilter. May be not worth it.
>
> I think this is a pretty good idea; it makes the event filter feel more like a
> requirement rather than an implementation detail. I guess it depends on how
you
> look at it though. It seems like each layout should be responsible for its
own.
getEventFilter makes sense if you want the subclass to create and own it. It's
a cleaner API :). You might want to allow them to return null though (e.g.
default behavior could be pass through or 'black hole').
Khushal
https://codereview.chromium.org/2774443002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java File chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java (right): https://codereview.chromium.org/2774443002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java#newcode102 chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java:102: protected EventFilter mEventFilter; On 2017/03/24 17:06:43, David Trainor-ping if ...
3 years, 9 months ago
(2017-03-24 22:33:09 UTC)
#6
https://codereview.chromium.org/2774443002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java
(right):
https://codereview.chromium.org/2774443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java:102:
protected EventFilter mEventFilter;
On 2017/03/24 17:06:43, David Trainor-ping if over 24h wrote:
> On 2017/03/23 17:18:55, mdjones wrote:
> > On 2017/03/22 21:20:09, Khushal wrote:
> > > I was thinking about just removing this and adding a getEventFilter()
which
> > lets
> > > the sub-class provide the EventFilter. May be not worth it.
> >
> > I think this is a pretty good idea; it makes the event filter feel more like
a
> > requirement rather than an implementation detail. I guess it depends on how
> you
> > look at it though. It seems like each layout should be responsible for its
> own.
>
> getEventFilter makes sense if you want the subclass to create and own it.
It's
> a cleaner API :). You might want to allow them to return null though (e.g.
> default behavior could be pass through or 'black hole').
Done. StaticLayout returns null because it only takes gestures from the toolbar.
Otherwise it passes through to the content. The rest that want to eat up all
events do the BlackHole one.
Khushal
The CQ bit was checked by khushalsagar@chromium.org to run a CQ dry run
3 years, 9 months ago
(2017-03-28 00:37:11 UTC)
#7
CQ is committing da patch. Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490664751038580, "parent_rev": "9d8f7b92656f41598fe2ce4e7cc1979fc339ebc4", "commit_rev": "206c6385481346cca814314e62b7084df82f9989"}
3 years, 9 months ago
(2017-03-28 01:54:58 UTC)
#14
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490664751038580,
"parent_rev": "9d8f7b92656f41598fe2ce4e7cc1979fc339ebc4", "commit_rev":
"206c6385481346cca814314e62b7084df82f9989"}
commit-bot: I haz the power
CQ is committing da patch. Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490664751038580, "parent_rev": "b9ee54948a13bebf39f76a3cac615aa986beb3c4", "commit_rev": "ef25f0df9d626028752de7d945d89abe15d39dfb"}
3 years, 9 months ago
(2017-03-28 01:59:49 UTC)
#15
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490664751038580,
"parent_rev": "b9ee54948a13bebf39f76a3cac615aa986beb3c4", "commit_rev":
"ef25f0df9d626028752de7d945d89abe15d39dfb"}
commit-bot: I haz the power
Description was changed from ========== chrome/android: Push EventFilters into the Layout implementations. EventFilters are currently ...
3 years, 9 months ago
(2017-03-28 02:00:40 UTC)
#16
Message was sent while issue was closed.
Description was changed from
==========
chrome/android: Push EventFilters into the Layout implementations.
EventFilters are currently built by the LayoutManager and all gestures
are routed through it to the active layout. This indirection is
unnecessary since only the StackLayout requires gesture handling. This
also ends up exposing public APIs on the Layouts making it difficult to
follow the path for events from different sources (CompositorView vs
the Toolbar).
This change pushes EventFilters deeper into the Layout implementations,
so the Layouts directly consume touch events and generate gestures if
necessary. Layout still has an API for swipe gestures coming from the
toolbar.
BUG=
==========
to
==========
chrome/android: Push EventFilters into the Layout implementations.
EventFilters are currently built by the LayoutManager and all gestures
are routed through it to the active layout. This indirection is
unnecessary since only the StackLayout requires gesture handling. This
also ends up exposing public APIs on the Layouts making it difficult to
follow the path for events from different sources (CompositorView vs
the Toolbar).
This change pushes EventFilters deeper into the Layout implementations,
so the Layouts directly consume touch events and generate gestures if
necessary. Layout still has an API for swipe gestures coming from the
toolbar.
BUG=
Review-Url: https://codereview.chromium.org/2774443002
Cr-Commit-Position: refs/heads/master@{#459980}
Committed:
https://chromium.googlesource.com/chromium/src/+/ef25f0df9d626028752de7d945d8...
==========
commit-bot: I haz the power
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/ef25f0df9d626028752de7d945d89abe15d39dfb
3 years, 9 months ago
(2017-03-28 02:00:41 UTC)
#17
Description was changed from ========== chrome/android: Push EventFilters into the Layout implementations. EventFilters are currently ...
3 years, 8 months ago
(2017-03-29 03:08:57 UTC)
#19
Message was sent while issue was closed.
Description was changed from
==========
chrome/android: Push EventFilters into the Layout implementations.
EventFilters are currently built by the LayoutManager and all gestures
are routed through it to the active layout. This indirection is
unnecessary since only the StackLayout requires gesture handling. This
also ends up exposing public APIs on the Layouts making it difficult to
follow the path for events from different sources (CompositorView vs
the Toolbar).
This change pushes EventFilters deeper into the Layout implementations,
so the Layouts directly consume touch events and generate gestures if
necessary. Layout still has an API for swipe gestures coming from the
toolbar.
BUG=
Review-Url: https://codereview.chromium.org/2774443002
Cr-Commit-Position: refs/heads/master@{#459980}
Committed:
https://chromium.googlesource.com/chromium/src/+/ef25f0df9d626028752de7d945d8...
==========
to
==========
chrome/android: Push EventFilters into the Layout implementations.
EventFilters are currently built by the LayoutManager and all gestures
are routed through it to the active layout. This indirection is
unnecessary since only the StackLayout requires gesture handling. This
also ends up exposing public APIs on the Layouts making it difficult to
follow the path for events from different sources (CompositorView vs
the Toolbar).
This change pushes EventFilters deeper into the Layout implementations,
so the Layouts directly consume touch events and generate gestures if
necessary. Layout still has an API for swipe gestures coming from the
toolbar.
BUG=
Review-Url: https://codereview.chromium.org/2774443002
Cr-Commit-Position: refs/heads/master@{#459980}
Committed:
https://chromium.googlesource.com/chromium/src/+/ef25f0df9d626028752de7d945d8...
==========
Khushal
I had missed passing the setting so the AreaGestureEventFilter for the tab strip wouldn't auto-offset ...
3 years, 8 months ago
(2017-03-29 03:15:12 UTC)
#20
I had missed passing the setting so the AreaGestureEventFilter for the tab strip
wouldn't auto-offset touch events. Fixed for landing this again. PTAL.
mdjones
lgtm
3 years, 8 months ago
(2017-03-29 17:18:37 UTC)
#21
lgtm
Khushal
The CQ bit was checked by khushalsagar@chromium.org
3 years, 8 months ago
(2017-03-29 18:44:12 UTC)
#22
CQ is committing da patch. Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490813052766970, "parent_rev": "f8e03c0c5adc572d5e1f51e4093cecb0e661f05e", "commit_rev": "2d3c8ab9800e2eebc6a379a78f5afba38bd4f640"}
3 years, 8 months ago
(2017-03-29 20:39:56 UTC)
#25
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490813052766970,
"parent_rev": "f8e03c0c5adc572d5e1f51e4093cecb0e661f05e", "commit_rev":
"2d3c8ab9800e2eebc6a379a78f5afba38bd4f640"}
commit-bot: I haz the power
Description was changed from ========== chrome/android: Push EventFilters into the Layout implementations. EventFilters are currently ...
3 years, 8 months ago
(2017-03-29 20:40:44 UTC)
#26
Message was sent while issue was closed.
Description was changed from
==========
chrome/android: Push EventFilters into the Layout implementations.
EventFilters are currently built by the LayoutManager and all gestures
are routed through it to the active layout. This indirection is
unnecessary since only the StackLayout requires gesture handling. This
also ends up exposing public APIs on the Layouts making it difficult to
follow the path for events from different sources (CompositorView vs
the Toolbar).
This change pushes EventFilters deeper into the Layout implementations,
so the Layouts directly consume touch events and generate gestures if
necessary. Layout still has an API for swipe gestures coming from the
toolbar.
BUG=
Review-Url: https://codereview.chromium.org/2774443002
Cr-Commit-Position: refs/heads/master@{#459980}
Committed:
https://chromium.googlesource.com/chromium/src/+/ef25f0df9d626028752de7d945d8...
==========
to
==========
chrome/android: Push EventFilters into the Layout implementations.
EventFilters are currently built by the LayoutManager and all gestures
are routed through it to the active layout. This indirection is
unnecessary since only the StackLayout requires gesture handling. This
also ends up exposing public APIs on the Layouts making it difficult to
follow the path for events from different sources (CompositorView vs
the Toolbar).
This change pushes EventFilters deeper into the Layout implementations,
so the Layouts directly consume touch events and generate gestures if
necessary. Layout still has an API for swipe gestures coming from the
toolbar.
BUG=
Review-Url: https://codereview.chromium.org/2774443002
Cr-Original-Commit-Position: refs/heads/master@{#459980}
Committed:
https://chromium.googlesource.com/chromium/src/+/ef25f0df9d626028752de7d945d8...
Review-Url: https://codereview.chromium.org/2774443002
Cr-Commit-Position: refs/heads/master@{#460525}
Committed:
https://chromium.googlesource.com/chromium/src/+/2d3c8ab9800e2eebc6a379a78f5a...
==========
commit-bot: I haz the power
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/2d3c8ab9800e2eebc6a379a78f5afba38bd4f640
3 years, 8 months ago
(2017-03-29 20:40:45 UTC)
#27
Issue 2774443002: chrome/android: Push EventFilters into the Layout implementations.
(Closed)
Created 3 years, 9 months ago by Khushal
Modified 3 years, 8 months ago
Reviewers: David Trainor- moved to gerrit, mdjones
Base URL:
Comments: 6