Add a simple compositor for Sky

Add a simple compositor for Sky

Currently we're only able to put up one frame and we can have only one layer, but
this is enough to draw a green circle in a sample app. A future CL will wire this
up to Sky and add support for drawing a second frame.

R=eseidel@chromium.org, ojan@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/d9134df4b33b56c40db5fc5445945bb37820b33f

[abarth-chromium, 2014-11-20 20:22:16]

abarth@chromium.org changed reviewers:
+ jamesr@chromium.org, ojan@chromium.org, rafaelw@chromium.org

[eseidel, 2014-11-20 20:40:30]

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

[eseidel, 2014-11-20 20:40:32]

lgtm

Honestly this is really ugly.  I'm sure we can do better, but a lot of this is
also chromium's sucky style choices.

https://codereview.chromium.org/740923002/diff/40001/examples/sky_compositor_...
File examples/sky_compositor_app/sky_compositor_app.cc (right):

https://codereview.chromium.org/740923002/diff/40001/examples/sky_compositor_...
examples/sky_compositor_app/sky_compositor_app.cc:22: gfx::Size ToSize(const
mojo::Rect& rect) {
Lame you have to define this yourself.  Does mojo::Rect not make it possible to
add a size() method?

https://codereview.chromium.org/740923002/diff/40001/examples/sky_compositor_...
examples/sky_compositor_app/sky_compositor_app.cc:54: root_layer_ =
make_scoped_refptr(new sky::Layer(this));
What's with the lower_case_method?

https://codereview.chromium.org/740923002/diff/40001/examples/sky_compositor_...
examples/sky_compositor_app/sky_compositor_app.cc:59:
base::MessageLoop::current()->Quit();
AppManager::Terminate or some such.  Look at what debugger.cc does.

https://codereview.chromium.org/740923002/diff/40001/examples/sky_compositor_...
examples/sky_compositor_app/sky_compositor_app.cc:64: mojo::Shell* GetShell()
override { return shell_; }
Style explosion.  I thought this was Shell()?

https://codereview.chromium.org/740923002/diff/40001/sky/compositor/layer.h
File sky/compositor/layer.h (right):

https://codereview.chromium.org/740923002/diff/40001/sky/compositor/layer.h#n...
sky/compositor/layer.h:29: void set_host(LayerHost* host) { host_ = host; }
Huh?  inlined methods are crazy_style?

https://codereview.chromium.org/740923002/diff/40001/sky/compositor/layer.h#n...
sky/compositor/layer.h:32: friend class base::RefCounted<Layer>;
You have to add this boilerplate so that RefCounted can delete you?

https://codereview.chromium.org/740923002/diff/40001/sky/compositor/resource_...
File sky/compositor/resource_manager.cc (right):

https://codereview.chromium.org/740923002/diff/40001/sky/compositor/resource_...
sky/compositor/resource_manager.cc:81: delete texture;
There is no way manual memory management is ever correct.

https://codereview.chromium.org/740923002/diff/40001/sky/compositor/surface_a...
File sky/compositor/surface_allocator.cc (right):

https://codereview.chromium.org/740923002/diff/40001/sky/compositor/surface_a...
sky/compositor/surface_allocator.cc:20: return
static_cast<uint64_t>(id_namespace_) << 32 | next_id_++;
Could you explain to the next person what the heck you're trying to do here.

[abarth-chromium, 2014-11-20 20:47:57]

https://codereview.chromium.org/740923002/diff/40001/examples/sky_compositor_...
File examples/sky_compositor_app/sky_compositor_app.cc (right):

https://codereview.chromium.org/740923002/diff/40001/examples/sky_compositor_...
examples/sky_compositor_app/sky_compositor_app.cc:22: gfx::Size ToSize(const
mojo::Rect& rect) {
On 2014/11/20 at 20:40:31, eseidel wrote:
> Lame you have to define this yourself.  Does mojo::Rect not make it possible
to add a size() method?

mojo::Rect doesn't have any helper functions.  The idea is that you're supposed
to convert to your own Rect type (e.g., gfx::Rect).

https://codereview.chromium.org/740923002/diff/40001/examples/sky_compositor_...
examples/sky_compositor_app/sky_compositor_app.cc:54: root_layer_ =
make_scoped_refptr(new sky::Layer(this));
On 2014/11/20 at 20:40:31, eseidel wrote:
> What's with the lower_case_method?

It's supposed to be an analogy to make_unique_ptr and make_shared_ptr from STL.

https://codereview.chromium.org/740923002/diff/40001/sky/compositor/layer.h
File sky/compositor/layer.h (right):

https://codereview.chromium.org/740923002/diff/40001/sky/compositor/layer.h#n...
sky/compositor/layer.h:29: void set_host(LayerHost* host) { host_ = host; }
On 2014/11/20 at 20:40:32, eseidel wrote:
> Huh?  inlined methods are crazy_style?

Trivial getters and setters are crazy_style.

https://codereview.chromium.org/740923002/diff/40001/sky/compositor/layer.h#n...
sky/compositor/layer.h:32: friend class base::RefCounted<Layer>;
On 2014/11/20 at 20:40:31, eseidel wrote:
> You have to add this boilerplate so that RefCounted can delete you?

Yep.  In Blink, we do the opposite (i.e., we make the constructor protected and
have the destruct public).  If you try that, the Clang style checker gets mad at
you and forces you do it this way.

https://codereview.chromium.org/740923002/diff/40001/sky/compositor/resource_...
File sky/compositor/resource_manager.cc (right):

https://codereview.chromium.org/740923002/diff/40001/sky/compositor/resource_...
sky/compositor/resource_manager.cc:81: delete texture;
On 2014/11/20 at 20:40:32, eseidel wrote:
> There is no way manual memory management is ever correct.

The problem is that resource_to_texture_map_ is an STL hash_map, which does't
understand move-only types like scoped_ptr or unique_ptr until the C++11 version
of the STL, which we're not allowed to use yet.

[ojan, 2014-11-20 21:25:47]

lgtm as well

https://codereview.chromium.org/740923002/diff/40001/examples/sky_compositor_...
File examples/sky_compositor_app/sky_compositor_app.cc (right):

https://codereview.chromium.org/740923002/diff/40001/examples/sky_compositor_...
examples/sky_compositor_app/sky_compositor_app.cc:72: // sky::LayerHostClient
// sky::LayerClient ?

https://codereview.chromium.org/740923002/diff/40001/sky/compositor/layer_cli...
File sky/compositor/layer_client.cc (right):

https://codereview.chromium.org/740923002/diff/40001/sky/compositor/layer_cli...
sky/compositor/layer_client.cc:9: LayerClient::~LayerClient() {
Does chromium style require we have a cc file here? Seems kinda silly to not
just put the empty destructor in the header?

https://codereview.chromium.org/740923002/diff/40001/sky/compositor/surface_a...
File sky/compositor/surface_allocator.cc (right):

https://codereview.chromium.org/740923002/diff/40001/sky/compositor/surface_a...
sky/compositor/surface_allocator.cc:20: return
static_cast<uint64_t>(id_namespace_) << 32 | next_id_++;
On 2014/11/20 at 20:40:32, eseidel wrote:
> Could you explain to the next person what the heck you're trying to do here.

+1

[eseidel, 2014-11-20 21:35:09]

https://codereview.chromium.org/740923002/diff/40001/sky/compositor/layer_cli...
File sky/compositor/layer_client.cc (right):

https://codereview.chromium.org/740923002/diff/40001/sky/compositor/layer_cli...
sky/compositor/layer_client.cc:9: LayerClient::~LayerClient() {
On 2014/11/20 21:25:46, ojan wrote:
> Does chromium style require we have a cc file here? Seems kinda silly to not
> just put the empty destructor in the header?

Destructors are deceptive.  All the member destructors are called from this.  In
this object's case that's like 5 destructors worth.  That ends up compiling into
every .o file and then needing to be stripped by the linker.  Much better to be
in a separate file imo.

[abarth-chromium, 2014-11-20 22:07:20]

https://codereview.chromium.org/740923002/diff/40001/examples/sky_compositor_...
File examples/sky_compositor_app/sky_compositor_app.cc (right):

https://codereview.chromium.org/740923002/diff/40001/examples/sky_compositor_...
examples/sky_compositor_app/sky_compositor_app.cc:59:
base::MessageLoop::current()->Quit();
On 2014/11/20 at 20:40:31, eseidel wrote:
> AppManager::Terminate or some such.  Look at what debugger.cc does.

Done.

https://codereview.chromium.org/740923002/diff/40001/examples/sky_compositor_...
examples/sky_compositor_app/sky_compositor_app.cc:72: // sky::LayerHostClient
On 2014/11/20 at 21:25:46, ojan wrote:
> // sky::LayerClient ?

Done.

https://codereview.chromium.org/740923002/diff/40001/sky/compositor/layer_cli...
File sky/compositor/layer_client.cc (right):

https://codereview.chromium.org/740923002/diff/40001/sky/compositor/layer_cli...
sky/compositor/layer_client.cc:9: LayerClient::~LayerClient() {
On 2014/11/20 at 21:35:09, eseidel wrote:
> Destructors are deceptive.  All the member destructors are called from this. 
In this object's case that's like 5 destructors worth.  That ends up compiling
into every .o file and then needing to be stripped by the linker.  Much better
to be in a separate file imo.

It also makes sure layer_client.h compiles without any other headers included
first.  I think the style guide allows us to include {} in the header though.

https://codereview.chromium.org/740923002/diff/40001/sky/compositor/surface_a...
File sky/compositor/surface_allocator.cc (right):

https://codereview.chromium.org/740923002/diff/40001/sky/compositor/surface_a...
sky/compositor/surface_allocator.cc:20: return
static_cast<uint64_t>(id_namespace_) << 32 | next_id_++;
On 2014/11/20 at 21:25:46, ojan wrote:
> On 2014/11/20 at 20:40:32, eseidel wrote:
> > Could you explain to the next person what the heck you're trying to do here.
> 
> +1

Done.

[abarth-chromium, 2014-11-20 22:10:44]

Committed patchset #4 (id:60001) manually as
d9134df4b33b56c40db5fc5445945bb37820b33f (presubmit successful).

[jamesr, 2014-12-02 05:43:48]

https://codereview.chromium.org/740923002/diff/60001/sky/compositor/BUILD.gn
File sky/compositor/BUILD.gn (right):

https://codereview.chromium.org/740923002/diff/60001/sky/compositor/BUILD.gn#...
sky/compositor/BUILD.gn:5: static_library("compositor") {
static_library is nearly never what you want, you probably mean source_set here

on linux the static_library target type generates a 'thin archive' which is
basically just a file that lists the .os but doesn't contain their contents, so
making something a static_library instead of a source_set just adds an extra
step but still requires the linker to look at the same set of stuff when linking
against this 'static lib' and it adds the risk of dropping symbols when going
through several layers of indirection

[abarth-chromium, 2014-12-02 06:30:12]

Will fix

[abarth-chromium, 2014-12-02 06:34:03]

Fix in https://codereview.chromium.org/766053007