Made the CompositorAnimationObserver class usable

Made the CompositorAnimationObserver class usable

If you use CompositorAnimationObserver, you must check
compositor's shutdown. Otherwise there is a crash when
you unsubscribe because of compositor's delete.
There is workaround: to inheritance from
CompositorObserver and listen for OnCompositingShuttingDown.
But this is not graceful solution.
So I added OnCompositingShuttingDown to CompositorAnimationObserver.
I fixed potentional crash and wrote a test.

R=ajuma, danakj

Committed: https://crrev.com/adf2ce86bc9c62d6a192fdfb1e8af246ec01c32d
Cr-Commit-Position: refs/heads/master@{#332480}

[ajuma, 2015-05-29 17:35:53]

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

[ajuma, 2015-05-29 17:42:23]

+sadrul who wrote CompositorAnimationObserver

Is there more context for this change? If this is a crash you've actually
observed, please also file a bug and add the number here. Also, does
AccessibilityFocusRingController need a similar fix?

[Lof, 2015-05-29 20:10:19]

On 2015/05/29 17:42:23, ajuma wrote:
> +sadrul who wrote CompositorAnimationObserver
> 
> Is there more context for this change? If this is a crash you've actually
> observed, please also file a bug and add the number here. Also, does
> AccessibilityFocusRingController need a similar fix?

We try to use CompositorAnimationObserver in Yandex.Browser and figure out that
we must check compositor's shutdown. Otherwise there is a crash when we
unsubscribe because of compositor's delete.
There is workaround: to inheritance from CompositorObserver and listen for
OnCompositingShuttingDown. But this is not graceful solution.
There is no crash in LayerAnimatorCollection, only because Compositor owns
LayerAnimatorCollection and is deleted after LayerAnimatorCollection.
It looks like AccessibilityFocusRingController has the same problem.

[ajuma, 2015-05-29 21:57:10]

On 2015/05/29 20:10:19, Lof wrote:
> On 2015/05/29 17:42:23, ajuma wrote:
> > +sadrul who wrote CompositorAnimationObserver
> > 
> > Is there more context for this change? If this is a crash you've actually
> > observed, please also file a bug and add the number here. Also, does
> > AccessibilityFocusRingController need a similar fix?
> 
> We try to use CompositorAnimationObserver in Yandex.Browser and figure out
that
> we must check compositor's shutdown. Otherwise there is a crash when we
> unsubscribe because of compositor's delete.
> There is workaround: to inheritance from CompositorObserver and listen for
> OnCompositingShuttingDown. But this is not graceful solution.
> There is no crash in LayerAnimatorCollection, only because Compositor owns
> LayerAnimatorCollection and is deleted after LayerAnimatorCollection.

Thanks, please add some of this context to the description.
 
> It looks like AccessibilityFocusRingController has the same problem.

Since you'll need to at least define
AccessibilityFocusRingController::OnCompositingShuttingDown so that the chromeos
build doesn't break, please add the same fix there too.

https://codereview.chromium.org/1147323003/diff/1/ui/compositor/layer_animato...
File ui/compositor/layer_animator_collection.cc (right):

https://codereview.chromium.org/1147323003/diff/1/ui/compositor/layer_animato...
ui/compositor/layer_animator_collection.cc:62: if (compositor_ &&
compositor_->HasAnimationObserver(this)) {
Is HasAnimationObserver necessary here? The call to RemoveAnimationObserver
below winds up in ObserverListBase::RemoveObserver, which doesn't assume the
observe is in the list.

[Lof, 2015-05-30 22:36:19]

On 2015/05/29 21:57:10, ajuma wrote:
> ...

All is fixed.

[ajuma, 2015-06-01 13:25:37]

On 2015/05/30 22:36:19, Lof wrote:
> On 2015/05/29 21:57:10, ajuma wrote:
> > ...
> 
> All is fixed.

Thanks! lgtm.

[Lof, 2015-06-01 13:48:02]

lof84@yandex-team.ru changed reviewers:
+ xiyuan@chromium.org

[Lof, 2015-06-01 13:48:03]

xiyuan@chromium.org: Please review changes in

[xiyuan, 2015-06-01 18:06:35]

accessibility_focus_ring_controller.* LGTM

https://codereview.chromium.org/1147323003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/ui/accessibility_focus_ring_controller.cc (right):

https://codereview.chromium.org/1147323003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/ui/accessibility_focus_ring_controller.cc:326:
ui::Compositor* compositor) {
nit: DCHECK_EQ(compositor_, compositor);

[Lof, 2015-06-02 08:00:32]

On 2015/06/01 18:06:35, xiyuan wrote:
> nit: DCHECK_EQ(compositor_, compositor);
Fixed.
Dana?

[danakj, 2015-06-02 18:24:16]

https://codereview.chromium.org/1147323003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/ui/accessibility_focus_ring_controller.cc (right):

https://codereview.chromium.org/1147323003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/ui/accessibility_focus_ring_controller.cc:328: if
(compositor_) {
this cant be null can it?

https://codereview.chromium.org/1147323003/diff/60001/ui/compositor/layer_ani...
File ui/compositor/layer_animator_collection.cc (right):

https://codereview.chromium.org/1147323003/diff/60001/ui/compositor/layer_ani...
ui/compositor/layer_animator_collection.cc:62: if (compositor_) {
how can compositor_ be null here?

https://codereview.chromium.org/1147323003/diff/60001/ui/compositor/layer_uni...
File ui/compositor/layer_unittest.cc (right):

https://codereview.chromium.org/1147323003/diff/60001/ui/compositor/layer_uni...
ui/compositor/layer_unittest.cc:385: if (compositor_)
how can it be null?

[Lof, 2015-06-02 19:44:50]

You are right. Fixed.

[danakj, 2015-06-02 19:52:06]

LGTM

[Lof, 2015-06-02 20:36:41]

The CQ bit was checked by lof84@yandex-team.ru

[Lof, 2015-06-02 20:36:42]

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

[commit-bot: I haz the power, 2015-06-02 20:36:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1147323003/80001

[commit-bot: I haz the power, 2015-06-02 22:23:43]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-06-02 22:24:42]

Patchset 5 (id:??) landed as
https://crrev.com/adf2ce86bc9c62d6a192fdfb1e8af246ec01c32d
Cr-Commit-Position: refs/heads/master@{#332480}