Clean up the RWHVMac to ui::Compositor interface (phase 1 of 2)

Clean up the RWHVMac to ui::Compositor interface

Move the "ui::Compositor's relationship to RWHVMac" state machine
into BrowserCompositorViewMac.

Clarify the state by renaming states from
  Active / Suspended / Destroyed
to
  HasAttachedCompositor / HasDetachedCompositor / HasNoCompositor

Change the thumbnailer's copy requests to be intercepted by the
BrowserCompositorViewMac, and thereby have the copy request
keep the ui::Compositor attached (even if the RenderWidgetHostImpl
is hidden).

BUG=623716
Committed: https://crrev.com/9e8481c2e211439771d0634043d2addf9fe7b4ec
Cr-Commit-Position: refs/heads/master@{#402541}

[ccameron, 2016-06-27 22:18:10]

Description was changed from

==========
Clean up the RWHVMac to ui::Compositor interface (phase 1 of 2)

BUG=
==========

to

==========
Clean up the RWHVMac to ui::Compositor interface (phase 1 of 2)

Move the "ui::Compositor's relationship to RWHVMac" state machine
into BrowserCompositorViewMac.

Clarify the state by renaming states from
  Active / Suspended / Destroyed
to
  HasAttachedCompositor / HasDetachedCompositor / HasNoCompositor

Change the thumbnailer's copy requests to be intercepted by the
BrowserCompositorViewMac, and thereby have the copy request
keep the ui::Compositor attached (even if the RenderWidgetHostImpl
is hidden).

This is phase 1 of 2 because we now have a strange relationship
where the BrowserCompositorViewMac the DelegatedFrameHost,
but passes its owner, the RenderWidgetHostViewMac in as the
DelegatedFrameHost. The next phase will clean up this part of
the interface.

BUG=623716
==========

[ccameron, 2016-06-27 22:18:10]

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

[ccameron, 2016-06-27 22:35:59]

LMK if you'd prefer me to clean up the DelegatedFrameHost interface in this
patch as well (I'm working on it now, but vaguely felt it should land
separately).

[ccameron, 2016-06-27 23:49:28]

Description was changed from

==========
Clean up the RWHVMac to ui::Compositor interface (phase 1 of 2)

Move the "ui::Compositor's relationship to RWHVMac" state machine
into BrowserCompositorViewMac.

Clarify the state by renaming states from
  Active / Suspended / Destroyed
to
  HasAttachedCompositor / HasDetachedCompositor / HasNoCompositor

Change the thumbnailer's copy requests to be intercepted by the
BrowserCompositorViewMac, and thereby have the copy request
keep the ui::Compositor attached (even if the RenderWidgetHostImpl
is hidden).

This is phase 1 of 2 because we now have a strange relationship
where the BrowserCompositorViewMac the DelegatedFrameHost,
but passes its owner, the RenderWidgetHostViewMac in as the
DelegatedFrameHost. The next phase will clean up this part of
the interface.

BUG=623716
==========

to

==========
Clean up the RWHVMac to ui::Compositor interface

Move the "ui::Compositor's relationship to RWHVMac" state machine
into BrowserCompositorViewMac.

Clarify the state by renaming states from
  Active / Suspended / Destroyed
to
  HasAttachedCompositor / HasDetachedCompositor / HasNoCompositor

Change the thumbnailer's copy requests to be intercepted by the
BrowserCompositorViewMac, and thereby have the copy request
keep the ui::Compositor attached (even if the RenderWidgetHostImpl
is hidden).

BUG=623716
==========

[ccameron, 2016-06-27 23:50:49]

On 2016/06/27 22:35:59, ccameron wrote:
> LMK if you'd prefer me to clean up the DelegatedFrameHost interface in this
> patch as well (I'm working on it now, but vaguely felt it should land
> separately).

Actually, I'm not convinced that BrowserCompositorViewMac should be the
DelegatedFrameHostClient, so I've removed the references to "phase 2". I'd be
curious as to your opinion about that (perhaps more should be merged in, perhaps
less).

[enne (OOO), 2016-06-28 00:25:38]

This is so much cleaner compared to the previous mess of unoccluding for
thumbnails inside of hide.  Thank you!

https://codereview.chromium.org/2093113003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/browser_compositor_view_mac.mm (right):

https://codereview.chromium.org/2093113003/diff/60001/content/browser/rendere...
content/browser/renderer_host/browser_compositor_view_mac.mm:271:
SetRenderWidgetHostIsHidden(render_widget_host_is_hidden);
This is really being called just for UpdateState and not for the "set hidden"
part, right? Shouldn't the set is hidden part already be done?

[ccameron, 2016-06-28 07:04:11]

Thanks!

https://codereview.chromium.org/2093113003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/browser_compositor_view_mac.mm (right):

https://codereview.chromium.org/2093113003/diff/60001/content/browser/rendere...
content/browser/renderer_host/browser_compositor_view_mac.mm:271:
SetRenderWidgetHostIsHidden(render_widget_host_is_hidden);
On 2016/06/28 00:25:37, enne wrote:
> This is really being called just for UpdateState and not for the "set hidden"
> part, right? Shouldn't the set is hidden part already be done?

This is intended as a "oh no, somehow we didn't get the hidden state correct,
let's get it correct at the last minute" thing.

And that's terrible, but it's the existing behavior. I'd like to continue with
this hack in place in place, and then remove it in a more surgical patch.

The next patch will do one of
A. just remove this entirely (and just DCHECK that the hidden state is what we
expect it to be)
B. find the place where it is not the RWHVMac that sets the RWHImpl is_hidden,
and subscribe to the appropriate notification so that we can keep these in sync
C. make DelegatedFrameHostIsVisible return a value from BrowserCompositorViewMac
instead of reaching all the way to RWHImpl

I suspect that either A or C will be the right thing to do, but I'd rather leave
the hack in for this patch.

[enne (OOO), 2016-06-28 17:11:41]

Ok, I think I still don't understand why the hidden state would not be correct
there in particular.  Doing it in swap buffers seems on par with running a timer
that clobbers the hidden state.

That said, I understand the fear of changing this code and breaking things,
having broken a lot here myself.  I'm ok with landing this patch as-is and
following up with one of those solutions.  I like A > C > B, but we can see what
works.

lgtm

[ccameron, 2016-06-28 17:27:36]

Thanks!

On 2016/06/28 17:11:41, enne wrote:
> Ok, I think I still don't understand why the hidden state would not be correct
> there in particular.  Doing it in swap buffers seems on par with running a
timer
> that clobbers the hidden state.

Yes, this is an accurate characterization :(

> That said, I understand the fear of changing this code and breaking things,
> having broken a lot here myself.  I'm ok with landing this patch as-is and
> following up with one of those solutions.  I like A > C > B, but we can see
what
> works.
> 
> lgtm

[ccameron, 2016-06-28 17:28:02]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2016-06-28 17:28:46]

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-06-28 17:41:07]

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

