cc: Move WebCompositorOutputSurface and related classes into cc/

cc: Move WebCompositorOutputSurface and related classes into cc/

This moves:
- WebKit::WebCompositorOutputSurface to cc::OutputSurface
- WebKit::WebCompositorOutputSurfaceClient to cc::OutputSurfaceClient
- Webkit::WebCompositorSoftwareOutputDevice to cc::SoftwareOutputDevice

They become new files in cc/ so they are chromified style. This affects
the various call-sites. This allows us to pass cc::CompositorFrame
directly, without any need for WebCompositorFrame.

BUG=146080
Depends on: https://bugs.webkit.org/show_bug.cgi?id=103967

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171847

[danakj, 2012-12-04 05:09:00]

James, Antoine please both take a look.

This needs https://bugs.webkit.org/show_bug.cgi?id=103967 to hide the
webkitCaseFunctionNames.

[danakj, 2012-12-04 05:29:05]

+aelias

[piman, 2012-12-04 06:41:06]

LGTM+nit.
In all the new files, you should keep to 80 cols to respect chromium style.

https://codereview.chromium.org/11348371/diff/5001/cc/output_surface.h
File cc/output_surface.h (right):

https://codereview.chromium.org/11348371/diff/5001/cc/output_surface.h#newcode20
cc/output_surface.h:20: //   1. Created on the main thread via
WebLayerTreeView::createOutputSurface.
nit: doc probably needs updating

[jamesr, 2012-12-04 07:06:51]

try to format new/moved chromium-style files to 80col. i didn't mark everything
that was over

https://codereview.chromium.org/11348371/diff/5001/cc/output_surface.h
File cc/output_surface.h (right):

https://codereview.chromium.org/11348371/diff/5001/cc/output_surface.h#newcode43
cc/output_surface.h:43: // Obtain the 3d context or the software device
associated with this output surface. Either of these may return a null pointer,
but not both.
80col

https://codereview.chromium.org/11348371/diff/5001/cc/output_surface_client.h
File cc/output_surface_client.h (right):

https://codereview.chromium.org/11348371/diff/5001/cc/output_surface_client.h...
cc/output_surface_client.h:1: // Copyright 2012 The Chromium Authors. All rights
reserved.
leading space here for some reason

https://codereview.chromium.org/11348371/diff/5001/cc/output_surface_client.h...
cc/output_surface_client.h:14: virtual void OnVSyncParametersChanged(double
monotonicTimebase, double intervalInSeconds) = 0;
the types on this should be base::TimeTicks and base::TimeDelta. they're only
double in WebKit because it doesn't have proper time types

80col

https://codereview.chromium.org/11348371/diff/5001/cc/output_surface_client.h...
cc/output_surface_client.h:15: virtual void
OnSendFrameToParentCompositorAck(const CompositorFrameAck&) {}
do we need a default impl of this or can it be pure virtual? it's useful to have
default impls on logically pure virtual interfaces in WebKit API since it's
annoying to update impls at the same time, but since this is all chromium you
should be able to find + update all OutputSurfaceClient implementations

https://codereview.chromium.org/11348371/diff/5001/cc/output_surface_client.h...
cc/output_surface_client.h:16: };
can you add an empty virtual dtor (probably protected) ?

https://codereview.chromium.org/11348371/diff/5001/cc/software_output_device.h
File cc/software_output_device.h (right):

https://codereview.chromium.org/11348371/diff/5001/cc/software_output_device....
cc/software_output_device.h:9: #include <public/WebImage.h>
do

#include "third_party/WebKit/Source/Platform/chromium/public/WebImage.h"

instead so we don't depend on having funky stuff on the include dirs for all
targets that pull this header in

and then resort the includes

https://codereview.chromium.org/11348371/diff/5001/cc/test/fake_graphics_cont...
File cc/test/fake_graphics_context.h (right):

