arc: Custom notification improvements

arc: Custom notification improvements

- Install a pre target event handler to notification surface window
  to forward events to hosting ArcCustomNotificationView;
- Only show close button when mouse hovering;
- Add SlideHelper to observe slide transform/animation and swap between
  a notification surface copy and real surface when a slide starts/stops;

BUG=635566, 636625
TEST=Manual. Close button only shows up when mouse hovering. Backspace/delete
  key dismisses the custom notification. Swipe gesture works as regular
  Chrome notification. Closing via close button in message list does not crash.

Committed: https://crrev.com/416f1f674c873e3b58963e64a2ef01e52dcfbe5e
Cr-Commit-Position: refs/heads/master@{#411272}

[xiyuan, 2016-08-08 17:00:47]

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

[xiyuan, 2016-08-08 17:00:55]

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

[commit-bot: I haz the power, 2016-08-08 17:01:07]

Dry run: 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, 2016-08-08 18:22:51]

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

[commit-bot: I haz the power, 2016-08-08 18:22:51]

Dry run: This issue passed the CQ dry run.

[yoshiki, 2016-08-09 16:01:57]

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

https://codereview.chromium.org/2221073002/diff/20001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view.cc:56:
owner_->surface_->window()->layer()->SetOpacity(1.0f);
Shouldn't we check if owner_->surface_->window() != null?

https://codereview.chromium.org/2221073002/diff/20001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view.cc:92: surface_copy_.reset();
Shouldn't we remove |surface_copy_| from owner_->layer() before freeing it?

[xiyuan, 2016-08-09 16:08:10]

Thanks for reviewing in such hour. :)

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

https://codereview.chromium.org/2221073002/diff/20001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view.cc:56:
owner_->surface_->window()->layer()->SetOpacity(1.0f);
On 2016/08/09 16:01:57, yoshiki wrote:
> Shouldn't we check if owner_->surface_->window() != null?

Done.

https://codereview.chromium.org/2221073002/diff/20001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view.cc:92: surface_copy_.reset();
On 2016/08/09 16:01:57, yoshiki wrote:
> Shouldn't we remove |surface_copy_| from owner_->layer() before freeing it?

This is okay because Layer removes itself from its parent in dtor [1].

[1]: https://cs.chromium.org/chromium/src/ui/compositor/layer.cc?rcl=0&l=109

[yoshiki, 2016-08-09 16:15:01]

lgtm

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

https://codereview.chromium.org/2221073002/diff/20001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view.cc:92: surface_copy_.reset();
On 2016/08/09 16:08:09, xiyuan wrote:
> On 2016/08/09 16:01:57, yoshiki wrote:
> > Shouldn't we remove |surface_copy_| from owner_->layer() before freeing it?
> 
> This is okay because Layer removes itself from its parent in dtor [1].
> 
> [1]: https://cs.chromium.org/chromium/src/ui/compositor/layer.cc?rcl=0&l=109

Acknowledged.

[xiyuan, 2016-08-09 16:39:06]

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

[xiyuan, 2016-08-09 16:39:06]

sadrul@, please approve adding the following deps for //ui/arc:

    '+ui/compositor',
    '+ui/display',
    '+ui/events',
    '+ui/wm',

Thanks.

[xiyuan, 2016-08-09 23:03:42]

xiyuan@chromium.org changed reviewers:
+ lhchavez@lhchavez.com

[xiyuan, 2016-08-09 23:03:43]

lhchavez@, could you approve //ui/arc/Build.gn change? Thanks.

[Luis Héctor Chávez, 2016-08-09 23:04:50]

lhchavez@chromium.org changed reviewers:
+ lhchavez@chromium.org

[Luis Héctor Chávez, 2016-08-09 23:04:51]

rs-lgtm

[sadrul, 2016-08-10 03:26:39]

I went ahead and reviewed the actual usage of the new DEPS too. I have left some
comments.

I am also curious to understand the differences between 'normal' notifications
vs. arc notifications. Are there some docs to explain this? For example, does
the normal notifications not have these behaviour (e.g. show the close button
only on hover, keyboard dismissal, swipe to dismiss)? If they do, then can you
clarify why arc notifications need a different set of code for doing the same
things?

