[Notifications] Fix swipe to close for ARC notifications.

[Notifications] Fix swipe to close for ARC notifications.

Change ArcCustomNotificationView::EventForwarder to not directly invoke On*Event methods of ArcCustomNotificationView (which then call up to the parent). Instead, re-dispatch them to the containing widget, which should handle the event propagation, letting SlideOutController get events as it should. A consequence of this is that I had to add a delegate method to SlideOutController to update ArcCustomNotificationView::SlideHelper. This is because ArcCustomNotificationView will no longer get the gesture events, since it is no longer being called directly from EventForwarder.

BUG=719407
Review-Url: https://codereview.chromium.org/2873553002
Cr-Commit-Position: refs/heads/master@{#473094}
Committed: https://chromium.googlesource.com/chromium/src/+/870290f623b47e436d231e354c84b8723bb7000a

[Eliot Courtney, 2017-05-09 04:16:21]

The CQ bit was checked by edcourtney@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-09 04:17:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eliot Courtney, 2017-05-09 04:17:48]

edcourtney@chromium.org changed reviewers:
+ estade@chromium.org, yhanada@chromium.org, yoshiki@chromium.org

[Eliot Courtney, 2017-05-09 04:17:49]

PTAL, thanks~!

[yoshiki, 2017-05-09 05:45:23]

LGTM

https://codereview.chromium.org/2873553002/diff/1/ui/message_center/views/sli...
File ui/message_center/views/slide_out_controller.h (right):

https://codereview.chromium.org/2873553002/diff/1/ui/message_center/views/sli...
ui/message_center/views/slide_out_controller.h:18: class SlideOutController :
public ui::EventHandler,
nit: we do no longer need to inherit EventHandler.

[commit-bot: I haz the power, 2017-05-09 05:47:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-09 05:47:34]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2017-05-09 14:47:14]

https://codereview.chromium.org/2873553002/diff/1/ui/message_center/views/sli...
File ui/message_center/views/slide_out_controller.h (left):

https://codereview.chromium.org/2873553002/diff/1/ui/message_center/views/sli...
ui/message_center/views/slide_out_controller.h:10: #include
"ui/events/scoped_target_handler.h"
nit: this include isn't needed any more.

Also the target parameter to the constructor is not needed. But I feel like a
better fix would be that EventForwarder doesn't call OnEvent directly but should
be using the proper EventHandler (i.e. the one we set).

[Evan Stade, 2017-05-09 15:03:21]

estade@chromium.org changed reviewers:
+ sky@chromium.org

[Evan Stade, 2017-05-09 15:03:21]

+sky for his input as this is not really how I designed slide out controller to
work. Future clients should not have to explicitly forward gesture events, and I
wouldn't think it's necessary here either as mentioned above.

[sky, 2017-05-09 17:30:49]

On 2017/05/09 15:03:21, Evan Stade wrote:
> +sky for his input as this is not really how I designed slide out controller
to
> work. Future clients should not have to explicitly forward gesture events, and
I
> wouldn't think it's necessary here either as mentioned above.

Yoshiki, could you elaborate on how events are wired up for arc and why? My
initial reaction on a cursory look at  ArcCustomNotificationView::EventForwarder
is that you shouldn't need to forward events. If the EventHandlers are wired up
correctly what is EventForwarder should only handle events it cares about and
shouldn't have to explicitly forward events.

[yoshiki, 2017-05-11 00:47:17]

On 2017/05/09 17:30:49, sky wrote:
> On 2017/05/09 15:03:21, Evan Stade wrote:
> > +sky for his input as this is not really how I designed slide out controller
> to
> > work. Future clients should not have to explicitly forward gesture events,
and
> I
> > wouldn't think it's necessary here either as mentioned above.
> 
> Yoshiki, could you elaborate on how events are wired up for arc and why? My
> initial reaction on a cursory look at 
ArcCustomNotificationView::EventForwarder
> is that you shouldn't need to forward events. If the EventHandlers are wired
up
> correctly what is EventForwarder should only handle events it cares about and
> shouldn't have to explicitly forward events.