https://codereview.chromium.org/11348371/diff/5001/cc/test/fake_graphics_cont...
cc/test/fake_graphics_context.h:12: namespace WebKit {
whaaa?

https://codereview.chromium.org/11348371/diff/5001/cc/test/fake_output_surface.h
File cc/test/fake_output_surface.h (right):

https://codereview.chromium.org/11348371/diff/5001/cc/test/fake_output_surfac...
cc/test/fake_output_surface.h:8: #include <public/WebGraphicsContext3D.h>
"third_party/WebKit/...." and sort with the rest of the headers

https://codereview.chromium.org/11348371/diff/5001/cc/test/fake_output_surfac...
cc/test/fake_output_surface.h:21: static inline scoped_ptr<FakeOutputSurface>
Create(scoped_ptr<WebKit::WebGraphicsContext3D> context3d) {
80col

https://codereview.chromium.org/11348371/diff/5001/cc/test/fake_output_surfac...
cc/test/fake_output_surface.h:25: static inline scoped_ptr<FakeOutputSurface>
CreateSoftware(scoped_ptr<SoftwareOutputDevice> software_device) {
guess what

https://codereview.chromium.org/11348371/diff/5001/cc/test/fake_output_surfac...
cc/test/fake_output_surface.h:39: explicit
FakeOutputSurface(scoped_ptr<WebKit::WebGraphicsContext3D> context3d);
1 char over :(

https://codereview.chromium.org/11348371/diff/5001/cc/test/fake_software_outp...
File cc/test/fake_software_output_device.h (right):

https://codereview.chromium.org/11348371/diff/5001/cc/test/fake_software_outp...
cc/test/fake_software_output_device.h:8: #include <public/WebImage.h>
"third_party/WebKit..."

https://codereview.chromium.org/11348371/diff/5001/cc/test/layer_tree_test_co...
File cc/test/layer_tree_test_common.cc (right):

https://codereview.chromium.org/11348371/diff/5001/cc/test/layer_tree_test_co...
cc/test/layer_tree_test_common.cc:38: using namespace WebKitTests;
can you using the specific types instead of the whole namespace to be more
inline with chromium style?

https://codereview.chromium.org/11348371/diff/5001/content/browser/renderer_h...
File content/browser/renderer_host/image_transport_factory.cc (right):

https://codereview.chromium.org/11348371/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/image_transport_factory.cc:339:
client_->OnVSyncParametersChanged(monotonicTimebase, intervalInSeconds);
you can rid of of all this ugly math when you make OnVSync... take base::Time*
parameters

https://codereview.chromium.org/11348371/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/image_transport_factory.cc:345: struct
Capabilities capabilities_;
don't need the "struct" here

https://codereview.chromium.org/11348371/diff/5001/content/renderer/gpu/compo...
File content/renderer/gpu/compositor_output_surface.cc (right):

https://codereview.chromium.org/11348371/diff/5001/content/renderer/gpu/compo...
content/renderer/gpu/compositor_output_surface.cc:113:
static_cast<double>(base::Time::kMicrosecondsPerSecond);
this math goes away too!

https://codereview.chromium.org/11348371/diff/5001/content/renderer/gpu/compo...
File content/renderer/gpu/compositor_output_surface.h (right):

https://codereview.chromium.org/11348371/diff/5001/content/renderer/gpu/compo...
content/renderer/gpu/compositor_output_surface.h:32: NON_EXPORTED_BASE(public
base::NonThreadSafe) {
fix alignment on this? not sure how it was busted before

https://codereview.chromium.org/11348371/diff/5001/content/renderer/gpu/compo...
content/renderer/gpu/compositor_output_surface.h:44: virtual const struct
Capabilities& Capabilities() const OVERRIDE;
no "struct" here prz

https://codereview.chromium.org/11348371/diff/5001/content/renderer/gpu/compo...
content/renderer/gpu/compositor_output_surface.h:79: struct Capabilities
capabilities_;
no "struct"

https://codereview.chromium.org/11348371/diff/5001/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/11348371/diff/5001/ui/compositor/compositor.c...
ui/compositor/compositor.cc:74: virtual const struct Capabilities&
Capabilities() const OVERRIDE {
s/struct //

https://codereview.chromium.org/11348371/diff/5001/ui/compositor/compositor.c...
ui/compositor/compositor.cc:92: struct Capabilities capabilities_;
s/struct //

https://codereview.chromium.org/11348371/diff/5001/ui/compositor/compositor.c...
ui/compositor/compositor.cc:505: Compositor::createOutputSurface() {
i think this fits on the previous line now, no?

[danakj, 2012-12-04 18:15:34]

PTAL

I have no idea why clang demands the "struct" eveywhere in the base classes, but
it is. It says "you must use struct from this scope."

https://codereview.chromium.org/11348371/diff/5001/cc/output_surface.h
File cc/output_surface.h (right):

https://codereview.chromium.org/11348371/diff/5001/cc/output_surface.h#newcode20
cc/output_surface.h:20: //   1. Created on the main thread via
WebLayerTreeView::createOutputSurface.
On 2012/12/04 06:41:06, piman wrote:
> nit: doc probably needs updating

I don't think this changed. The code paths are the same, just the data type
names changed. It still goes through this function.

https://codereview.chromium.org/11348371/diff/5001/cc/output_surface_client.h
File cc/output_surface_client.h (right):

https://codereview.chromium.org/11348371/diff/5001/cc/output_surface_client.h...
cc/output_surface_client.h:14: virtual void OnVSyncParametersChanged(double
monotonicTimebase, double intervalInSeconds) = 0;
On 2012/12/04 07:06:51, jamesr wrote:
> the types on this should be base::TimeTicks and base::TimeDelta. they're only
> double in WebKit because it doesn't have proper time types
> 
> 80col

Done.

https://codereview.chromium.org/11348371/diff/5001/cc/output_surface_client.h...
cc/output_surface_client.h:15: virtual void
OnSendFrameToParentCompositorAck(const CompositorFrameAck&) {}
On 2012/12/04 07:06:51, jamesr wrote:
> do we need a default impl of this or can it be pure virtual? it's useful to
have
> default impls on logically pure virtual interfaces in WebKit API since it's
> annoying to update impls at the same time, but since this is all chromium you
> should be able to find + update all OutputSurfaceClient implementations

Sure, I'll have to add the method to LayerTreeHost now then.

https://codereview.chromium.org/11348371/diff/5001/cc/output_surface_client.h...
cc/output_surface_client.h:16: };
On 2012/12/04 07:06:51, jamesr wrote:
> can you add an empty virtual dtor (probably protected) ?

Done.

https://codereview.chromium.org/11348371/diff/5001/cc/software_output_device.h
File cc/software_output_device.h (right):

https://codereview.chromium.org/11348371/diff/5001/cc/software_output_device....
cc/software_output_device.h:9: #include <public/WebImage.h>
On 2012/12/04 07:06:51, jamesr wrote:
> do
> 
> #include "third_party/WebKit/Source/Platform/chromium/public/WebImage.h"
> 
> instead so we don't depend on having funky stuff on the include dirs for all
> targets that pull this header in
> 
> and then resort the includes

Oh, I was trying to avoid adding things to DEPS. Switching!

https://codereview.chromium.org/11348371/diff/5001/cc/test/fake_graphics_cont...
File cc/test/fake_graphics_context.h (right):

https://codereview.chromium.org/11348371/diff/5001/cc/test/fake_graphics_cont...
cc/test/fake_graphics_context.h:12: namespace WebKit {
On 2012/12/04 07:06:51, jamesr wrote:
> whaaa?

ya.. changed to cc!

We have a bunch of namespace WebKit and/or WebKitTests in test/ still I think.

https://codereview.chromium.org/11348371/diff/5001/cc/test/fake_output_surface.h
File cc/test/fake_output_surface.h (right):

https://codereview.chromium.org/11348371/diff/5001/cc/test/fake_output_surfac...
cc/test/fake_output_surface.h:8: #include <public/WebGraphicsContext3D.h>
On 2012/12/04 07:06:51, jamesr wrote:
> "third_party/WebKit/...." and sort with the rest of the headers

k, and added to DEPS

https://codereview.chromium.org/11348371/diff/5001/cc/test/layer_tree_test_co...
File cc/test/layer_tree_test_common.cc (right):

https://codereview.chromium.org/11348371/diff/5001/cc/test/layer_tree_test_co...
cc/test/layer_tree_test_common.cc:38: using namespace WebKitTests;
On 2012/12/04 07:06:51, jamesr wrote:
> can you using the specific types instead of the whole namespace to be more
> inline with chromium style?

I moved the animation test stuff to cc:: so this can go away.

https://codereview.chromium.org/11348371/diff/5001/content/browser/renderer_h...
File content/browser/renderer_host/image_transport_factory.cc (right):

https://codereview.chromium.org/11348371/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/image_transport_factory.cc:345: struct
Capabilities capabilities_;
On 2012/12/04 07:06:51, jamesr wrote:
> don't need the "struct" here

The compiler makes me use struct everywhere :/ I have no idea why though. Maybe
you will..

https://codereview.chromium.org/11348371/diff/5001/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/11348371/diff/5001/ui/compositor/compositor.c...
ui/compositor/compositor.cc:505: Compositor::createOutputSurface() {
On 2012/12/04 07:06:51, jamesr wrote:
> i think this fits on the previous line now, no?

yes :)

[danakj, 2012-12-04 18:16:36]

On Tue, Dec 4, 2012 at 1:15 PM,  <danakj@chromium.org> wrote:
> PTAL
>
> I have no idea why clang demands the "struct" eveywhere in the base classes,

sub classes*

[piman, 2012-12-04 22:24:42]

https://codereview.chromium.org/11348371/diff/5001/cc/output_surface.h
File cc/output_surface.h (right):

https://codereview.chromium.org/11348371/diff/5001/cc/output_surface.h#newcode20
cc/output_surface.h:20: //   1. Created on the main thread via
WebLayerTreeView::createOutputSurface.
On 2012/12/04 18:15:34, danakj wrote:
> On 2012/12/04 06:41:06, piman wrote:
> > nit: doc probably needs updating
> 
> I don't think this changed. The code paths are the same, just the data type
> names changed. It still goes through this function.

I was thinking here and below.

couple of things:
- this isn't created through WebLayerTreeView::createOutputSurface on Aura
(since we use cc/ directly). Generically I wouldn't define the API in cc in
terms of what happens in WebKit.
- below, s/bindToClient/BindToClient/

[danakj, 2012-12-04 22:33:57]

PTAL

On 2012/12/04 22:24:42, piman wrote:
> https://codereview.chromium.org/11348371/diff/5001/cc/output_surface.h
> File cc/output_surface.h (right):
> 
>
https://codereview.chromium.org/11348371/diff/5001/cc/output_surface.h#newcode20
> cc/output_surface.h:20: //   1. Created on the main thread via
> WebLayerTreeView::createOutputSurface.
> On 2012/12/04 18:15:34, danakj wrote:
> > On 2012/12/04 06:41:06, piman wrote:
> > > nit: doc probably needs updating
> > 
> > I don't think this changed. The code paths are the same, just the data type
> > names changed. It still goes through this function.
> 
> I was thinking here and below.
> 
> couple of things:
> - this isn't created through WebLayerTreeView::createOutputSurface on Aura
> (since we use cc/ directly). Generically I wouldn't define the API in cc in
> terms of what happens in WebKit.
> - below, s/bindToClient/BindToClient/

I see. Ok I changed it to this, which sounds nice to my ears:
//   1. Created on the main thread by the LayerTreeHost through its client.

I have added WebCompositorSupport stuff to this CL so that DRT can create a
surface. The WebKit CL will make use of these when they become available here.
Then we can remove the old WebKit side things.

[danakj, 2012-12-06 04:56:25]

jamesr: can you please look at the webkit/ bits? that's the only part that was
added since your last look.

The only other questionable thing was using "struct" everywhere, but clang has
been insistent on it.

[danakj, 2012-12-07 17:06:56]

review ping

[jamesr, 2012-12-07 17:36:22]

lgtm

https://codereview.chromium.org/11348371/diff/19001/webkit/compositor_binding...
File webkit/compositor_bindings/web_layer_tree_view_impl.cc (right):

https://codereview.chromium.org/11348371/diff/19001/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_tree_view_impl.cc:241:
WebKit::WebCompositorOutputSurface *web = m_client->createOutputSurface();
i think the * should go with the type

https://codereview.chromium.org/11348371/diff/19001/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_tree_view_impl.cc:242: return
make_scoped_ptr(static_cast<cc::OutputSurface*>(web));
can you avoid the static cast?  perhaps

scoped_ptr<cc::OutputSurface>(web).Pass()

[danakj, 2012-12-07 17:43:12]

thanks!

https://codereview.chromium.org/11348371/diff/19001/webkit/compositor_binding...
File webkit/compositor_bindings/web_layer_tree_view_impl.cc (right):

https://codereview.chromium.org/11348371/diff/19001/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_tree_view_impl.cc:241:
WebKit::WebCompositorOutputSurface *web = m_client->createOutputSurface();
On 2012/12/07 17:36:24, jamesr wrote:
> i think the * should go with the type

Done.

https://codereview.chromium.org/11348371/diff/19001/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_tree_view_impl.cc:242: return
make_scoped_ptr(static_cast<cc::OutputSurface*>(web));
On 2012/12/07 17:36:24, jamesr wrote:
> can you avoid the static cast?  perhaps
> 
> scoped_ptr<cc::OutputSurface>(web).Pass()

No :( That doesn't compile, nor does:

make_scoped_ptr(web).PassAs<cc::OutputSurface>()

[commit-bot: I haz the power, 2012-12-07 17:50:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11348371/25001

[commit-bot: I haz the power, 2012-12-07 18:05:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11348371/29002

[commit-bot: I haz the power, 2012-12-07 18:28:41]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-12-07 18:34:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11348371/36005

[commit-bot: I haz the power, 2012-12-07 19:04:47]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-12-07 19:08:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11348371/31004