https://codereview.chromium.org/2221073002/diff/40001/ui/arc/BUILD.gn
File ui/arc/BUILD.gn (right):

https://codereview.chromium.org/2221073002/diff/40001/ui/arc/BUILD.gn#newcode33
ui/arc/BUILD.gn:33: "//ui/compositor:compositor",
Just //ui/compositor

Similarly, just //ui/display, //ui/events, and //ui/wm (also applies to existing
deps, like //ash, //ui/aura, //components/exo)

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

https://codereview.chromium.org/2221073002/diff/40001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view.cc:40: }
Do you only care about these events, or do you want to override just
OnEvent(ui::Event*) instead?

https://codereview.chromium.org/2221073002/diff/40001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view.cc:50:
SlideHelper(ArcCustomNotificationView* owner) : owner_(owner) {
explicit

https://codereview.chromium.org/2221073002/diff/40001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view.cc:176:
surface_->window()->AddPreTargetHandler(event_forwarder_.get());
Can you clarify why we need a pre-target handler for this? For example, could
you set |this| as the surface_->window()'s target_handler instead (without
needing |event_forwarder_|)?

https://codereview.chromium.org/2221073002/diff/40001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view.cc:206: }
Consider using views::MouseWatcher (with MouseWatcherViewHost) instead to detect
when to hide/show the close button.

[xiyuan, 2016-08-10 15:13:16]

On 2016/08/10 03:26:39, sadrul wrote:
> I went ahead and reviewed the actual usage of the new DEPS too. I have left
some
> comments.
> 
> I am also curious to understand the differences between 'normal' notifications
> vs. arc notifications. Are there some docs to explain this? For example, does
> the normal notifications not have these behaviour (e.g. show the close button
> only on hover, keyboard dismissal, swipe to dismiss)? If they do, then can you
> clarify why arc notifications need a different set of code for doing the same
> things?
> 

Before I finish updating the CL, here are some backgrounds for your questions.

I don't have a ddoc to cover all the details. There are two relevant ones:
1. Chrome Custom Notification [1]. That adds support to have a notification
   provides an View as its UI;
2. ARC++ Notification Design Proposal [2]. Part of this ddoc covers how arc
   notification is provided. Check out "Custom Control Support" option b.
Basically,
   we handle an arc notification similar to an arc app window. The UI is
rendered at
   Android side in a surface buffer and sends this surface buffer back to
wayland
   server running in Chrome.

In the implementation, the arc notification surface is managed by
exo::NotificationSurface
and the surface is hosted under an aura::Window's layer tree. This aura::Window
is hosted
by an ArcCustomNotificationView (derived from NativeViewHost), whose parent is a
CustomNotificationView that is shown as part of the message_center. The keyboard
dismisal
and swipe is supposed to be handled by the CustomNotificationView. However,
aura::Window
receives all the events and CustomNotificationView does not see them. Thus we
lose these
standard notification behavior. This CL attempts to bridge the events to
CustomNotificationView
to fix the problem.

For showing closing button only on hover, this is arc notification specific.
Because arc
is M based and has a timestamp label on top right. UX wants to add this behavior
so that
closing button is not always there, overlapping with the timestamp label.


[1]
https://docs.google.com/document/d/1qQY9m4wQI6hYajPWLMfSL3vypCsxGlc_ZXZ2nvXfo...
[2]
https://docs.google.com/document/d/1tMP3T6JX6os2TObyjMKfJEdZUJ4gPpWDHVDFiMPog...

[xiyuan, 2016-08-10 16:28:08]

https://codereview.chromium.org/2221073002/diff/40001/ui/arc/BUILD.gn
File ui/arc/BUILD.gn (right):

https://codereview.chromium.org/2221073002/diff/40001/ui/arc/BUILD.gn#newcode33
ui/arc/BUILD.gn:33: "//ui/compositor:compositor",
On 2016/08/10 03:26:39, sadrul wrote:
> Just //ui/compositor
> 
> Similarly, just //ui/display, //ui/events, and //ui/wm (also applies to
existing
> deps, like //ash, //ui/aura, //components/exo)

