Explicitly specify the ui::Compositor to DelegatedFrameHost

Explicitly specify the ui::Compositor to DelegatedFrameHost

When DelegatedFrameHost was split off from RenderWidgetHostViewAura,
I had DelegatedFrameHost access the ui::Compositor via a call into the
client. This is harder to reason about than it should be, and leaves open
the possibility of keeping the DelegatedFrameHost as an observer of a
dead ui::Compositor.

Explicitly detach the DelegatedFrameHost from the ui::Compositor
when the ui::Compositor is being destroyed. Add a check to the
observer list, to ensure that it is empty at destruction.

BUG=(paying down technical debt)

Committed: https://crrev.com/92bcf3157b47fd6dffc3fafffd6227d87d4b7f99
Cr-Commit-Position: refs/heads/master@{#312926}

[ccameron, 2015-01-22 01:53:39]

ccameron@chromium.org changed reviewers:
+ jbauman@chromium.org

[ccameron, 2015-01-22 01:53:40]

ptal -- small change.

I'm planning some other changes to the DelegatedFrameHost, but I want to do them
separately because they will be entirely cosmetic (this shouldn't change
behavior, but might if there are bugs).

[jbauman, 2015-01-22 02:11:20]

lgtm

https://codereview.chromium.org/859423002/diff/40001/content/browser/composit...
File content/browser/compositor/delegated_frame_host.cc (right):

https://codereview.chromium.org/859423002/diff/40001/content/browser/composit...
content/browser/compositor/delegated_frame_host.cc:82: compositor_(NULL),
The style guide says to use nullptr.

https://codereview.chromium.org/859423002/diff/40001/content/browser/composit...
content/browser/compositor/delegated_frame_host.cc:1000: compositor_ = NULL;
Same.

[ccameron, 2015-01-22 23:33:13]

Thanks!

[ccameron, 2015-01-22 23:33:30]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2015-01-22 23:35:28]

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

[commit-bot: I haz the power, 2015-01-23 01:34:24]

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

[commit-bot: I haz the power, 2015-01-23 01:34:25]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[ccameron, 2015-01-23 03:03:05]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2015-01-23 03:04:02]

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

[commit-bot: I haz the power, 2015-01-23 05:29:58]

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

[commit-bot: I haz the power, 2015-01-23 05:29:59]

Try jobs failed on following builders:
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ccameron, 2015-01-23 11:33:13]

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

[ccameron, 2015-01-23 11:34:24]

Adding sky for ui/aura stuff

This indeed uncovered an issue where we were leaving dead observers attached to
the a compositor and just hoping not to use-after-free them. I've cleaned that
up now.

[sky, 2015-01-23 17:32:18]

https://codereview.chromium.org/859423002/diff/120001/ui/aura/window_tree_hos...
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/859423002/diff/120001/ui/aura/window_tree_hos...
ui/aura/window_tree_host.cc:184: FOR_EACH_OBSERVER(WindowTreeHostObserver,
observers_,
Can whoever cares about this notification listen directly to the compositor?

[ccameron, 2015-01-23 18:52:27]

Thanks -- I've changed the patch to use a compositor observer instead.

https://codereview.chromium.org/859423002/diff/120001/ui/aura/window_tree_hos...
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/859423002/diff/120001/ui/aura/window_tree_hos...
ui/aura/window_tree_host.cc:184: FOR_EACH_OBSERVER(WindowTreeHostObserver,
observers_,
On 2015/01/23 17:32:18, sky wrote:
> Can whoever cares about this notification listen directly to the compositor?

Sure (just checked that observers can be removed while traversing).

I still do like this version, but either seems fine to me.

[ccameron, 2015-01-23 19:37:07]

Patchset #8 (id:140001) has been deleted

[ccameron, 2015-01-23 19:37:25]

Patchset #8 (id:160001) has been deleted

[ccameron, 2015-01-23 19:43:11]

Patchset #8 (id:180001) has been deleted

[sky, 2015-01-23 20:27:02]

On 2015/01/23 18:52:27, ccameron wrote:
> Thanks -- I've changed the patch to use a compositor observer instead.
> 
>
https://codereview.chromium.org/859423002/diff/120001/ui/aura/window_tree_hos...
> File ui/aura/window_tree_host.cc (right):
> 
>
https://codereview.chromium.org/859423002/diff/120001/ui/aura/window_tree_hos...
> ui/aura/window_tree_host.cc:184: FOR_EACH_OBSERVER(WindowTreeHostObserver,
> observers_,
> On 2015/01/23 17:32:18, sky wrote:
> > Can whoever cares about this notification listen directly to the compositor?
> 
> Sure (just checked that observers can be removed while traversing).
> 
> I still do like this version, but either seems fine to me.

If someone is installing a listener on an object than the object the listener is
attached to should be able to tell them when the object goes stale. The approach
in your first patch was each containing object exposes a separate listener
interface to tell when the contained object is destroyed. That doesn't scale
particularly well and is why I prefer this approach here. Now if WindowTreeHost
changed it's compositor and you needed to reattacah a listener, then I agree you
would need another listener. But that doesn't seem to be the case here.

aura changes LGTM

[ccameron, 2015-01-23 20:32:39]

On 2015/01/23 20:27:02, sky wrote:
> On 2015/01/23 18:52:27, ccameron wrote:
> > Thanks -- I've changed the patch to use a compositor observer instead.
> > 
> >
>
https://codereview.chromium.org/859423002/diff/120001/ui/aura/window_tree_hos...
> > File ui/aura/window_tree_host.cc (right):
> > 
> >
>
https://codereview.chromium.org/859423002/diff/120001/ui/aura/window_tree_hos...
> > ui/aura/window_tree_host.cc:184: FOR_EACH_OBSERVER(WindowTreeHostObserver,
> > observers_,
> > On 2015/01/23 17:32:18, sky wrote:
> > > Can whoever cares about this notification listen directly to the
compositor?
> > 
> > Sure (just checked that observers can be removed while traversing).
> > 
> > I still do like this version, but either seems fine to me.
> 
> If someone is installing a listener on an object than the object the listener
is
> attached to should be able to tell them when the object goes stale. The
approach
> in your first patch was each containing object exposes a separate listener
> interface to tell when the contained object is destroyed. That doesn't scale
> particularly well and is why I prefer this approach here. Now if
WindowTreeHost
> changed it's compositor and you needed to reattacah a listener, then I agree
you
> would need another listener. But that doesn't seem to be the case here.

Agree.

> aura changes LGTM

Thanks!

[ccameron, 2015-01-23 20:32:44]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2015-01-23 20:33:13]

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

[commit-bot: I haz the power, 2015-01-23 20:44:26]

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

[commit-bot: I haz the power, 2015-01-23 20:44:28]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Alpha Left Google, 2015-01-23 20:55:38]

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

[Alpha Left Google, 2015-01-23 20:55:39]

LGTM.

[ccameron, 2015-01-23 20:56:05]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2015-01-23 20:57:05]

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

[commit-bot: I haz the power, 2015-01-23 20:57:54]

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

[commit-bot: I haz the power, 2015-01-23 20:57:55]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[ccameron, 2015-01-23 21:02:23]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2015-01-23 21:03:02]

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

[commit-bot: I haz the power, 2015-01-23 21:04:54]

Committed patchset #8 (id:200001)

[commit-bot: I haz the power, 2015-01-23 21:06:12]

Patchset 8 (id:??) landed as
https://crrev.com/92bcf3157b47fd6dffc3fafffd6227d87d4b7f99
Cr-Commit-Position: refs/heads/master@{#312926}