Split surface id and simplify connecting to surfaces service

Split surface id and simplify connecting to surfaces service

Surface IDs are composed of a namespace component and a local
identifier component. This splits up the two pieces in the mojom to make
it clearer.

This also simplifies the path for connecting to the surfaces service. In
order to perform any operations with surfaces, each client must know
what its id namespace value is. This was done by first connecting to a
SurfacesService interface and then calling CreateSurfaceConnection which
returned a Surface pointer and the namespace. Having to connect to this
extra interface and receive the Surface impl asynchronously complicated
startup code for applications by adding extra states in startup.

Instead of that, this allows connecting to the Surface interface
directly and promises that the ID namespace will be provided as the
first message in the pipe directed at the SurfaceClient. Callers can
then either block on startup or handle setting the ID namespace
asynchronously depending on what other things that thread could be doing
at the time.

In a follow-up, I plan to define the id namespace value 0 as meaning "the
namespace of this connection" which will allow creating surfaces and
submitting frames before learning the connection's namespace. The only
thing the namespace will be passing surface IDs to other clients for them
to embed.

This also removes the Size parameter from CreateSurface, which wasn't
used by anything. The size of submitted frames is part of the embedder /
embedee contract.

R=esprehn@chromium.org, sky@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/6f344a8b02226f7375580fd271f00e193b7062e2

[jamesr, 2014-12-16 00:02:48]

https://codereview.chromium.org/807733002/diff/1/sky/viewer/document_view.cc
File sky/viewer/document_view.cc (right):

https://codereview.chromium.org/807733002/diff/1/sky/viewer/document_view.cc#...
sky/viewer/document_view.cc:159: if (root_)
this is not directly related, but fixes a flaky shutdown crash

[esprehn, 2014-12-16 00:51:10]

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

[esprehn, 2014-12-16 00:51:11]

https://codereview.chromium.org/807733002/diff/1/sky/viewer/document_view.cc
File sky/viewer/document_view.cc (right):

https://codereview.chromium.org/807733002/diff/1/sky/viewer/document_view.cc#...
sky/viewer/document_view.cc:159: if (root_)
On 2014/12/16 at 00:02:47, jamesr wrote:
> this is not directly related, but fixes a flaky shutdown crash

Wouldn't we want to shutdown the connection to surfaces service after
OnViewDestroyed instead? What frees the surface with this id?

[jamesr, 2014-12-16 00:56:09]

https://codereview.chromium.org/807733002/diff/1/sky/viewer/document_view.cc
File sky/viewer/document_view.cc (right):

https://codereview.chromium.org/807733002/diff/1/sky/viewer/document_view.cc#...
sky/viewer/document_view.cc:159: if (root_)
On 2014/12/16 00:51:11, esprehn wrote:
> On 2014/12/16 at 00:02:47, jamesr wrote:
> > this is not directly related, but fixes a flaky shutdown crash
> 
> Wouldn't we want to shutdown the connection to surfaces service after
> OnViewDestroyed instead? What frees the surface with this id?

This isn't about controlling the surface, this is about communicating the ID
with the view manager.  At this point, if root_ is null then our app's part of
the view manager lib (including its connection to the view manager service) have
already gone down so there's nowhere to send the ID.

The surface itself is managed elsewhere.

[eseidel, 2014-12-16 00:58:23]

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

[eseidel, 2014-12-16 00:58:23]

Does this fix https://github.com/domokit/mojo/issues/31?

[jamesr, 2014-12-16 01:09:03]

On 2014/12/16 00:58:23, eseidel wrote:
> Does this fix https://github.com/domokit/mojo/issues/31

Yes

[esprehn, 2014-12-16 01:23:57]

https://codereview.chromium.org/807733002/diff/1/sky/viewer/document_view.cc
File sky/viewer/document_view.cc (right):

https://codereview.chromium.org/807733002/diff/1/sky/viewer/document_view.cc#...
sky/viewer/document_view.cc:159: if (root_)
On 2014/12/16 at 00:56:09, jamesr wrote:
> On 2014/12/16 00:51:11, esprehn wrote:
> > On 2014/12/16 at 00:02:47, jamesr wrote:
> > > this is not directly related, but fixes a flaky shutdown crash
> > 
> > Wouldn't we want to shutdown the connection to surfaces service after
> > OnViewDestroyed instead? What frees the surface with this id?
> 
> This isn't about controlling the surface, this is about communicating the ID
with the view manager.  At this point, if root_ is null then our app's part of
the view manager lib (including its connection to the view manager service) have
already gone down so there's nowhere to send the ID.
> 
> The surface itself is managed elsewhere.

