ozone: Add OzoneSurface object that is owned by compositor, GLSurface

ozone: Add OzoneSurface object that is owned by compositor, GLSurface

There is currently no way to clean up any objects created by ozone platforms
during surface initialization (i.e., RealizeAcceleratedWidget).

Therefore, it is not possible to implement a platform that does
nontrivial initialization in RealizeAcceleratedWidget without
leaking resources.

This adds an object representing the surface that is owned by the
GL surface or software output device. Any platform objects that
need to be destroyed at that time should be owned by this object.

There's no RealizeAcceleratedWidget any longer; this is replaced
by creation of the SurfaceOzone object.

TEST=ui_unittests, content_shell --ozone-platform={file,dri}
BUG=353788
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260580

[spang, 2014-03-20 00:41:44]

OzoneSurface or SurfaceOzone?

[spang, 2014-03-20 17:22:42]

This depends on https://codereview.chromium.org/196403008/

[rjkroege, 2014-03-24 16:07:03]

https://codereview.chromium.org/205433002/diff/20001/ui/aura/window_tree_host...
File ui/aura/window_tree_host_ozone.cc (left):

https://codereview.chromium.org/205433002/diff/20001/ui/aura/window_tree_host...
ui/aura/window_tree_host_ozone.cc:29:
surface_factory->AttemptToResizeAcceleratedWidget(widget_, bounds_);
I see that this change is necessary to compile but will the SurfaceOzone end up
being resized correctly without a different replacement?

https://codereview.chromium.org/205433002/diff/20001/ui/gfx/ozone/surface_fac...
File ui/gfx/ozone/surface_factory_ozone.h (right):

https://codereview.chromium.org/205433002/diff/20001/ui/gfx/ozone/surface_fac...
ui/gfx/ozone/surface_factory_ozone.h:113: // Create a surface for the specified
gfx::AcceleratedWidget.
you removed the text about needing a valid graphics context. I think something
more nuanced is needed. In particular: can one usefully build a useful
SurfaceOzone outside of the GPU process if the implementation is EGL-based?

https://codereview.chromium.org/205433002/diff/20001/ui/gfx/ozone/surface_ozo...
File ui/gfx/ozone/surface_ozone.h (right):

https://codereview.chromium.org/205433002/diff/20001/ui/gfx/ozone/surface_ozo...
ui/gfx/ozone/surface_ozone.h:29: // platform doesn't support, as the base class
will fails initialization
nit: "fails initialization" is weird grammar.

https://codereview.chromium.org/205433002/diff/20001/ui/gfx/ozone/surface_ozo...
ui/gfx/ozone/surface_ozone.h:45: // Initialize canvas for software output.
Returns true of initialization
s/of/if/

https://codereview.chromium.org/205433002/diff/20001/ui/gfx/ozone/surface_ozo...
ui/gfx/ozone/surface_ozone.h:49: // Returns a SkCanvas for the backing buffers.
Drawing to the canvas will draw
For better compatibility with Skia, the long term intent is that there will be a
1 SkCanvas per buffer instead of a single SkCanvas containing two SkBitmap
instances. At a minimum, this should be noted. Better perhaps would be to change
this API now to return two canvas instances. (Which at present we can hackily
make the same perhaps.)

https://codereview.chromium.org/205433002/diff/20001/ui/gfx/ozone/surface_ozo...
ui/gfx/ozone/surface_ozone.h:60: // Swap buffers in the SkCanvas. This back
buffer becomes the new front
This too needs to be adjusted to reflect the "two canvas" model.

https://codereview.chromium.org/205433002/diff/20001/ui/gfx/ozone/surface_ozo...
ui/gfx/ozone/surface_ozone.h:63: virtual bool SwapCanvas();
Ozone will need to intercept SwapBuffers from EGL land to inter-op with HWC
(i.e. calling set). This method seems inadequate.

[spang, 2014-03-24 16:42:10]

https://codereview.chromium.org/205433002/diff/20001/ui/aura/window_tree_host...
File ui/aura/window_tree_host_ozone.cc (left):

https://codereview.chromium.org/205433002/diff/20001/ui/aura/window_tree_host...
ui/aura/window_tree_host_ozone.cc:29:
surface_factory->AttemptToResizeAcceleratedWidget(widget_, bounds_);
On 2014/03/24 16:07:04, rjkroege wrote:
> I see that this change is necessary to compile but will the SurfaceOzone end
up
> being resized correctly without a different replacement?

This is redundant. The surface gets its size from the compositor size, and the
compositor is initialized from the window bounds inside
WindowTreeHost::InitCompositor().

WindowTreeHostOzone should not concern itself with the surface, only the window.

https://codereview.chromium.org/205433002/diff/20001/ui/gfx/ozone/surface_fac...
File ui/gfx/ozone/surface_factory_ozone.h (right):

https://codereview.chromium.org/205433002/diff/20001/ui/gfx/ozone/surface_fac...
ui/gfx/ozone/surface_factory_ozone.h:113: // Create a surface for the specified
gfx::AcceleratedWidget.
On 2014/03/24 16:07:04, rjkroege wrote:
> you removed the text about needing a valid graphics context. I think something
> more nuanced is needed. In particular: can one usefully build a useful
> SurfaceOzone outside of the GPU process if the implementation is EGL-based?

The language was wrong. SoftwareOutputDeviceOzone uses this and has no GL
context.

We can put the language at SurfaceOzone::InitializeEGL(), and require that you
have GL bindings initialized before trying to create the EGL native window.

Is such a requirement useful? That is, can we think of platforms that need it?

The native window is a:

X11 - Window. Don't need GL bindings to create this.
Android - ANativeWindow. Don't need GL bindings to create this.
Windows - HWND. Don't need GL bindings to create this.