Done. Updated all except the //component/arc ones because part of that target
depends on //ui/arc (this one) so have to be explicit.

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

https://codereview.chromium.org/2221073002/diff/40001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view.cc:40: }
On 2016/08/10 03:26:39, sadrul wrote:
> Do you only care about these events, or do you want to override just
> OnEvent(ui::Event*) instead?

OnEvent makes more sense. Replace individual handlers with OnEvent.

https://codereview.chromium.org/2221073002/diff/40001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view.cc:50:
SlideHelper(ArcCustomNotificationView* owner) : owner_(owner) {
On 2016/08/10 03:26:39, sadrul wrote:
> explicit

Done.

https://codereview.chromium.org/2221073002/diff/40001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view.cc:176:
surface_->window()->AddPreTargetHandler(event_forwarder_.get());
On 2016/08/10 03:26:39, sadrul wrote:
> Can you clarify why we need a pre-target handler for this? For example, could
> you set |this| as the surface_->window()'s target_handler instead (without
> needing |event_forwarder_|)?

This is because |surface_| has children aura::Windows and we want to take over
events dispatched to them as well. The surface is mapped from an Android window
that hosts the notification UI. Each layer of that Android window is mapped to
an exo::Surface and that exo::Surface's aura::Window is added as a child to the
surface's window. We want to pre handle the events on those child windows as
well. The exo::Surface is on the way to get rid of aura::Window and just be a
Layer. But before that, we have to use a pre-target handler.

Added a comment for |event_forwarder_| in header to document this.

https://codereview.chromium.org/2221073002/diff/40001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view.cc:206: }
On 2016/08/10 03:26:39, sadrul wrote:
> Consider using views::MouseWatcher (with MouseWatcherViewHost) instead to
detect
> when to hide/show the close button.

Contains() is needed for determining the visibility on close button creation. We
don't save that with MouseWatcher. Mouse enter/exit detection is provided by
aura::Window's mouse event (OnMouseEntered/Exited). Think the current impl is
simpler and more straightforward.

[xiyuan, 2016-08-10 17:16:16]

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

[commit-bot: I haz the power, 2016-08-10 17:16:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Luis Héctor Chávez, 2016-08-10 17:38:17]

lhchavez@chromium.org changed reviewers:
- lhchavez@lhchavez.com

[Luis Héctor Chávez, 2016-08-10 17:38:18]

removing the wrong lhchavez.

[commit-bot: I haz the power, 2016-08-10 19:21:37]

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

[commit-bot: I haz the power, 2016-08-10 19:21:40]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2016-08-11 05:10:33]

Description was changed from

==========
arc: Custom notification improvements

- Install a pre target event handler to notification surface window
  to forward events to hosting ArcCustomNotificationView;
- Only show close button when mouse hovering;
- Add SlideHelper to observe slide transform/animation and swap between
  a notification surface copy and real surface when a slide starts/stops;

BUG=635566
TEST=Manual. Close button only shows up when mouse hovering. Backspace/delete
  key dismisses the custom notification. Swipe gesture works as regular
  Chrome notification.
==========

to

==========
arc: Custom notification improvements

- Install a pre target event handler to notification surface window
  to forward events to hosting ArcCustomNotificationView;
- Only show close button when mouse hovering;
- Add SlideHelper to observe slide transform/animation and swap between
  a notification surface copy and real surface when a slide starts/stops;

BUG=635566,636625
TEST=Manual. Close button only shows up when mouse hovering. Backspace/delete
  key dismisses the custom notification. Swipe gesture works as regular
  Chrome notification.
==========

[xiyuan, 2016-08-11 05:11:04]

Description was changed from

==========
arc: Custom notification improvements

- Install a pre target event handler to notification surface window
  to forward events to hosting ArcCustomNotificationView;
- Only show close button when mouse hovering;
- Add SlideHelper to observe slide transform/animation and swap between
  a notification surface copy and real surface when a slide starts/stops;

