The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/99c145d628465ca7c71f46ae39fe875466e3fb5e

commit 99c145d628465ca7c71f46ae39fe875466e3fb5e
Author: estade <estade@chromium.org>
Date: Fri May 05 22:53:04 2017

Fix ARC notification crash.

Make ArcCustomNotificationView::SlideHelper operate on the correct
layer, i.e. that which is sliding out.

This is meant as a temporary hack to address the crash, but the code
still needs to be restructured to reduce fragility and improve test
coverage.

BUG=718965

Review-Url: https://codereview.chromium.org/2863163002
Cr-Commit-Position: refs/heads/master@{#469787}

[modify] https://crrev.com/99c145d628465ca7c71f46ae39fe875466e3fb5e/ui/arc/notification/arc_custom_notification_view.cc

Issue 719612 has been merged into this issue.

bug 718956 has been approved for a merge but it needs this to be merged first.

+Grace, see #5.

Your change meets the bar and is auto-approved for M59. Please go ahead and merge the CL to branch 3071 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8f75642cf2ecc597f87a8a66aa227ec79e947db1

commit 8f75642cf2ecc597f87a8a66aa227ec79e947db1
Author: Evan Stade <estade@chromium.org>
Date: Fri May 19 15:22:15 2017

Fix ARC notification crash.

Make ArcCustomNotificationView::SlideHelper operate on the correct
layer, i.e. that which is sliding out.

This is meant as a temporary hack to address the crash, but the code
still needs to be restructured to reduce fragility and improve test
coverage.

BUG=718965

Review-Url: https://codereview.chromium.org/2863163002
Cr-Original-Commit-Position: refs/heads/master@{#469787}
Review-Url: https://codereview.chromium.org/2886283006 .
Cr-Commit-Position: refs/branch-heads/3071@{#632}
Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641}

[modify] https://crrev.com/8f75642cf2ecc597f87a8a66aa227ec79e947db1/ui/arc/notification/arc_custom_notification_view.cc

the crash fix is merged, but the original idea of this bug (refactoring SlideOutHelper) still needs addressing, and I believe Eliot mentioned he was going to look into that.

I've had a look into this - it seems like there are a few options. If anyone has any comments that would be useful:

1. Use NotificationSurface's Window's layer in SlideOutController
 - Shadows are still drawn on CustomNotificationView's layer
 - Need to draw both and apply the transform to both (2), or move shadow drawing to ArcCustomNotificationView (3)

2. Animate multiple layers in SlideOutController, leaving the shadow drawing in CustomNotificationView.
 - If the notification is updated while sliding, then NativeViewHost::Layout() changes the position of the aura window based on CustomNotificationView's layer transform. This means that we can't animate NotificationSurface's window's layer - it will also take on its parent window's transform, but only sometimes. If we only animated CustomNotificationView's layer, we would need to call ArcCustomNotificationView::Layout every time the layer transform changed.

3. Draw the shadow in ArcCustomNotificationView
 - Oshima-san suggested using wm::SetShadowElevation, but the shadow ends up being clipped by ClippingWindow. 
 - I haven't been able to find a good way to get ClippingWindow to be a different size to what it contains.
 - Still need to animate ArcCustomNotificationView's layer (as well as NotificationSurface's window's layer)

4. Use BoundsAnimator in SlideOutController, which directly calls SetBounds
 - Still need to animate opacity on the layers, and SlideOutController will need a BoundsAnimator.

5. Temporarily reparent the NotificationSurface layer to ArcCustomNotificationView
 - Aura expects the layer hierarchy to match the window hierarchy, so it's a bit dodgy
 
6. Make ArcCustomNotificationView not a child view of CustomNotificationView, and merge them
 - Would fix the underlying issue (will still have to animate multiple layers though), but seems like a big refactor, assuming we don't use virtual multiple inheritance with NativeViewHost and MessageView.

All these options seem like a lot of work, more hacky than SlideHelper, or both.

> more hacky than SlideHelper

Anything that doesn't make assumptions about the hierarchy of views/layers in a different file and fail silently when that other file changes is less hacky than SlideHelper.

For #1, isn't the CustomNotificationView a child of the NotificationSurface window's layer? I would have thought that sliding the parent layer would also move the child.

Sorry for going silent on this.

In the mean time there were a few changes: CustomNotificationView moved to ui/arc/notification and is now called ArcNotificationView. ArcCustomNotificationView is now called ArcNotificationContentView. 

My understanding is that:
In the View hierarchy, ArcNotificationContentView is a child of ArcNotificationView. ArcNotificationView is a child of MessageListView.

The parent window of NotificationSurface's window is ClippingWindow, which is parented to the Aura window for the message center, and the layer hierarchy follows this.

So we have three layers: ArcNotificationView's (where the shadow is drawn), ArcNotificationContentView's (where the snapshot is drawn), and NotficationSurface's (with the notification content). NotificationSurface's layer is like a sibling to the others, so it's hard to get it to move in step with them. In the options I listed above I tried temporarily re-parenting the layer to be a child of ArcNotificationContentView, which worked. However, I heard from Oshima-san that it's a bit dodgy to change the layer hierarchy to be different to the window hierarchy.

Reassigning to Yoshiki-san for now since I haven't had cycles to work on notifications code recently.