It doesn't seem to me like this actually matters. It seems that in general, you
do not need GL bindings initialized to create the EGLNativeWindow.

https://codereview.chromium.org/205433002/diff/20001/ui/gfx/ozone/surface_ozo...
File ui/gfx/ozone/surface_ozone.h (right):

https://codereview.chromium.org/205433002/diff/20001/ui/gfx/ozone/surface_ozo...
ui/gfx/ozone/surface_ozone.h:29: // platform doesn't support, as the base class
will fails initialization
On 2014/03/24 16:07:04, rjkroege wrote:
> nit: "fails initialization" is weird grammar.

Done.

https://codereview.chromium.org/205433002/diff/20001/ui/gfx/ozone/surface_ozo...
ui/gfx/ozone/surface_ozone.h:45: // Initialize canvas for software output.
Returns true of initialization
On 2014/03/24 16:07:04, rjkroege wrote:
> s/of/if/

Done.

https://codereview.chromium.org/205433002/diff/20001/ui/gfx/ozone/surface_ozo...
ui/gfx/ozone/surface_ozone.h:49: // Returns a SkCanvas for the backing buffers.
Drawing to the canvas will draw
On 2014/03/24 16:07:04, rjkroege wrote:
> For better compatibility with Skia, the long term intent is that there will be
a
> 1 SkCanvas per buffer instead of a single SkCanvas containing two SkBitmap
> instances. At a minimum, this should be noted. Better perhaps would be to
change
> this API now to return two canvas instances. (Which at present we can hackily
> make the same perhaps.)

Agreed, let's break the API slightly more now to avoid breaking it again.

Looking into it.

https://codereview.chromium.org/205433002/diff/20001/ui/gfx/ozone/surface_ozo...
ui/gfx/ozone/surface_ozone.h:63: virtual bool SwapCanvas();
On 2014/03/24 16:07:04, rjkroege wrote:
> Ozone will need to intercept SwapBuffers from EGL land to inter-op with HWC
> (i.e. calling set). This method seems inadequate. 

We can add [On]SwapBuffersEGL(void *display, void *surface) after the next patch
and call it from GLSurfaceOzoneEGL. This would allow platforms to override [or
supplement] swap behavior.

[rjkroege, 2014-03-24 20:23:14]

https://codereview.chromium.org/205433002/diff/20001/ui/aura/window_tree_host...
File ui/aura/window_tree_host_ozone.cc (left):

https://codereview.chromium.org/205433002/diff/20001/ui/aura/window_tree_host...
ui/aura/window_tree_host_ozone.cc:29:
surface_factory->AttemptToResizeAcceleratedWidget(widget_, bounds_);
On 2014/03/24 16:42:11, spang wrote:
> On 2014/03/24 16:07:04, rjkroege wrote:
> > I see that this change is necessary to compile but will the SurfaceOzone end
> up
> > being resized correctly without a different replacement?
> 
> This is redundant. The surface gets its size from the compositor size, and the
> compositor is initialized from the window bounds inside
> WindowTreeHost::InitCompositor().
> 
> WindowTreeHostOzone should not concern itself with the surface, only the
window.

in implementations where it matters, how does the widget get its size?

https://codereview.chromium.org/205433002/diff/20001/ui/gfx/ozone/surface_fac...
File ui/gfx/ozone/surface_factory_ozone.h (right):

https://codereview.chromium.org/205433002/diff/20001/ui/gfx/ozone/surface_fac...
ui/gfx/ozone/surface_factory_ozone.h:113: // Create a surface for the specified
gfx::AcceleratedWidget.
On 2014/03/24 16:42:11, spang wrote:
> On 2014/03/24 16:07:04, rjkroege wrote:
> > you removed the text about needing a valid graphics context. I think
something
> > more nuanced is needed. In particular: can one usefully build a useful
> > SurfaceOzone outside of the GPU process if the implementation is EGL-based?
> 
> The language was wrong. SoftwareOutputDeviceOzone uses this and has no GL
> context.
> 
> We can put the language at SurfaceOzone::InitializeEGL(), and require that you
> have GL bindings initialized before trying to create the EGL native window.
> 
> Is such a requirement useful? That is, can we think of platforms that need it?
> 
> The native window is a:
> 
> X11 - Window. Don't need GL bindings to create this.
> Android - ANativeWindow. Don't need GL bindings to create this.
> Windows - HWND. Don't need GL bindings to create this.
> 
> It doesn't seem to me like this actually matters. It seems that in general,
you
> do not need GL bindings initialized to create the EGLNativeWindow.

OK. The point that I wanted captured in the documentation is that the
SurfaceOzone you get back from this call might not be particularly useful for
rendering if created in the browser process.

Can SurfaceOzone pointers be passed usefully between processes? I would think
not. If so, it seems worth noting that the consumer must make its SurfaceOzone
in the process where it intends to use it.

https://codereview.chromium.org/205433002/diff/20001/ui/gfx/ozone/surface_ozo...
File ui/gfx/ozone/surface_ozone.h (right):

https://codereview.chromium.org/205433002/diff/20001/ui/gfx/ozone/surface_ozo...
ui/gfx/ozone/surface_ozone.h:63: virtual bool SwapCanvas();
On 2014/03/24 16:42:11, spang wrote:
> On 2014/03/24 16:07:04, rjkroege wrote:
> > Ozone will need to intercept SwapBuffers from EGL land to inter-op with HWC
> > (i.e. calling set). This method seems inadequate. 
> 
> We can add [On]SwapBuffersEGL(void *display, void *surface) after the next
patch
> and call it from GLSurfaceOzoneEGL. This would allow platforms to override [or
> supplement] swap behavior.

that sounds reasonable.

[spang, 2014-03-24 21:58:28]

https://codereview.chromium.org/205433002/diff/20001/ui/aura/window_tree_host...
File ui/aura/window_tree_host_ozone.cc (left):

