Prepare to move BrowserCompositorViewMac to ui

Prepare to move BrowserCompositorViewMac to ui

This cleans up the the interface to BrowserCompositorViewMac to
prepare for moving it closer to ui::views.

BUG=424058
Committed: https://crrev.com/c0e0e0ffa9d5f1c4631084e94dbbcb8f56b862f1
Cr-Commit-Position: refs/heads/master@{#300300}

[ccameron, 2014-10-16 23:13:29]

ccameron@chromium.org changed reviewers:
+ danakj@chromium.org, tapted@chromium.org

[ccameron, 2014-10-16 23:20:35]

The next step here would be to move
* content::BrowserCompositorViewMac
    -> ui::views::NativeViewCocoa
* content::BrowserCompositorViewMacClient
    -> ui::views::NativeViewCocoaClient

Then make ui::ContextFactory have a CreateNativeViewCocoa method, which will
give you a ui::views::NativeViewCocoa for a NSView. The
GpuProcessTransportFactory implementation of this will return a create a
content::BrowserCompositorViewMac, and everything else will stay the same.

Or something to that effect -- figuring out what exactly to name the ui::
interfaces will be tricky.

[danakj, 2014-10-16 23:38:56]

https://codereview.chromium.org/638123003/diff/1/content/browser/compositor/b...
File content/browser/compositor/browser_compositor_view_mac.h (right):

https://codereview.chromium.org/638123003/diff/1/content/browser/compositor/b...
content/browser/compositor/browser_compositor_view_mac.h:42: // TODO(ccameron):
This should implement an interface in the ui namespace.
Can you mark which methods would be that interface?

[ccameron, 2014-10-17 00:02:19]

https://codereview.chromium.org/638123003/diff/1/content/browser/compositor/b...
File content/browser/compositor/browser_compositor_view_mac.h (right):

https://codereview.chromium.org/638123003/diff/1/content/browser/compositor/b...
content/browser/compositor/browser_compositor_view_mac.h:42: // TODO(ccameron):
This should implement an interface in the ui namespace.
On 2014/10/16 23:38:56, danakj wrote:
> Can you mark which methods would be that interface? 

Done.

[tapted, 2014-10-17 00:04:36]

Looks nice - I think I see where it's headed, and will aim to try it out today. 
(and don't wait for my lg). Some thoughts below.

https://codereview.chromium.org/638123003/diff/1/content/browser/compositor/b...
File content/browser/compositor/browser_compositor_view_mac.h (right):

https://codereview.chromium.org/638123003/diff/1/content/browser/compositor/b...
content/browser/compositor/browser_compositor_view_mac.h:8: #include
<Cocoa/Cocoa.h>
ui/gfx/native_widget_types.h (and NSView -> gfx::NativeView) might be an option
if we want to avoid ObjC creeping in here. However, I'm pretty sure the current
client code is all .mm.

https://codereview.chromium.org/638123003/diff/1/content/browser/compositor/b...
content/browser/compositor/browser_compositor_view_mac.h:47: explicit
BrowserCompositorViewMac(
nit: explicit not needed

https://codereview.chromium.org/638123003/diff/1/content/browser/compositor/b...
content/browser/compositor/browser_compositor_view_mac.h:71: NSView*
native_view_;
Thinking if there might be any lifetime issue around these... Perhaps comment in
the constructor like "|client| should own both the BrowserCompositorViewMac(..)
and |native_view|.

https://codereview.chromium.org/638123003/diff/1/content/browser/compositor/b...
content/browser/compositor/browser_compositor_view_mac.h:73:
scoped_ptr<BrowserCompositorCALayerTreeMac> ca_layer_tree_;
is the plan to move this into a subclass of BrowserCompositorViewMac?

https://codereview.chromium.org/638123003/diff/1/content/browser/gpu/gpu_proc...
File content/browser/gpu/gpu_process_host_ui_shim.cc (right):

https://codereview.chromium.org/638123003/diff/1/content/browser/gpu/gpu_proc...
content/browser/gpu/gpu_process_host_ui_shim.cc:44: #endif
I'm probably unnecessarily scared by this :). I'd maybe define a function
outside any class definition. Something like
content/browser/compositor/browser_compositor_frame_dispatch.h with
BrowserCompositorGot{Accelerated,Software}Frame() declared. But if there's
precedent for this, what's here looks OK too.

[ccameron, 2014-10-17 00:26:54]

https://codereview.chromium.org/638123003/diff/1/content/browser/compositor/b...
File content/browser/compositor/browser_compositor_view_mac.h (right):

https://codereview.chromium.org/638123003/diff/1/content/browser/compositor/b...
content/browser/compositor/browser_compositor_view_mac.h:8: #include
<Cocoa/Cocoa.h>
On 2014/10/17 00:04:35, tapted wrote:
> ui/gfx/native_widget_types.h (and NSView -> gfx::NativeView) might be an
option
> if we want to avoid ObjC creeping in here. However, I'm pretty sure the
current
> client code is all .mm.

Actually, the #include turned out not the be necessary at all -- removed.

https://codereview.chromium.org/638123003/diff/1/content/browser/compositor/b...
content/browser/compositor/browser_compositor_view_mac.h:47: explicit
BrowserCompositorViewMac(
On 2014/10/17 00:04:36, tapted wrote:
> nit: explicit not needed

Done.

https://codereview.chromium.org/638123003/diff/1/content/browser/compositor/b...
content/browser/compositor/browser_compositor_view_mac.h:71: NSView*
native_view_;
On 2014/10/17 00:04:36, tapted wrote:
> Thinking if there might be any lifetime issue around these... Perhaps comment
in
> the constructor like "|client| should own both the
BrowserCompositorViewMac(..)
> and |native_view|.

Done -- added a comment to the ctor.

https://codereview.chromium.org/638123003/diff/1/content/browser/compositor/b...
content/browser/compositor/browser_compositor_view_mac.h:73:
scoped_ptr<BrowserCompositorCALayerTreeMac> ca_layer_tree_;
On 2014/10/17 00:04:36, tapted wrote:
> is the plan to move this into a subclass of BrowserCompositorViewMac?

Yes, exactly. I may go with something to the effect of
ui::NativeViewCompositorCocoa and ui::NativeViewCompositorCocoaImpl.

https://codereview.chromium.org/638123003/diff/1/content/browser/gpu/gpu_proc...
File content/browser/gpu/gpu_process_host_ui_shim.cc (right):

https://codereview.chromium.org/638123003/diff/1/content/browser/gpu/gpu_proc...
content/browser/gpu/gpu_process_host_ui_shim.cc:44: #endif
On 2014/10/17 00:04:36, tapted wrote:
> I'm probably unnecessarily scared by this :). I'd maybe define a function
> outside any class definition. Something like
> content/browser/compositor/browser_compositor_frame_dispatch.h with
> BrowserCompositorGot{Accelerated,Software}Frame() declared. But if there's
> precedent for this, what's here looks OK too.

I was wondering which of the horrors was worse. Given that there's precedent for
outside-class functions, I'll go with that.

[danakj, 2014-10-17 00:30:33]

https://codereview.chromium.org/638123003/diff/40001/content/browser/composit...
File content/browser/compositor/browser_compositor_view_mac.h (right):

https://codereview.chromium.org/638123003/diff/40001/content/browser/composit...
content/browser/compositor/browser_compositor_view_mac.h:55: ui::Compositor*
GetCompositor() const;
I wanna make sure we're on the right path and i'm understanding. This interface
is for content to implement and provide functionality to ui:: right? What code
in ui would need to use this to get the compositor?

https://codereview.chromium.org/638123003/diff/40001/content/browser/composit...
content/browser/compositor/browser_compositor_view_mac.h:59: bool
HasFrameOfSize(const gfx::Size& dip_size) const;
Similarly can you give a brief example or handwaving about what would use each
of these from ui::?

[ccameron, 2014-10-17 00:40:14]

The GetCompositor bit is the only one that I'm 100% sure all clients will need
-- the rest they'll probably want in some form, but maybe we can wait until that
bit is implemented before adding it to the interface.

https://codereview.chromium.org/638123003/diff/40001/content/browser/composit...
File content/browser/compositor/browser_compositor_view_mac.h (right):

https://codereview.chromium.org/638123003/diff/40001/content/browser/composit...
content/browser/compositor/browser_compositor_view_mac.h:55: ui::Compositor*
GetCompositor() const;
On 2014/10/17 00:30:33, danakj wrote:
> I wanna make sure we're on the right path and i'm understanding. This
interface
> is for content to implement and provide functionality to ui:: right? What code
> in ui would need to use this to get the compositor?

The NSView that owns this will be calling SetSizeAndScale on the ui::Compositor
(among any other properties that it wants to set).

https://codereview.chromium.org/638123003/diff/40001/content/browser/composit...
content/browser/compositor/browser_compositor_view_mac.h:59: bool
HasFrameOfSize(const gfx::Size& dip_size) const;
On 2014/10/17 00:30:33, danakj wrote:
> Similarly can you give a brief example or handwaving about what would use each
> of these from ui::?

The frame pumping bit is useful for making sure that you get a frame of a given
size up on the screen before the NSView has completed doing a resize. An example
would be:

GetCompositor()->SetSizeAndScale(newFrameSize)
BeginPumpingFrames()
while (!HasFrameOfSize(newFrameSize) && !timed_out)
  // pump run loop
EndPumpingFrames()

Trent's code only uses GetCompositor so far -- I could remove the
pumping/HasFrameOfSize functions for the time being -- I'll suspect he'll want
to use them eventually (although maybe we should leave them out until that need
becomes concrete).

[danakj, 2014-10-17 00:44:14]

https://codereview.chromium.org/638123003/diff/40001/content/browser/composit...
File content/browser/compositor/browser_compositor_view_mac.h (right):

https://codereview.chromium.org/638123003/diff/40001/content/browser/composit...
content/browser/compositor/browser_compositor_view_mac.h:55: ui::Compositor*
GetCompositor() const;
On 2014/10/17 00:40:14, ccameron1 wrote:
> On 2014/10/17 00:30:33, danakj wrote:
> > I wanna make sure we're on the right path and i'm understanding. This
> interface
> > is for content to implement and provide functionality to ui:: right? What
code
> > in ui would need to use this to get the compositor?
> 
> The NSView that owns this will be calling SetSizeAndScale on the
ui::Compositor
> (among any other properties that it wants to set).

OK. The NSView doesn't exist yet right? I guess it would have this thing as it's
only path to the Compositor? I was picturing a view as also having ui::Layers or
views::Widgets or something which also give you the compositor. It seems a bit
funny to have ui:: code ask content:: to give it back the ui::Compositor. But if
it makes sense then it does.

https://codereview.chromium.org/638123003/diff/40001/content/browser/composit...
content/browser/compositor/browser_compositor_view_mac.h:59: bool
HasFrameOfSize(const gfx::Size& dip_size) const;
On 2014/10/17 00:40:13, ccameron1 wrote:
> On 2014/10/17 00:30:33, danakj wrote:
> > Similarly can you give a brief example or handwaving about what would use
each
> > of these from ui::?
> 
> The frame pumping bit is useful for making sure that you get a frame of a
given
> size up on the screen before the NSView has completed doing a resize. An
example
> would be:
> 
> GetCompositor()->SetSizeAndScale(newFrameSize)
> BeginPumpingFrames()
> while (!HasFrameOfSize(newFrameSize) && !timed_out)
>   // pump run loop
> EndPumpingFrames()
> 
> Trent's code only uses GetCompositor so far -- I could remove the
> pumping/HasFrameOfSize functions for the time being -- I'll suspect he'll want
> to use them eventually (although maybe we should leave them out until that
need
> becomes concrete).

OK thanks. Ya I'd add it when we need it.

[ccameron, 2014-10-17 01:06:38]

Thanks!

https://codereview.chromium.org/638123003/diff/40001/content/browser/composit...
File content/browser/compositor/browser_compositor_view_mac.h (right):

https://codereview.chromium.org/638123003/diff/40001/content/browser/composit...
content/browser/compositor/browser_compositor_view_mac.h:55: ui::Compositor*
GetCompositor() const;
On 2014/10/17 00:44:14, danakj wrote:
> On 2014/10/17 00:40:14, ccameron1 wrote:
> > On 2014/10/17 00:30:33, danakj wrote:
> > > I wanna make sure we're on the right path and i'm understanding. This
> > interface
> > > is for content to implement and provide functionality to ui:: right? What
> code
> > > in ui would need to use this to get the compositor?
> > 
> > The NSView that owns this will be calling SetSizeAndScale on the
> ui::Compositor
> > (among any other properties that it wants to set).
> 
> OK. The NSView doesn't exist yet right? I guess it would have this thing as
it's
> only path to the Compositor? I was picturing a view as also having ui::Layers
or
> views::Widgets or something which also give you the compositor. It seems a bit
> funny to have ui:: code ask content:: to give it back the ui::Compositor. But
if
> it makes sense then it does.

Ah -- the NSView exists -- this class then goes in an messes around with the
drawing mechanism (the CALayers hiding behind the NSView) of the NSView when it
gets frames.

WRT the ui::Compositor being given to you by content, that's because of the way
that the ui::Compositors are recycled between tabs -- because we have a separate
NSView for each tab, we need to make sure that we don't alloc/free a
ui::Compositor per-tab.

https://codereview.chromium.org/638123003/diff/40001/content/browser/composit...
content/browser/compositor/browser_compositor_view_mac.h:59: bool
HasFrameOfSize(const gfx::Size& dip_size) const;
On 2014/10/17 00:44:14, danakj wrote:
> On 2014/10/17 00:40:13, ccameron1 wrote:
> > On 2014/10/17 00:30:33, danakj wrote:
> > > Similarly can you give a brief example or handwaving about what would use
> each
> > > of these from ui::?
> > 
> > The frame pumping bit is useful for making sure that you get a frame of a
> given
> > size up on the screen before the NSView has completed doing a resize. An
> example
> > would be:
> > 
> > GetCompositor()->SetSizeAndScale(newFrameSize)
> > BeginPumpingFrames()
> > while (!HasFrameOfSize(newFrameSize) && !timed_out)
> >   // pump run loop
> > EndPumpingFrames()
> > 
> > Trent's code only uses GetCompositor so far -- I could remove the
> > pumping/HasFrameOfSize functions for the time being -- I'll suspect he'll
want
> > to use them eventually (although maybe we should leave them out until that
> need
> > becomes concrete).
> 
> OK thanks. Ya I'd add it when we need it.

K -- removed the TODOs for these guys.

[tapted, 2014-10-17 11:32:09]

lgtm

rebased my epic experiment-patch and tried this out. Everything still works :).

http://crrev.com/659233002 is the rebased patch against master. Plonked a video
at http://go/ares-on-mac if you're curious what this is all about.

http://crrev.com/646263003 is the same thing with this CL incorporated. It's
neat.

https://codereview.chromium.org/638123003/diff/60001/content/browser/composit...
File content/browser/compositor/browser_compositor_ca_layer_tree_mac.h (right):

https://codereview.chromium.org/638123003/diff/60001/content/browser/composit...
content/browser/compositor/browser_compositor_ca_layer_tree_mac.h:116: void
BrowserCompositorCALayerTreeMacGotAcceleratedFrame(
(optional) I'd consider moving these to their own header, to avoid replicating
the declaration in gpu_process_host_ui_shim.cc

[ccameron, 2014-10-17 20:57:55]

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

[ccameron, 2014-10-17 20:57:55]

Awesome -- thanks!

Adding zmo for content/browser/gpu

https://codereview.chromium.org/638123003/diff/60001/content/browser/composit...
File content/browser/compositor/browser_compositor_ca_layer_tree_mac.h (right):

https://codereview.chromium.org/638123003/diff/60001/content/browser/composit...
content/browser/compositor/browser_compositor_ca_layer_tree_mac.h:116: void
BrowserCompositorCALayerTreeMacGotAcceleratedFrame(
On 2014/10/17 11:32:09, tapted wrote:
> (optional) I'd consider moving these to their own header, to avoid replicating
> the declaration in gpu_process_host_ui_shim.cc

I put in some #if defined(__OBJC__) to allow including this file in C++, which
seems to do the trick.

I also considered keeping them in the class and #if defined(__OBJC__)-ing out
parts of the class definition, but that seemed a bit too evil.

[Zhenyao Mo, 2014-10-17 22:12:51]

On 2014/10/17 20:57:55, ccameron1 wrote:
> Awesome -- thanks!
> 
> Adding zmo for content/browser/gpu
> 
>
https://codereview.chromium.org/638123003/diff/60001/content/browser/composit...
> File content/browser/compositor/browser_compositor_ca_layer_tree_mac.h
(right):
> 
>
https://codereview.chromium.org/638123003/diff/60001/content/browser/composit...
> content/browser/compositor/browser_compositor_ca_layer_tree_mac.h:116: void
> BrowserCompositorCALayerTreeMacGotAcceleratedFrame(
> On 2014/10/17 11:32:09, tapted wrote:
> > (optional) I'd consider moving these to their own header, to avoid
replicating
> > the declaration in gpu_process_host_ui_shim.cc
> 
> I put in some #if defined(__OBJC__) to allow including this file in C++, which
> seems to do the trick.
> 
> I also considered keeping them in the class and #if defined(__OBJC__)-ing out
> parts of the class definition, but that seemed a bit too evil.

content/browser/gpu lgtm

[danakj, 2014-10-18 18:42:01]

https://codereview.chromium.org/638123003/diff/40001/content/browser/composit...
File content/browser/compositor/browser_compositor_view_mac.h (right):

https://codereview.chromium.org/638123003/diff/40001/content/browser/composit...
content/browser/compositor/browser_compositor_view_mac.h:55: ui::Compositor*
GetCompositor() const;
On 2014/10/17 01:06:38, ccameron1 wrote:
> On 2014/10/17 00:44:14, danakj wrote:
> > On 2014/10/17 00:40:14, ccameron1 wrote:
> > > On 2014/10/17 00:30:33, danakj wrote:
> > > > I wanna make sure we're on the right path and i'm understanding. This
> > > interface
> > > > is for content to implement and provide functionality to ui:: right?
What
> > code
> > > > in ui would need to use this to get the compositor?
> > > 
> > > The NSView that owns this will be calling SetSizeAndScale on the
> > ui::Compositor
> > > (among any other properties that it wants to set).
> > 
> > OK. The NSView doesn't exist yet right? I guess it would have this thing as
> it's
> > only path to the Compositor? I was picturing a view as also having
ui::Layers
> or
> > views::Widgets or something which also give you the compositor. It seems a
bit
> > funny to have ui:: code ask content:: to give it back the ui::Compositor.
But
> if
> > it makes sense then it does.
> 
> Ah -- the NSView exists -- this class then goes in an messes around with the
> drawing mechanism (the CALayers hiding behind the NSView) of the NSView when
it
> gets frames.
> 
> WRT the ui::Compositor being given to you by content, that's because of the
way
> that the ui::Compositors are recycled between tabs -- because we have a
separate
> NSView for each tab, we need to make sure that we don't alloc/free a
> ui::Compositor per-tab.

OK. Maybe it would make sense to do that messing around in ui instead of
content? You'd wanna do the same thing with an inprocess GL too? But maybe it
doesn't work out, I'm not familiar enough to say. This LGTM for now anyhow.

[ccameron, 2014-10-20 16:50:18]

Thanks!

On 2014/10/18 18:42:01, danakj wrote:
>
https://codereview.chromium.org/638123003/diff/40001/content/browser/composit...
> File content/browser/compositor/browser_compositor_view_mac.h (right):
> 
>
https://codereview.chromium.org/638123003/diff/40001/content/browser/composit...
> content/browser/compositor/browser_compositor_view_mac.h:55: ui::Compositor*
> GetCompositor() const;
> On 2014/10/17 01:06:38, ccameron1 wrote:
> > On 2014/10/17 00:44:14, danakj wrote:
> > > On 2014/10/17 00:40:14, ccameron1 wrote:
> > > > On 2014/10/17 00:30:33, danakj wrote:
> > > > > I wanna make sure we're on the right path and i'm understanding. This
> > > > interface
> > > > > is for content to implement and provide functionality to ui:: right?
> What
> > > code
> > > > > in ui would need to use this to get the compositor?
> > > > 
> > > > The NSView that owns this will be calling SetSizeAndScale on the
> > > ui::Compositor
> > > > (among any other properties that it wants to set).
> > > 
> > > OK. The NSView doesn't exist yet right? I guess it would have this thing
as
> > it's
> > > only path to the Compositor? I was picturing a view as also having
> ui::Layers
> > or
> > > views::Widgets or something which also give you the compositor. It seems a
> bit
> > > funny to have ui:: code ask content:: to give it back the ui::Compositor.
> But
> > if
> > > it makes sense then it does.
> > 
> > Ah -- the NSView exists -- this class then goes in an messes around with the
> > drawing mechanism (the CALayers hiding behind the NSView) of the NSView when
> it
> > gets frames.
> > 
> > WRT the ui::Compositor being given to you by content, that's because of the
> way
> > that the ui::Compositors are recycled between tabs -- because we have a
> separate
> > NSView for each tab, we need to make sure that we don't alloc/free a
> > ui::Compositor per-tab.
> 
> OK. Maybe it would make sense to do that messing around in ui instead of
> content? You'd wanna do the same thing with an inprocess GL too? But maybe it
> doesn't work out, I'm not familiar enough to say. This LGTM for now anyhow.

Yes, it will probably make sense to do that in ui:: instead of content
(especially if ui:: starts getting fairly complicated stuff with multiple
webviews going).

But yeah, I'l wait until we have a bit more of the ui:: side solidified before
trying to clean up the piping between the two.

[ccameron, 2014-10-20 16:50:26]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2014-10-20 16:51:27]

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

[commit-bot: I haz the power, 2014-10-20 18:03:24]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-10-20 18:04:25]

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