[commit-bot: I haz the power, 2016-06-28 17:41:08]

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

[ccameron, 2016-06-28 17:49:39]

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

[ccameron, 2016-06-28 17:49:39]

Adding thakis@ for chrome/.../thumbnails OWNER

https://codereview.chromium.org/2093113003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/browser_compositor_view_mac.mm (right):

https://codereview.chromium.org/2093113003/diff/60001/content/browser/rendere...
content/browser/renderer_host/browser_compositor_view_mac.mm:271:
SetRenderWidgetHostIsHidden(render_widget_host_is_hidden);
Moved this "set" to the RWHVMac to be more clear that it's syncing the state.

[Nico, 2016-06-28 18:14:54]

rs-lgtm

[ccameron, 2016-06-28 19:19:15]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2016-06-28 19:21:05]

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-06-28 21:05:45]

Description was changed from

==========
Clean up the RWHVMac to ui::Compositor interface

Move the "ui::Compositor's relationship to RWHVMac" state machine
into BrowserCompositorViewMac.

Clarify the state by renaming states from
  Active / Suspended / Destroyed
to
  HasAttachedCompositor / HasDetachedCompositor / HasNoCompositor

Change the thumbnailer's copy requests to be intercepted by the
BrowserCompositorViewMac, and thereby have the copy request
keep the ui::Compositor attached (even if the RenderWidgetHostImpl
is hidden).

BUG=623716
==========

to

==========
Clean up the RWHVMac to ui::Compositor interface

Move the "ui::Compositor's relationship to RWHVMac" state machine
into BrowserCompositorViewMac.

Clarify the state by renaming states from
  Active / Suspended / Destroyed
to
  HasAttachedCompositor / HasDetachedCompositor / HasNoCompositor

Change the thumbnailer's copy requests to be intercepted by the
BrowserCompositorViewMac, and thereby have the copy request
keep the ui::Compositor attached (even if the RenderWidgetHostImpl
is hidden).

BUG=623716
==========

[commit-bot: I haz the power, 2016-06-28 21:05:46]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-28 21:08:56]

Description was changed from

==========
Clean up the RWHVMac to ui::Compositor interface

Move the "ui::Compositor's relationship to RWHVMac" state machine
into BrowserCompositorViewMac.

Clarify the state by renaming states from
  Active / Suspended / Destroyed
to
  HasAttachedCompositor / HasDetachedCompositor / HasNoCompositor

Change the thumbnailer's copy requests to be intercepted by the
BrowserCompositorViewMac, and thereby have the copy request
keep the ui::Compositor attached (even if the RenderWidgetHostImpl
is hidden).

BUG=623716
==========

to

==========
Clean up the RWHVMac to ui::Compositor interface

Move the "ui::Compositor's relationship to RWHVMac" state machine
into BrowserCompositorViewMac.

Clarify the state by renaming states from
  Active / Suspended / Destroyed
to
  HasAttachedCompositor / HasDetachedCompositor / HasNoCompositor

Change the thumbnailer's copy requests to be intercepted by the
BrowserCompositorViewMac, and thereby have the copy request
keep the ui::Compositor attached (even if the RenderWidgetHostImpl
is hidden).

BUG=623716

Committed: https://crrev.com/9e8481c2e211439771d0634043d2addf9fe7b4ec
Cr-Commit-Position: refs/heads/master@{#402541}
==========

[commit-bot: I haz the power, 2016-06-28 21:08:57]

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