https://codereview.chromium.org/205433002/diff/20001/ui/aura/window_tree_host...
ui/aura/window_tree_host_ozone.cc:29:
surface_factory->AttemptToResizeAcceleratedWidget(widget_, bounds_);
On 2014/03/24 20:23:15, rjkroege wrote:
> On 2014/03/24 16:42:11, spang wrote:
> > On 2014/03/24 16:07:04, rjkroege wrote:
> > > I see that this change is necessary to compile but will the SurfaceOzone
end
> > up
> > > being resized correctly without a different replacement?
> > 
> > This is redundant. The surface gets its size from the compositor size, and
the
> > compositor is initialized from the window bounds inside
> > WindowTreeHost::InitCompositor().
> > 
> > WindowTreeHostOzone should not concern itself with the surface, only the
> window.
> 
> in implementations where it matters, how does the widget get its size?

Sizing upon initialization goes this way:

  1. Window is created with an initial bounds. This sets the initial size in the
windowing system (future: WindowFactoryOzone::CreateWindow). 
  2. WindowTreeHost calls GetBounds(), which gets the current bounds from the
windowing system, and uses that value to initialize the compositor
(Compositor::SetScaleAndSize).
  3. The compositor plumbs the size down into SoftwareOutputDeviceOzone.
  4. SoftwareOutputDeviceOzone calls AttemptToResizeAcceleratedWidget

That's what I meant when I said this was redundant. The same call already
happens via another path.

Let's let the compositor drive the surface sizing and not worry about it from
WindowTreeHost.

https://codereview.chromium.org/205433002/diff/20001/ui/gfx/ozone/surface_fac...
File ui/gfx/ozone/surface_factory_ozone.h (right):

https://codereview.chromium.org/205433002/diff/20001/ui/gfx/ozone/surface_fac...
ui/gfx/ozone/surface_factory_ozone.h:113: // Create a surface for the specified
gfx::AcceleratedWidget.
On 2014/03/24 20:23:15, rjkroege wrote:
> On 2014/03/24 16:42:11, spang wrote:
> > On 2014/03/24 16:07:04, rjkroege wrote:
> > > you removed the text about needing a valid graphics context. I think
> something
> > > more nuanced is needed. In particular: can one usefully build a useful
> > > SurfaceOzone outside of the GPU process if the implementation is
EGL-based?
> > 
> > The language was wrong. SoftwareOutputDeviceOzone uses this and has no GL
> > context.
> > 
> > We can put the language at SurfaceOzone::InitializeEGL(), and require that
you
> > have GL bindings initialized before trying to create the EGL native window.
> > 
> > Is such a requirement useful? That is, can we think of platforms that need
it?
> > 
> > The native window is a:
> > 
> > X11 - Window. Don't need GL bindings to create this.
> > Android - ANativeWindow. Don't need GL bindings to create this.
> > Windows - HWND. Don't need GL bindings to create this.
> > 
> > It doesn't seem to me like this actually matters. It seems that in general,
> you
> > do not need GL bindings initialized to create the EGLNativeWindow.
> 
> OK. The point that I wanted captured in the documentation is that the
> SurfaceOzone you get back from this call might not be particularly useful for
> rendering if created in the browser process.
> 
> Can SurfaceOzone pointers be passed usefully between processes? I would think
> not. If so, it seems worth noting that the consumer must make its SurfaceOzone
> in the process where it intends to use it.

Yeah, can't be passed between processes. That's what gfx::AcceleratedWidget is
for.

Updated with a blurb about GPU sometimes being out-of-process.

> 
> 
>

[spang, 2014-03-26 19:02:10]

https://codereview.chromium.org/205433002/diff/20001/ui/gfx/ozone/surface_ozo...
File ui/gfx/ozone/surface_ozone.h (right):

https://codereview.chromium.org/205433002/diff/20001/ui/gfx/ozone/surface_ozo...
ui/gfx/ozone/surface_ozone.h:49: // Returns a SkCanvas for the backing buffers.
Drawing to the canvas will draw
On 2014/03/24 16:42:11, spang wrote:
> On 2014/03/24 16:07:04, rjkroege wrote:
> > For better compatibility with Skia, the long term intent is that there will
be
> a
> > 1 SkCanvas per buffer instead of a single SkCanvas containing two SkBitmap
> > instances. At a minimum, this should be noted. Better perhaps would be to
> change
> > this API now to return two canvas instances. (Which at present we can
hackily
> > make the same perhaps.)
> 
> Agreed, let's break the API slightly more now to avoid breaking it again.
> 
> Looking into it.

I've coordinated with dnicoara to update the API here so that it hopefully does
not need to change again soon.

You now must call GetCanvas() each frame.

https://codereview.chromium.org/205433002/diff/20001/ui/gfx/ozone/surface_ozo...
ui/gfx/ozone/surface_ozone.h:60: // Swap buffers in the SkCanvas. This back
buffer becomes the new front
On 2014/03/24 16:07:04, rjkroege wrote:
> This too needs to be adjusted to reflect the "two canvas" model.

Done.

[rjkroege, 2014-03-26 19:59:02]

https://codereview.chromium.org/205433002/diff/80001/ui/gfx/ozone/surface_ozo...
File ui/gfx/ozone/surface_ozone.h (right):

https://codereview.chromium.org/205433002/diff/80001/ui/gfx/ozone/surface_ozo...
ui/gfx/ozone/surface_ozone.h:30: // platform doesn't support, as the base class
will fail initialization for
nit: "will provide no-op implementations for all methods"

https://codereview.chromium.org/205433002/diff/80001/ui/gfx/ozone/surface_ozo...
ui/gfx/ozone/surface_ozone.h:45: // Initialize canvas for software output.
Returns true of initialization
s/of/if/