Ok, let me investigate.

[Eliot Courtney, 2017-05-16 07:24:23]

The CQ bit was checked by edcourtney@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 07:24:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eliot Courtney, 2017-05-16 07:34:15]

Description was changed from

==========
[Notifications] Fix swipe to close for ARC notifications.

SlideOutController uses a ScopedTargetHandler to get gesture
events. But, events for ARC notifications are forwarded to
ArcCustomNotificationView in ArcCustomNotificationView::EventForwarder,
which does not re-dispatch the event. So, ScopedTargetHandler never gets
the events. Remove ScopedTargetHandler and re-add tests for
CustomNotificationView.

BUG=719407
==========

to

==========
[Notifications] Fix swipe to close for ARC notifications.

Change ArcCustomNotificationView::EventForwarder to not directly invoke On*Event
methods of ArcCustomNotificationView (which then call up to the parent).
Instead, re-dispatch them to the containing widget, which should handle the
event propagation, letting SlideOutController get events as it should. A
consequence of this is that I had to add a delegate method to SlideOutController
to update ArcCustomNotificationView::SlideHelper. This is because
ArcCustomNotificationView will no longer get the gesture events, since it is no
longer being called directly from EventForwarder.

BUG=719407
==========

[Eliot Courtney, 2017-05-16 07:34:15]

edcourtney@chromium.org changed reviewers:
+ xiyuan@chromium.org

[yoshiki, 2017-05-16 08:09:12]

https://codereview.chromium.org/2873553002/diff/40001/ui/arc/notification/arc...
File ui/arc/notification/arc_custom_notification_view.cc (right):

https://codereview.chromium.org/2873553002/diff/40001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view.cc:94:
widget->OnKeyEvent(event->AsKeyEvent());
Is this necessary? I think key events are passed to view accordingly in
CustomWindowDelegate even if we don't do it here.

see. https://cs.chromium.org/chromium/src/components/exo/surface.cc?l=125

If it's actually necessary, please add a brief comment.

[Eliot Courtney, 2017-05-16 09:15:09]

The CQ bit was checked by edcourtney@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 09:15:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eliot Courtney, 2017-05-16 09:15:54]

I talked with Oshima-san and Yoshiki-san, who both helped me a lot. We think it
might be good to eventually have an EventTargeter to decide which events should
go to the message center, and which should go to exosphere. But since this is
for M59, this was the least invasive way to fix it we could think of.

As for SlideHelper, we could investigate adding a delegate to return the correct
slide layer for ArcCustomNotificationView and then we may be able to get rid of
SlideHelper.

https://codereview.chromium.org/2873553002/diff/40001/ui/arc/notification/arc...
File ui/arc/notification/arc_custom_notification_view.cc (right):

https://codereview.chromium.org/2873553002/diff/40001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view.cc:94:
widget->OnKeyEvent(event->AsKeyEvent());
On 2017/05/16 08:09:12, yoshiki wrote:
> Is this necessary? I think key events are passed to view accordingly in
> CustomWindowDelegate even if we don't do it here.
> 
> see. https://cs.chromium.org/chromium/src/components/exo/surface.cc?l=125
> 
> If it's actually necessary, please add a brief comment.

Good catch - yes this is not necessary, and ends up sending key events twice.
Done.

[commit-bot: I haz the power, 2017-05-16 10:05:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-16 10:05:07]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-05-16 16:27:58]

lgtm

Another perspective that we could consider (for longer term) is to figure out a
way to make NotificationSurface a layer so that it could be added as child layer
of ArcCustomNotificationView instead of being an aura::Window and embedded via
NativeViewHost. We still need to figure out a way to handle events properly but
SlideHelper could be gone completely.

Current layer tree:

MessageCenter
+ ArcCustomNotificationView
+ NativeViewHost clipping window
  + NotificationSurface

NotificationSurface is sort of a sibling of ArcCustomNotificationView thus the
sliding translation does not apply to it and we need SlideHelper. And events
would be dispatched NotificationSurface aura::Window and needs to be forwarded
back to MessageCenter before sending to exo (exo handle all events).