BUG=635566,636625
TEST=Manual. Close button only shows up when mouse hovering. Backspace/delete
  key dismisses the custom notification. Swipe gesture works as regular
  Chrome notification.
==========

to

==========
arc: Custom notification improvements

- Install a pre target event handler to notification surface window
  to forward events to hosting ArcCustomNotificationView;
- Only show close button when mouse hovering;
- Add SlideHelper to observe slide transform/animation and swap between
  a notification surface copy and real surface when a slide starts/stops;

BUG=635566,636625
TEST=Manual. Close button only shows up when mouse hovering. Backspace/delete
  key dismisses the custom notification. Swipe gesture works as regular
  Chrome notification. Closing via close button in message list does not crash.
==========

[sadrul, 2016-08-11 05:11:27]

Thanks for the the explanations and links to docs. lgtm

[xiyuan, 2016-08-11 05:17:51]

On 2016/08/11 05:11:27, sadrul wrote:
> Thanks for the the explanations and links to docs. lgtm

Great. Thank you.

[xiyuan, 2016-08-11 05:18:00]

The CQ bit was checked by xiyuan@chromium.org

[xiyuan, 2016-08-11 05:18:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
yoshiki@chromium.org, lhchavez@chromium.org
Link to the patchset: https://codereview.chromium.org/2221073002/#ps60001
(title: "for #3")

[commit-bot: I haz the power, 2016-08-11 05:18:10]

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, 2016-08-11 05:21:26]

Description was changed from

==========
arc: Custom notification improvements

- Install a pre target event handler to notification surface window
  to forward events to hosting ArcCustomNotificationView;
- Only show close button when mouse hovering;
- Add SlideHelper to observe slide transform/animation and swap between
  a notification surface copy and real surface when a slide starts/stops;

BUG=635566,636625
TEST=Manual. Close button only shows up when mouse hovering. Backspace/delete
  key dismisses the custom notification. Swipe gesture works as regular
  Chrome notification. Closing via close button in message list does not crash.
==========

to

==========
arc: Custom notification improvements

- Install a pre target event handler to notification surface window
  to forward events to hosting ArcCustomNotificationView;
- Only show close button when mouse hovering;
- Add SlideHelper to observe slide transform/animation and swap between
  a notification surface copy and real surface when a slide starts/stops;

BUG=635566,636625
TEST=Manual. Close button only shows up when mouse hovering. Backspace/delete
  key dismisses the custom notification. Swipe gesture works as regular
  Chrome notification. Closing via close button in message list does not crash.
==========

[commit-bot: I haz the power, 2016-08-11 05:21:29]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-11 05:23:05]

Description was changed from

==========
arc: Custom notification improvements

- Install a pre target event handler to notification surface window
  to forward events to hosting ArcCustomNotificationView;
- Only show close button when mouse hovering;
- Add SlideHelper to observe slide transform/animation and swap between
  a notification surface copy and real surface when a slide starts/stops;

BUG=635566,636625
TEST=Manual. Close button only shows up when mouse hovering. Backspace/delete
  key dismisses the custom notification. Swipe gesture works as regular
  Chrome notification. Closing via close button in message list does not crash.
==========

to

==========
arc: Custom notification improvements

- Install a pre target event handler to notification surface window
  to forward events to hosting ArcCustomNotificationView;
- Only show close button when mouse hovering;
- Add SlideHelper to observe slide transform/animation and swap between
  a notification surface copy and real surface when a slide starts/stops;

BUG=635566,636625
TEST=Manual. Close button only shows up when mouse hovering. Backspace/delete
  key dismisses the custom notification. Swipe gesture works as regular
  Chrome notification. Closing via close button in message list does not crash.

Committed: https://crrev.com/416f1f674c873e3b58963e64a2ef01e52dcfbe5e
Cr-Commit-Position: refs/heads/master@{#411272}
==========

[commit-bot: I haz the power, 2016-08-11 05:23:07]

Patchset 4 (id:??) landed as
https://crrev.com/416f1f674c873e3b58963e64a2ef01e52dcfbe5e
Cr-Commit-Position: refs/heads/master@{#411272}