https://codereview.chromium.org/205433002/diff/80001/ui/gfx/ozone/surface_ozo...
ui/gfx/ozone/surface_ozone.h:51: virtual skia::RefPtr<SkCanvas> GetCanvas();
GetDrawableCanvas?

https://codereview.chromium.org/205433002/diff/80001/ui/gfx/ozone/surface_ozo...
ui/gfx/ozone/surface_ozone.h:61: virtual bool SwapCanvas();
call this "PresentCurrentCanvas" and reduce the comment.

https://codereview.chromium.org/205433002/diff/80001/ui/gl/gl_surface_ozone.cc
File ui/gl/gl_surface_ozone.cc (right):

https://codereview.chromium.org/205433002/diff/80001/ui/gl/gl_surface_ozone.c...
ui/gl/gl_surface_ozone.cc:22: class GL_EXPORT GLSurfaceOzoneEGL : public
NativeViewGLSurfaceEGL {
it bothers me that there is a whole class here whose only reason for existing is
to add a scoped pointer for an ozone_surface_ 

what is the overlap between the EGLSurface and the SurfaceOzone? Could they be
the same?

[spang, 2014-03-26 22:21:27]

https://codereview.chromium.org/205433002/diff/80001/ui/gfx/ozone/surface_ozo...
File ui/gfx/ozone/surface_ozone.h (right):

https://codereview.chromium.org/205433002/diff/80001/ui/gfx/ozone/surface_ozo...
ui/gfx/ozone/surface_ozone.h:30: // platform doesn't support, as the base class
will fail initialization for
On 2014/03/26 19:59:02, rjkroege wrote:
> nit: "will provide no-op implementations for all methods"

My idea was that InitializeFoo() in the base class returns false, and in that
case it's a bug to call any others since you fail the precondition.

In fact in this patch the default GetEGLNativeWindow and GetCanvas are not
merely unimplemented but NOTREACHED().

This is what I was trying to communicate.

https://codereview.chromium.org/205433002/diff/80001/ui/gfx/ozone/surface_ozo...
ui/gfx/ozone/surface_ozone.h:45: // Initialize canvas for software output.
Returns true of initialization
On 2014/03/26 19:59:02, rjkroege wrote:
> s/of/if/

Doh! I swear I did this already..

https://codereview.chromium.org/205433002/diff/80001/ui/gfx/ozone/surface_ozo...
ui/gfx/ozone/surface_ozone.h:51: virtual skia::RefPtr<SkCanvas> GetCanvas();
On 2014/03/26 19:59:02, rjkroege wrote:
> GetDrawableCanvas? 

If we had both GetDrawableCanvas and GetCanvas, what would the difference be?

Doesn't seem like there is one. And so I like the simpler name.

https://codereview.chromium.org/205433002/diff/80001/ui/gfx/ozone/surface_ozo...
ui/gfx/ozone/surface_ozone.h:61: virtual bool SwapCanvas();
On 2014/03/26 19:59:02, rjkroege wrote:
> call this "PresentCurrentCanvas" and reduce the comment.

Hah, I was thinking it should say Present as well. How about PresentCanvas?

Or maybe GetCurrentCanvas + PresentCurrentCanvas?

I think it's probably just THE canvas.

https://codereview.chromium.org/205433002/diff/80001/ui/gl/gl_surface_ozone.cc
File ui/gl/gl_surface_ozone.cc (right):

https://codereview.chromium.org/205433002/diff/80001/ui/gl/gl_surface_ozone.c...
ui/gl/gl_surface_ozone.cc:22: class GL_EXPORT GLSurfaceOzoneEGL : public
NativeViewGLSurfaceEGL {
On 2014/03/26 19:59:02, rjkroege wrote:
> it bothers me that there is a whole class here whose only reason for existing
is
> to add a scoped pointer for an ozone_surface_ 
> 
> what is the overlap between the EGLSurface and the SurfaceOzone? Could they be
> the same?

The point of SurfaceOzone is that platforms implement it.

It's not possible for ozone-based platforms to override GLSurface currently.
That is, unless you want to make major changes to the dependency structure.

If they could override the GLSurface itself, we absolutely would not need to do
this.

There is a strict-ownership relationship between
GLSurfaceOzoneEGL/SoftwareOutputDeviceOzone (platform-independent) and
SurfaceOzone (platform-dependent). We need that in order to tear things down at
the right time.

[rjkroege, 2014-03-27 19:24:11]

lgtm

https://codereview.chromium.org/205433002/diff/100001/ui/gfx/ozone/surface_oz...
File ui/gfx/ozone/surface_ozone.h (right):

https://codereview.chromium.org/205433002/diff/100001/ui/gfx/ozone/surface_oz...
ui/gfx/ozone/surface_ozone.h:50: virtual skia::RefPtr<SkCanvas> GetCanvas();
if I call GetCanvas twice without an intervening PresentCanvas, what ought to
happen?

[spang, 2014-03-27 19:43:46]

https://codereview.chromium.org/205433002/diff/100001/ui/gfx/ozone/surface_oz...
File ui/gfx/ozone/surface_ozone.h (right):

https://codereview.chromium.org/205433002/diff/100001/ui/gfx/ozone/surface_oz...
ui/gfx/ozone/surface_ozone.h:50: virtual skia::RefPtr<SkCanvas> GetCanvas();
On 2014/03/27 19:24:12, rjkroege wrote:
> if I call GetCanvas twice without an intervening PresentCanvas, what ought to
> happen?

I think "GetFoo" should be assumed idempotent. Therefore, calling it twice
returns the same canvas. It is not dequeuing the canvas, or anything like that.

An alternative API would be something like:

 DequeueCanvas()
 EnqueueCanvas()

That way is closer to how ANativeWindow works.

Doing that might allow some more "interesting" semantics, but we don't actually
need them for SoftwareOutputDeviceOzone, which is really the only user of this
API.

[spang, 2014-03-27 19:52:49]

+piman for ui/gl + content/browser/compositor
+sadrul for ui/aura

[sadrul, 2014-03-27 19:53:33]

aura lgtm

[piman, 2014-03-27 21:20:56]

https://codereview.chromium.org/205433002/diff/100001/ui/gfx/ozone/surface_oz...
File ui/gfx/ozone/surface_ozone.cc (right):

https://codereview.chromium.org/205433002/diff/100001/ui/gfx/ozone/surface_oz...
ui/gfx/ozone/surface_ozone.cc:22: return 0;  // not reached
nit: comment is superfluous

https://codereview.chromium.org/205433002/diff/100001/ui/gfx/ozone/surface_oz...
ui/gfx/ozone/surface_ozone.cc:29: return skia::RefPtr<SkCanvas>();  // not
reached
nit: comment is superfluous

https://codereview.chromium.org/205433002/diff/100001/ui/gfx/ozone/surface_oz...
File ui/gfx/ozone/surface_ozone.h (right):

https://codereview.chromium.org/205433002/diff/100001/ui/gfx/ozone/surface_oz...
ui/gfx/ozone/surface_ozone.h:31: class GFX_EXPORT SurfaceOzone {
nit: can we keep SurfaceOzone a pure virtual interface, and have a
SurfaceOzoneImpl (or SurfaceOzoneBase) implement a cross-platform base for the
in-tree implementations?

This is to make sure the implementation of this doesn't become part of the
interface for out-of-tree implementations ("fragile base class problem"),
causing implicit circular dependencies.

Actually, do we even need the implementations? They're all
NOTIMPLEMENTED/NOTREACHED.

[spang, 2014-03-28 16:09:13]

https://codereview.chromium.org/205433002/diff/100001/ui/gfx/ozone/surface_oz...
File ui/gfx/ozone/surface_ozone.cc (right):

https://codereview.chromium.org/205433002/diff/100001/ui/gfx/ozone/surface_oz...
ui/gfx/ozone/surface_ozone.cc:22: return 0;  // not reached
On 2014/03/27 21:20:56, piman wrote:
> nit: comment is superfluous

Done.

https://codereview.chromium.org/205433002/diff/100001/ui/gfx/ozone/surface_oz...
ui/gfx/ozone/surface_ozone.cc:29: return skia::RefPtr<SkCanvas>();  // not
reached
On 2014/03/27 21:20:56, piman wrote:
> nit: comment is superfluous

Done.

https://codereview.chromium.org/205433002/diff/100001/ui/gfx/ozone/surface_oz...
File ui/gfx/ozone/surface_ozone.h (right):

https://codereview.chromium.org/205433002/diff/100001/ui/gfx/ozone/surface_oz...
ui/gfx/ozone/surface_ozone.h:31: class GFX_EXPORT SurfaceOzone {
On 2014/03/27 21:20:56, piman wrote:
> nit: can we keep SurfaceOzone a pure virtual interface, and have a
> SurfaceOzoneImpl (or SurfaceOzoneBase) implement a cross-platform base for the
> in-tree implementations?
> 
> This is to make sure the implementation of this doesn't become part of the
> interface for out-of-tree implementations ("fragile base class problem"),
> causing implicit circular dependencies.
> 
> Actually, do we even need the implementations? They're all
> NOTIMPLEMENTED/NOTREACHED.

On 2014/03/27 21:20:56, piman wrote:
> nit: can we keep SurfaceOzone a pure virtual interface, and have a
> SurfaceOzoneImpl (or SurfaceOzoneBase) implement a cross-platform base for the
> in-tree implementations?

I'm happy to make this a pure interface but I'd like to better understand the
reason. Can you explain further how it might create circular dependencies?

Out-of-tree platforms actually get built into the ui/ozone component, and so can
safely depend on anything ui/ozone depends on. In particular ui/gfx is allowed.

> 
> This is to make sure the implementation of this doesn't become part of the
> interface for out-of-tree implementations ("fragile base class problem"),
> causing implicit circular dependencies.
> 
> Actually, do we even need the implementations? They're all
> NOTIMPLEMENTED/NOTREACHED.

The default implementations are here only as a convenience. It would also allow
us to add more methods without necessarily breaking derived classes.

[piman, 2014-03-28 17:41:24]

On Fri, Mar 28, 2014 at 9:09 AM, <spang@chromium.org> wrote:

>
> https://codereview.chromium.org/205433002/diff/100001/ui/
> gfx/ozone/surface_ozone.cc
> File ui/gfx/ozone/surface_ozone.cc (right):
>
> https://codereview.chromium.org/205433002/diff/100001/ui/
> gfx/ozone/surface_ozone.cc#newcode22
> ui/gfx/ozone/surface_ozone.cc:22: return 0;  // not reached
> On 2014/03/27 21:20:56, piman wrote:
>
>> nit: comment is superfluous
>>
>
> Done.
>
>
> https://codereview.chromium.org/205433002/diff/100001/ui/
> gfx/ozone/surface_ozone.cc#newcode29
> ui/gfx/ozone/surface_ozone.cc:29: return skia::RefPtr<SkCanvas>();  //
> not reached
> On 2014/03/27 21:20:56, piman wrote:
>
>> nit: comment is superfluous
>>
>
> Done.
>
>
> https://codereview.chromium.org/205433002/diff/100001/ui/
> gfx/ozone/surface_ozone.h
> File ui/gfx/ozone/surface_ozone.h (right):
>
> https://codereview.chromium.org/205433002/diff/100001/ui/
> gfx/ozone/surface_ozone.h#newcode31
> ui/gfx/ozone/surface_ozone.h:31: class GFX_EXPORT SurfaceOzone {
> On 2014/03/27 21:20:56, piman wrote:
>
>> nit: can we keep SurfaceOzone a pure virtual interface, and have a
>> SurfaceOzoneImpl (or SurfaceOzoneBase) implement a cross-platform base
>>
> for the
>
>> in-tree implementations?
>>
>
>  This is to make sure the implementation of this doesn't become part of
>>
> the
>
>> interface for out-of-tree implementations ("fragile base class
>>
> problem"),
>
>> causing implicit circular dependencies.
>>
>
>  Actually, do we even need the implementations? They're all
>> NOTIMPLEMENTED/NOTREACHED.
>>
>
> On 2014/03/27 21:20:56, piman wrote:
>
>> nit: can we keep SurfaceOzone a pure virtual interface, and have a
>> SurfaceOzoneImpl (or SurfaceOzoneBase) implement a cross-platform base
>>
> for the
>
>> in-tree implementations?
>>
>
> I'm happy to make this a pure interface but I'd like to better
> understand the reason. Can you explain further how it might create
> circular dependencies?


> Out-of-tree platforms actually get built into the ui/ozone component,
> and so can safely depend on anything ui/ozone depends on. In particular
> ui/gfx is allowed.



If I change a base class in a way that breaks functionality without
changing the interface:
1- I'll never get a compilation error, but I broke things. Effectively the
implementation becomes part of the interface ("fragile base class
problem"), which means changing any of the implementation should be held to
the same standard as changing the interface, but it's worse because it
causes run-time failures instead of compile-time failures.
2- by definition, I can't update out-of-tree implementations since they're
out-of-tree. Next time they update their version of chromium, they'll
silently break.

The style guide also forbids inheriting from more than 1 non-pure base
class. That means if one derives from SurfaceOzone, it can't derive from
anything other than pure interfaces. It's an unnecessary constraint, since
SurfaceOzone is meant to be an interface

Btw, having out-of-tree implementations depend on ui/gfx is actually a
problem, because it causes circular dependencies (chrome->out-of-tree
implementation->chrome). I wish we didn't do that.


>  This is to make sure the implementation of this doesn't become part of
>>
> the
>
>> interface for out-of-tree implementations ("fragile base class
>>
> problem"),
>
>> causing implicit circular dependencies.
>>
>
>  Actually, do we even need the implementations? They're all
>> NOTIMPLEMENTED/NOTREACHED.
>>
>
> The default implementations are here only as a convenience. It would
> also allow us to add more methods without necessarily breaking derived
> classes.
>

How would you know if you break them or not?
At least, with a pure interface, you get a compile error which lets you
know that something has changed.


>
> https://codereview.chromium.org/205433002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[spang, 2014-03-28 18:14:00]

On 2014/03/28 17:41:24, piman wrote:
> On Fri, Mar 28, 2014 at 9:09 AM, <mailto:spang@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/205433002/diff/100001/ui/
> > gfx/ozone/surface_ozone.cc
> > File ui/gfx/ozone/surface_ozone.cc (right):
> >
> > https://codereview.chromium.org/205433002/diff/100001/ui/
> > gfx/ozone/surface_ozone.cc#newcode22
> > ui/gfx/ozone/surface_ozone.cc:22: return 0;  // not reached
> > On 2014/03/27 21:20:56, piman wrote:
> >
> >> nit: comment is superfluous
> >>
> >
> > Done.
> >
> >
> > https://codereview.chromium.org/205433002/diff/100001/ui/
> > gfx/ozone/surface_ozone.cc#newcode29
> > ui/gfx/ozone/surface_ozone.cc:29: return skia::RefPtr<SkCanvas>();  //
> > not reached
> > On 2014/03/27 21:20:56, piman wrote:
> >
> >> nit: comment is superfluous
> >>
> >
> > Done.
> >
> >
> > https://codereview.chromium.org/205433002/diff/100001/ui/
> > gfx/ozone/surface_ozone.h
> > File ui/gfx/ozone/surface_ozone.h (right):
> >
> > https://codereview.chromium.org/205433002/diff/100001/ui/
> > gfx/ozone/surface_ozone.h#newcode31
> > ui/gfx/ozone/surface_ozone.h:31: class GFX_EXPORT SurfaceOzone {
> > On 2014/03/27 21:20:56, piman wrote:
> >
> >> nit: can we keep SurfaceOzone a pure virtual interface, and have a
> >> SurfaceOzoneImpl (or SurfaceOzoneBase) implement a cross-platform base
> >>
> > for the
> >
> >> in-tree implementations?
> >>
> >
> >  This is to make sure the implementation of this doesn't become part of
> >>
> > the
> >
> >> interface for out-of-tree implementations ("fragile base class
> >>
> > problem"),
> >
> >> causing implicit circular dependencies.
> >>
> >
> >  Actually, do we even need the implementations? They're all
> >> NOTIMPLEMENTED/NOTREACHED.
> >>
> >
> > On 2014/03/27 21:20:56, piman wrote:
> >
> >> nit: can we keep SurfaceOzone a pure virtual interface, and have a
> >> SurfaceOzoneImpl (or SurfaceOzoneBase) implement a cross-platform base
> >>
> > for the
> >
> >> in-tree implementations?
> >>
> >
> > I'm happy to make this a pure interface but I'd like to better
> > understand the reason. Can you explain further how it might create
> > circular dependencies?
> 
> 
> > Out-of-tree platforms actually get built into the ui/ozone component,
> > and so can safely depend on anything ui/ozone depends on. In particular
> > ui/gfx is allowed.
> 
> 
> 
> If I change a base class in a way that breaks functionality without
> changing the interface:
> 1- I'll never get a compilation error, but I broke things. Effectively the
> implementation becomes part of the interface ("fragile base class
> problem"), which means changing any of the implementation should be held to
> the same standard as changing the interface, but it's worse because it
> causes run-time failures instead of compile-time failures.
> 2- by definition, I can't update out-of-tree implementations since they're
> out-of-tree. Next time they update their version of chromium, they'll
> silently break.

This is fair, I was considering that we would add new optional functionality,
not change the behavior of old functionality. I do think it is preferable for
the latter to break at compile time rather than runtime.

> 
> The style guide also forbids inheriting from more than 1 non-pure base
> class. That means if one derives from SurfaceOzone, it can't derive from
> anything other than pure interfaces. It's an unnecessary constraint, since
> SurfaceOzone is meant to be an interface
> 
> Btw, having out-of-tree implementations depend on ui/gfx is actually a
> problem, because it causes circular dependencies (chrome->out-of-tree
> implementation->chrome). I wish we didn't do that.
 
It's not circular within the build-system visible components, at least. We are
sandwiching out-of-tree code between in-tree components in a way that works
within the dependency scheme. It is only circular if you consider chrome as a
single component.

Of course, you would be correct to point out that there is a versioning problem
lurking here. That is, you can't necessarily dump two random out-of-tree
platforms into the same checkout and expect that both build. You may have to
garden one or both of the platforms first.

I think that's fine, though. We don't promise that this works, and if you are
building an out-of-tree platform it is your responsibility to do any necessary
gardening. Wayland is handling the gardening using gclient & DEPS.

I do not think there is really a better solution that fits our needs. We don't
really want to make a stable interface guarantee; ozone needs to be keep up with
the rate of change in chrome.

Ozone with a stable interface would simply be another windowing system, like X11
or wayland. We wouldn't be able to use any of chromium's native types
(ui::Event, SkBitmap, etc) because they themselves are not stable. And so we
would need another translation layer from the native types to ozone types. The
gardening requirement seems like the lesser of two evils here.

> 
> 
> >  This is to make sure the implementation of this doesn't become part of
> >>
> > the
> >
> >> interface for out-of-tree implementations ("fragile base class
> >>
> > problem"),
> >
> >> causing implicit circular dependencies.
> >>
> >
> >  Actually, do we even need the implementations? They're all
> >> NOTIMPLEMENTED/NOTREACHED.
> >>
> >
> > The default implementations are here only as a convenience. It would
> > also allow us to add more methods without necessarily breaking derived
> > classes.
> >
> 
> How would you know if you break them or not?
> At least, with a pure interface, you get a compile error which lets you
> know that something has changed.
> 
> 
> >
> > https://codereview.chromium.org/205433002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[spang, 2014-03-28 18:41:20]

On 2014/03/28 18:14:00, spang wrote:
> On 2014/03/28 17:41:24, piman wrote:
> > On Fri, Mar 28, 2014 at 9:09 AM, <mailto:spang@chromium.org> wrote:
> > 
> > >
> > > https://codereview.chromium.org/205433002/diff/100001/ui/
> > > gfx/ozone/surface_ozone.cc
> > > File ui/gfx/ozone/surface_ozone.cc (right):
> > >
> > > https://codereview.chromium.org/205433002/diff/100001/ui/
> > > gfx/ozone/surface_ozone.cc#newcode22
> > > ui/gfx/ozone/surface_ozone.cc:22: return 0;  // not reached
> > > On 2014/03/27 21:20:56, piman wrote:
> > >
> > >> nit: comment is superfluous
> > >>
> > >
> > > Done.
> > >
> > >
> > > https://codereview.chromium.org/205433002/diff/100001/ui/
> > > gfx/ozone/surface_ozone.cc#newcode29
> > > ui/gfx/ozone/surface_ozone.cc:29: return skia::RefPtr<SkCanvas>();  //
> > > not reached
> > > On 2014/03/27 21:20:56, piman wrote:
> > >
> > >> nit: comment is superfluous
> > >>
> > >
> > > Done.
> > >
> > >
> > > https://codereview.chromium.org/205433002/diff/100001/ui/
> > > gfx/ozone/surface_ozone.h
> > > File ui/gfx/ozone/surface_ozone.h (right):
> > >
> > > https://codereview.chromium.org/205433002/diff/100001/ui/
> > > gfx/ozone/surface_ozone.h#newcode31
> > > ui/gfx/ozone/surface_ozone.h:31: class GFX_EXPORT SurfaceOzone {
> > > On 2014/03/27 21:20:56, piman wrote:
> > >
> > >> nit: can we keep SurfaceOzone a pure virtual interface, and have a
> > >> SurfaceOzoneImpl (or SurfaceOzoneBase) implement a cross-platform base
> > >>
> > > for the
> > >
> > >> in-tree implementations?
> > >>
> > >
> > >  This is to make sure the implementation of this doesn't become part of
> > >>
> > > the
> > >
> > >> interface for out-of-tree implementations ("fragile base class
> > >>
> > > problem"),
> > >
> > >> causing implicit circular dependencies.
> > >>
> > >
> > >  Actually, do we even need the implementations? They're all
> > >> NOTIMPLEMENTED/NOTREACHED.
> > >>
> > >
> > > On 2014/03/27 21:20:56, piman wrote:
> > >
> > >> nit: can we keep SurfaceOzone a pure virtual interface, and have a
> > >> SurfaceOzoneImpl (or SurfaceOzoneBase) implement a cross-platform base
> > >>
> > > for the
> > >
> > >> in-tree implementations?
> > >>
> > >
> > > I'm happy to make this a pure interface but I'd like to better
> > > understand the reason. Can you explain further how it might create
> > > circular dependencies?
> > 
> > 
> > > Out-of-tree platforms actually get built into the ui/ozone component,
> > > and so can safely depend on anything ui/ozone depends on. In particular
> > > ui/gfx is allowed.
> > 
> > 
> > 
> > If I change a base class in a way that breaks functionality without
> > changing the interface:
> > 1- I'll never get a compilation error, but I broke things. Effectively the
> > implementation becomes part of the interface ("fragile base class
> > problem"), which means changing any of the implementation should be held to
> > the same standard as changing the interface, but it's worse because it
> > causes run-time failures instead of compile-time failures.
> > 2- by definition, I can't update out-of-tree implementations since they're
> > out-of-tree. Next time they update their version of chromium, they'll
> > silently break.
> 
> This is fair, I was considering that we would add new optional functionality,
> not change the behavior of old functionality. I do think it is preferable for
> the latter to break at compile time rather than runtime.
> 
> > 
> > The style guide also forbids inheriting from more than 1 non-pure base
> > class. That means if one derives from SurfaceOzone, it can't derive from
> > anything other than pure interfaces. It's an unnecessary constraint, since
> > SurfaceOzone is meant to be an interface
> > 
> > Btw, having out-of-tree implementations depend on ui/gfx is actually a
> > problem, because it causes circular dependencies (chrome->out-of-tree
> > implementation->chrome). I wish we didn't do that.
>  
> It's not circular within the build-system visible components, at least. We are
> sandwiching out-of-tree code between in-tree components in a way that works
> within the dependency scheme. It is only circular if you consider chrome as a
> single component.
> 
> Of course, you would be correct to point out that there is a versioning
problem
> lurking here. That is, you can't necessarily dump two random out-of-tree
> platforms into the same checkout and expect that both build. You may have to
> garden one or both of the platforms first.
> 
> I think that's fine, though. We don't promise that this works, and if you are
> building an out-of-tree platform it is your responsibility to do any necessary
> gardening. Wayland is handling the gardening using gclient & DEPS.
> 
> I do not think there is really a better solution that fits our needs. We don't
> really want to make a stable interface guarantee; ozone needs to be keep up
with
> the rate of change in chrome.
> 
> Ozone with a stable interface would simply be another windowing system, like
X11
> or wayland. We wouldn't be able to use any of chromium's native types
> (ui::Event, SkBitmap, etc) because they themselves are not stable. And so we
> would need another translation layer from the native types to ozone types. The
> gardening requirement seems like the lesser of two evils here.

FWIW I actually think it is more appropriate to consider out-of-tree platforms
such as
ozone-wayland as branches or forks of chromium, rather than thinking of the
platform
as a layer that sits below chromium.

Ozone is mainly about reducing friction by making gardening easier. Our
intention
is that you need zero patches to existing files, but rather merely drop in more
files
that inherit from some base interfaces and provide a platform constructor. Those
interfaces will change over time, so its not as if gardening is eliminated, but
it is
much easier than having to patch hundreds of ifdefs all over the code and also
drop in
_fooplatform files everywhere.

> > 
> > 
> > >  This is to make sure the implementation of this doesn't become part of
> > >>
> > > the
> > >
> > >> interface for out-of-tree implementations ("fragile base class
> > >>
> > > problem"),
> > >
> > >> causing implicit circular dependencies.
> > >>
> > >
> > >  Actually, do we even need the implementations? They're all
> > >> NOTIMPLEMENTED/NOTREACHED.
> > >>
> > >
> > > The default implementations are here only as a convenience. It would
> > > also allow us to add more methods without necessarily breaking derived
> > > classes.
> > >
> > 
> > How would you know if you break them or not?
> > At least, with a pure interface, you get a compile error which lets you
> > know that something has changed.
> > 
> > 
> > >
> > > https://codereview.chromium.org/205433002/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

[spang, 2014-03-28 19:08:25]

https://codereview.chromium.org/205433002/diff/100001/ui/gfx/ozone/surface_oz...
File ui/gfx/ozone/surface_ozone.h (right):

https://codereview.chromium.org/205433002/diff/100001/ui/gfx/ozone/surface_oz...
ui/gfx/ozone/surface_ozone.h:31: class GFX_EXPORT SurfaceOzone {
On 2014/03/27 21:20:56, piman wrote:
> nit: can we keep SurfaceOzone a pure virtual interface, and have a
> SurfaceOzoneImpl (or SurfaceOzoneBase) implement a cross-platform base for the
> in-tree implementations?
> 
> This is to make sure the implementation of this doesn't become part of the
> interface for out-of-tree implementations ("fragile base class problem"),
> causing implicit circular dependencies.
> 
> Actually, do we even need the implementations? They're all
> NOTIMPLEMENTED/NOTREACHED.

Done.

[piman, 2014-03-28 20:08:28]

lgtm

[spang, 2014-03-28 20:48:20]

The CQ bit was checked by spang@chromium.org

[commit-bot: I haz the power, 2014-03-28 20:51:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/spang@chromium.org/205433002/150001

[commit-bot: I haz the power, 2014-03-28 23:45:19]

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

[commit-bot: I haz the power, 2014-03-28 23:45:19]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[spang, 2014-03-31 14:06:08]

The CQ bit was checked by spang@chromium.org

[commit-bot: I haz the power, 2014-03-31 14:06:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/spang@chromium.org/205433002/150001

[commit-bot: I haz the power, 2014-03-31 15:38:24]

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

[commit-bot: I haz the power, 2014-03-31 15:38:25]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[spang, 2014-03-31 15:43:30]

The CQ bit was checked by spang@chromium.org

[spang, 2014-03-31 15:43:36]

The CQ bit was unchecked by spang@chromium.org

[spang, 2014-03-31 15:43:38]

The CQ bit was checked by spang@chromium.org

[commit-bot: I haz the power, 2014-03-31 15:45:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/spang@chromium.org/205433002/150001

[commit-bot: I haz the power, 2014-03-31 17:16:44]

Change committed as 260580