Fix stale compositor references from ui::Layer

Fix stale compositor references from ui::Layer

This is needed to fix some issues in tests exposed by the WebKit compositor. The
problem is that sometimes during the teardown paths, some layers have stale a
pointer to the compositor that has already been destroyed. The WebKit layer
(rightfully) calls ScheduleDraw when the layer hierarchy is changed, exposing
the issue.

We now always walk back to the root layer to find the compositor, and reset the
pointer when the compositor's root changes (or it gets destroyed).

BUG=99524
TEST=views_unittest, aura_unittest, compositor_unittests with (and without) use_webkit_compositor=1


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110223

[sky, 2011-11-15 04:57:04]

Is it possible to add tests for the case you're fixing?

  -Scott

http://codereview.chromium.org/8510076/diff/1/ui/gfx/compositor/layer.cc
File ui/gfx/compositor/layer.cc (right):

http://codereview.chromium.org/8510076/diff/1/ui/gfx/compositor/layer.cc#newc...
ui/gfx/compositor/layer.cc:82: return compositor_ ? compositor_ : parent_ ?
parent_->GetCompositor() : NULL;
Should this be updated to GetRoot(layer)->compositor_ ?

http://codereview.chromium.org/8510076/diff/1/ui/gfx/compositor/layer.h
File ui/gfx/compositor/layer.h (left):

http://codereview.chromium.org/8510076/diff/1/ui/gfx/compositor/layer.h#oldco...
ui/gfx/compositor/layer.h:156: const Compositor* compositor() const { return
compositor_; }
Can these be removed? Seems error prone to use this instead of GetCompositor.

http://codereview.chromium.org/8510076/diff/1/ui/gfx/compositor/layer_unittes...
File ui/gfx/compositor/layer_unittest.cc (right):

http://codereview.chromium.org/8510076/diff/1/ui/gfx/compositor/layer_unittes...
ui/gfx/compositor/layer_unittest.cc:32: ColoredLayer(SkColor color)
explicit

[jonathan.backer, 2011-11-15 13:22:46]

SGTM

As an aside, are we finally in a place where we can have one compositor? For the
longest time, we had a compositor for each GtkWindow (e.g. pop-up menus and
tooltips each had one). Setting a breakpoint on the compositor factory suggests
that we only have one compositor for aura. Unfortunately, it's being hit twice
on touchui running on the views_desktop (but this will switch Real Soon Now).

Can we imagine use cases where we would want more than one compositor?

I only ask because if there can be only one, there may be room for API
simplification...

[sky, 2011-11-15 16:41:38]

On Tue, Nov 15, 2011 at 5:22 AM,  <backer@chromium.org> wrote:
> SGTM
>
> As an aside, are we finally in a place where we can have one compositor? For
> the
> longest time, we had a compositor for each GtkWindow (e.g. pop-up menus and
> tooltips each had one). Setting a breakpoint on the compositor factory
> suggests
> that we only have one compositor for aura. Unfortunately, it's being hit
> twice
> on touchui running on the views_desktop (but this will switch Real Soon
> Now).
>
> Can we imagine use cases where we would want more than one compositor?

If we wanted compositing at the window level on a traditional desktop
we would want a compositor per window.

  -Scott

[commit-bot: I haz the power, 2011-11-15 17:13:12]

No LGTM from valid reviewers yet.

[piman, 2011-11-15 22:06:33]

The bugs I'm fixing here were actually already found by existing tests, but I
added another explicit one.

http://codereview.chromium.org/8510076/diff/1/ui/gfx/compositor/layer.cc
File ui/gfx/compositor/layer.cc (right):

http://codereview.chromium.org/8510076/diff/1/ui/gfx/compositor/layer.cc#newc...
ui/gfx/compositor/layer.cc:82: return compositor_ ? compositor_ : parent_ ?
parent_->GetCompositor() : NULL;
On 2011/11/15 04:57:04, sky wrote:
> Should this be updated to GetRoot(layer)->compositor_ ?

Originally I didn't want to do that because the compositor might in theory use a
sub-tree as the root (e.g. if we want to screen-shot a window, we may want to do
that). But it doesn't do that today, and we'd need multiple compositors on the
same tree to be able to do that which is far off, so I changed it to that and
added DCHECKs to ensure it's the case.

http://codereview.chromium.org/8510076/diff/1/ui/gfx/compositor/layer.h
File ui/gfx/compositor/layer.h (left):

http://codereview.chromium.org/8510076/diff/1/ui/gfx/compositor/layer.h#oldco...
ui/gfx/compositor/layer.h:156: const Compositor* compositor() const { return
compositor_; }
On 2011/11/15 04:57:04, sky wrote:
> Can these be removed? Seems error prone to use this instead of GetCompositor.

Good idea. Done.

http://codereview.chromium.org/8510076/diff/1/ui/gfx/compositor/layer_unittes...
File ui/gfx/compositor/layer_unittest.cc (right):

http://codereview.chromium.org/8510076/diff/1/ui/gfx/compositor/layer_unittes...
ui/gfx/compositor/layer_unittest.cc:32: ColoredLayer(SkColor color)
On 2011/11/15 04:57:04, sky wrote:
> explicit

Done.

[sky, 2011-11-15 22:08:08]

LGTM

[commit-bot: I haz the power, 2011-11-15 22:15:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/8510076/5001

[commit-bot: I haz the power, 2011-11-15 22:38:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/8510076/4004

[commit-bot: I haz the power, 2011-11-15 23:22:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/8510076/6003

[commit-bot: I haz the power, 2011-11-16 00:51:57]

Change committed as 110223