If we can make it a layer, we could have
MessageCenter
+ ArcCustomNotificationView
  + NotificationSurface

NotificationSurface layer as a child of ArcCustomNotificationView. Then slide
translation should automatically be applied and no need to have SlideHelper. The
tricky part is to figure out how to dispatch events properly so that exo still
gets it and sends them to Android.

https://codereview.chromium.org/2873553002/diff/60001/ui/arc/notification/arc...
File ui/arc/notification/arc_custom_notification_view.cc (right):

https://codereview.chromium.org/2873553002/diff/60001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view.cc:93:
widget->OnGestureEvent(located_event->AsGestureEvent());
nit: wrap with {} for all branches

https://codereview.chromium.org/2873553002/diff/60001/ui/message_center/views...
File ui/message_center/views/custom_notification_view.h (right):

https://codereview.chromium.org/2873553002/diff/60001/ui/message_center/views...
ui/message_center/views/custom_notification_view.h:42: //
views::SlideOutController::Delegate:
nit: insert an empty line

[Evan Stade, 2017-05-16 21:58:15]

https://codereview.chromium.org/2873553002/diff/60001/ui/message_center/views...
File ui/message_center/views/slide_out_controller.cc (right):

https://codereview.chromium.org/2873553002/diff/60001/ui/message_center/views...
ui/message_center/views/slide_out_controller.cc:75: delegate_->OnSlideChanged();
Can the interested party (ArcCustomNotificationView) observe animations on the
slide out layer via LayerAnimationObserver?

[oshima, 2017-05-17 01:49:22]

On 2017/05/16 16:27:58, xiyuan wrote:
> lgtm
> 
> Another perspective that we could consider (for longer term) is to figure out
a
> way to make NotificationSurface a layer so that it could be added as child
layer
> of ArcCustomNotificationView instead of being an aura::Window and embedded via
> NativeViewHost. We still need to figure out a way to handle events properly
but
> SlideHelper could be gone completely.
> 
> Current layer tree:
> 
> MessageCenter
> + ArcCustomNotificationView
> + NativeViewHost clipping window
>   + NotificationSurface
> 
> NotificationSurface is sort of a sibling of ArcCustomNotificationView thus the
> sliding translation does not apply to it and we need SlideHelper. And events
> would be dispatched NotificationSurface aura::Window and needs to be forwarded
> back to MessageCenter before sending to exo (exo handle all events).
> 
> If we can make it a layer, we could have
> MessageCenter
> + ArcCustomNotificationView
>   + NotificationSurface

(I assume you meant MessageView by MessageCenter)
ArcCustonNotificationView is a NativeviewHost, so this is roughly how it looks
from layer point of view.

One of issues was that MessageView creates a layer for animation, assuming that
the content is
drawn onto it, which isn't true for ARC notification because it has its own
window (and layer).
As a result, it's animating the layer that has no content.

Looks like SlideHelper was added to workaround by creating a copy and add it to
the
layer of MessageView?

Instead, we can simply animate the layer of NativeViewHost (hosting one), i
think.
 
> NotificationSurface layer as a child of ArcCustomNotificationView. Then slide
> translation should automatically be applied and no need to have SlideHelper.
The
> tricky part is to figure out how to dispatch events properly so that exo still
> gets it and sends them to Android.
> 
>
https://codereview.chromium.org/2873553002/diff/60001/ui/arc/notification/arc...
> File ui/arc/notification/arc_custom_notification_view.cc (right):
> 
>
https://codereview.chromium.org/2873553002/diff/60001/ui/arc/notification/arc...
> ui/arc/notification/arc_custom_notification_view.cc:93:
> widget->OnGestureEvent(located_event->AsGestureEvent());
> nit: wrap with {} for all branches
> 
>
https://codereview.chromium.org/2873553002/diff/60001/ui/message_center/views...
> File ui/message_center/views/custom_notification_view.h (right):
> 
>
https://codereview.chromium.org/2873553002/diff/60001/ui/message_center/views...
> ui/message_center/views/custom_notification_view.h:42: //
> views::SlideOutController::Delegate:
> nit: insert an empty line