Why do we even get this message if everything is torn down?

[jamesr, 2014-12-16 01:32:00]

This doesn't come from a message, this runs from a posted task generated by sky:

https://github.com/domokit/mojo/issues/31

STDERR: Received signal 11 SEGV_MAPERR 000000000000
STDERR: #0 0x7fc40867dfde base::debug::StackTrace::StackTrace()
STDERR: #1 0x7fc40867db13 base::debug::(anonymous
namespace)::StackDumpSignalHandler()
STDERR: #2 0x7fc45da56340 
STDERR: #3 0x7fc4078ea11d mojo::View::SetSurfaceId()
STDERR: #4 0x7fc4077c8041 sky::DocumentView::OnSurfaceIdAvailable()
STDERR: #5 0x7fc4077c809c sky::DocumentView::OnSurfaceIdAvailable()
STDERR: #6 0x7fc40858b77a sky::LayerHost::OnSurfaceIdAvailable()
STDERR: #7 0x7fc408594772 sky::SurfaceHolder::SetSize()
STDERR: #8 0x7fc40858c03b sky::LayerHost::Upload()
STDERR: #9 0x7fc40858bd07 sky::LayerHost::BeginFrame()
STDERR: #10 0x7fc40858e162 base::internal::RunnableAdapter<>::Run()

Sky posts a task to the message loop to run sky::LayerHost::BeginFrame().  In
between this task being posted and running, many things can happen.  In this
instance the view manager connection closed, but other connections could very
well be active still.

[esprehn, 2014-12-16 02:21:10]

On 2014/12/16 at 01:32:00, jamesr wrote:
> This doesn't come from a message, this runs from a posted task generated by
sky:
> 
> https://github.com/domokit/mojo/issues/31
> 
> STDERR: Received signal 11 SEGV_MAPERR 000000000000
> STDERR: #0 0x7fc40867dfde base::debug::StackTrace::StackTrace()
> STDERR: #1 0x7fc40867db13 base::debug::(anonymous
namespace)::StackDumpSignalHandler()
> STDERR: #2 0x7fc45da56340 
> STDERR: #3 0x7fc4078ea11d mojo::View::SetSurfaceId()
> STDERR: #4 0x7fc4077c8041 sky::DocumentView::OnSurfaceIdAvailable()
> STDERR: #5 0x7fc4077c809c sky::DocumentView::OnSurfaceIdAvailable()
> STDERR: #6 0x7fc40858b77a sky::LayerHost::OnSurfaceIdAvailable()
> STDERR: #7 0x7fc408594772 sky::SurfaceHolder::SetSize()
> STDERR: #8 0x7fc40858c03b sky::LayerHost::Upload()
> STDERR: #9 0x7fc40858bd07 sky::LayerHost::BeginFrame()
> STDERR: #10 0x7fc40858e162 base::internal::RunnableAdapter<>::Run()
> 
> Sky posts a task to the message loop to run sky::LayerHost::BeginFrame().  In
between this task being posted and running, many things can happen.  In this
instance the view manager connection closed, but other connections could very
well be active still.

I see, it looks like we should discard the LayerHost on View destruction which
would cancel the pending tasks for the LayerHost. lgtm for sky for now, I can
fix that later.

[sky, 2014-12-16 03:34:04]

LGTM

https://codereview.chromium.org/807733002/diff/1/examples/surfaces_app/embedd...
File examples/surfaces_app/embedder.h (right):

https://codereview.chromium.org/807733002/diff/1/examples/surfaces_app/embedd...
examples/surfaces_app/embedder.h:30: void set_surface_id(SurfaceIdPtr id) { id_
= id.Clone(); }
id.Pass()?

https://codereview.chromium.org/807733002/diff/1/services/surfaces/surfaces_i...
File services/surfaces/surfaces_impl.cc (right):

https://codereview.chromium.org/807733002/diff/1/services/surfaces/surfaces_i...
services/surfaces/surfaces_impl.cc:27:
SurfacesImpl::SurfacesImpl(cc::SurfaceManager* manager,
nit: move to match position in header.

[jamesr, 2014-12-16 23:15:26]

Committed patchset #1 (id:1) manually as
6f344a8b02226f7375580fd271f00e193b7062e2 (presubmit successful).