[Eliot Courtney, 2017-05-17 04:48:25]

Thanks everyone for their advice~! I'm going to try out, for now, getting
SlideOutController to slide out the correct layer for ArcCustomNotificationView,
and then see if we can remove SlideHelper based on that.

https://codereview.chromium.org/2873553002/diff/60001/ui/arc/notification/arc...
File ui/arc/notification/arc_custom_notification_view.cc (right):

https://codereview.chromium.org/2873553002/diff/60001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view.cc:93:
widget->OnGestureEvent(located_event->AsGestureEvent());
On 2017/05/16 16:27:58, xiyuan wrote:
> nit: wrap with {} for all branches

Done.

https://codereview.chromium.org/2873553002/diff/60001/ui/message_center/views...
File ui/message_center/views/custom_notification_view.h (right):

https://codereview.chromium.org/2873553002/diff/60001/ui/message_center/views...
ui/message_center/views/custom_notification_view.h:42: //
views::SlideOutController::Delegate:
On 2017/05/16 16:27:58, xiyuan wrote:
> nit: insert an empty line

Done.

https://codereview.chromium.org/2873553002/diff/60001/ui/message_center/views...
File ui/message_center/views/slide_out_controller.cc (right):

https://codereview.chromium.org/2873553002/diff/60001/ui/message_center/views...
ui/message_center/views/slide_out_controller.cc:75: delegate_->OnSlideChanged();
On 2017/05/16 21:58:14, Evan Stade wrote:
> Can the interested party (ArcCustomNotificationView) observe animations on the
> slide out layer via LayerAnimationObserver?

I tried this (by adding an override for OnLayerAnimationStarted), but it seems
to me like the animation is only for when the notification is slid out to close
(SlideOutAndClose), or restored back to its original position
(RestoreVisualState). ArcCustomNotificationView still needs to know about the
'animation' during the gesture.

[Eliot Courtney, 2017-05-19 04:22:00]

The CQ bit was checked by edcourtney@chromium.org

[Eliot Courtney, 2017-05-19 04:22:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
yoshiki@chromium.org, xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/2873553002/#ps80001
(title: "Address comments.")

[commit-bot: I haz the power, 2017-05-19 04:22:39]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 05:15:16]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495167720457160,
"parent_rev": "a30d68388644568f7930aa57cdd2fa015816aeb9", "commit_rev":
"870290f623b47e436d231e354c84b8723bb7000a"}

[commit-bot: I haz the power, 2017-05-19 05:15:29]

Description was changed from

==========
[Notifications] Fix swipe to close for ARC notifications.

Change ArcCustomNotificationView::EventForwarder to not directly invoke On*Event
methods of ArcCustomNotificationView (which then call up to the parent).
Instead, re-dispatch them to the containing widget, which should handle the
event propagation, letting SlideOutController get events as it should. A
consequence of this is that I had to add a delegate method to SlideOutController
to update ArcCustomNotificationView::SlideHelper. This is because
ArcCustomNotificationView will no longer get the gesture events, since it is no
longer being called directly from EventForwarder.

BUG=719407
==========

to

==========
[Notifications] Fix swipe to close for ARC notifications.

Change ArcCustomNotificationView::EventForwarder to not directly invoke On*Event
methods of ArcCustomNotificationView (which then call up to the parent).
Instead, re-dispatch them to the containing widget, which should handle the
event propagation, letting SlideOutController get events as it should. A
consequence of this is that I had to add a delegate method to SlideOutController
to update ArcCustomNotificationView::SlideHelper. This is because
ArcCustomNotificationView will no longer get the gesture events, since it is no
longer being called directly from EventForwarder.

BUG=719407

Review-Url: https://codereview.chromium.org/2873553002
Cr-Commit-Position: refs/heads/master@{#473094}
Committed:
https://chromium.googlesource.com/chromium/src/+/870290f623b47e436d231e354c84...
==========

[commit-bot: I haz the power, 2017-05-19 05:15:32]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/870290f623b47e436d231e354c84...