Mojo: Use InterfaceFactory<Interface> for service registration

Mojo: Use InterfaceFactory<Interface> for service registration

This adds an InterfaceFactory<Interface> type and allows registering a
service through its provider. Using an InterfaceFactory allows the app
to be in control of the lifetime of the implementation of the interface
and hides all of the implementation details of the interface from the
application library code (i.e. no subclassing impl classes or anything
like that).  The default binding behavior is to bind the lifetime of
the impl to the lifetime of the pipe, but the application
can also weakly bind to a service in cases where it needs to manage
the lifetime explicitly.

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

[jamesr, 2014-07-10 23:52:24]

With this the forest of templated types in service_connector.h can go away, I
believe.

https://codereview.chromium.org/380413003/diff/1/mojo/services/test_service/t...
File mojo/services/test_service/test_service_application.cc (right):

https://codereview.chromium.org/380413003/diff/1/mojo/services/test_service/t...
mojo/services/test_service/test_service_application.cc:33:
mojo::BindToRequest(new TestServiceImpl(connection, this), &request);
this is how simple BindToRequest() implementations will look - just construct
the implementation and bind it to the request.

i also experimented with returning an Interface* and having the
InterfaceProviderConnector do the bind, but i'd have to use an out param instead
of return type since you can't override on return type in C++. 
InterfaceRequest<Interface> seems like a clearer out-param style thing than
something like Interface**.

https://codereview.chromium.org/380413003/diff/1/mojo/services/test_service/t...
File mojo/services/test_service/test_service_application.h (right):

https://codereview.chromium.org/380413003/diff/1/mojo/services/test_service/t...
mojo/services/test_service/test_service_application.h:11: #include
"mojo/services/test_service/test_service.mojom.h"
this could be a forward declaration, actually

https://codereview.chromium.org/380413003/diff/1/mojo/services/test_service/t...
mojo/services/test_service/test_service_application.h:19: public
InterfaceProvider<TestTimeService> {
this is probably the most interesting usage of the type. this class is the
provider for 2 different interfaces (TestService and TestTimeService) and
constructs them in slightly different ways (one wants a back pointer in the
constructor)

[jamesr, 2014-07-11 00:35:22]

ps2 converts more things over.  I'm generally liking the feel of this so far,
but see a few places that could perhaps be a bit tighter.

[jamesr, 2014-07-11 00:44:07]

https://codereview.chromium.org/380413003/diff/40001/mojo/shell/profile_servi...
File mojo/shell/profile_service_loader.cc (right):

https://codereview.chromium.org/380413003/diff/40001/mojo/shell/profile_servi...
mojo/shell/profile_service_loader.cc:48: mojo::BindToRequest(&impl_, &request);
check out this trick.  since we're in control of the lifetime of the
implementation of the Interface, we can do things like use one instance for all
connections in cases (like this one) where that's appropriate

[tim (not reviewing), 2014-07-11 01:33:42]

nice quick work!

https://codereview.chromium.org/380413003/diff/40001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/interface_impl.h (right):

https://codereview.chromium.org/380413003/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/interface_impl.h:64: virtual void OnConnectionError() {
delete this; }
I might be missing something, but I think one thing this changes from the
ServiceConnection method is that it's easy to break the
InterfaceImplDeleteOnError behavior if the implementation overrides
OnConnectionError and forgets to call the super version.. right?   With
ServiceConnection that (somewhat subtly) wouldn't happen, because
ServiceConnection was the bottom of the chain.

[chromium-reviews, 2014-07-11 01:52:19]

Perhaps we could avoid that by making the InterfaceImplState be responsible
for deleting, after forwarding the OnConnectionError call to the instance()?
On Jul 10, 2014 6:33 PM, <tim@chromium.org> wrote:

> nice quick work!
>
>
> https://codereview.chromium.org/380413003/diff/40001/mojo/
> public/cpp/bindings/interface_impl.h
> File mojo/public/cpp/bindings/interface_impl.h (right):
>
> https://codereview.chromium.org/380413003/diff/40001/mojo/
> public/cpp/bindings/interface_impl.h#newcode64
> mojo/public/cpp/bindings/interface_impl.h:64: virtual void
> OnConnectionError() { delete this; }
> I might be missing something, but I think one thing this changes from
> the ServiceConnection method is that it's easy to break the
> InterfaceImplDeleteOnError behavior if the implementation overrides
> OnConnectionError and forgets to call the super version.. right?   With
> ServiceConnection that (somewhat subtly) wouldn't happen, because
> ServiceConnection was the bottom of the chain.
>
> https://codereview.chromium.org/380413003/
>

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

[darin (slow to review), 2014-07-11 03:52:35]

https://codereview.chromium.org/380413003/diff/40001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/interface_impl.h (right):

https://codereview.chromium.org/380413003/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/interface_impl.h:64: virtual void OnConnectionError() {
delete this; }
On 2014/07/11 01:33:41, tim (OOO until Jul 14) wrote:
> I might be missing something, but I think one thing this changes from the
> ServiceConnection method is that it's easy to break the
> InterfaceImplDeleteOnError behavior if the implementation overrides
> OnConnectionError and forgets to call the super version.. right?   With
> ServiceConnection that (somewhat subtly) wouldn't happen, because
> ServiceConnection was the bottom of the chain.

A template arg to InterfaceImpl<> would help solve this issue. InterfaceImplBase
could then implement the "delete on error" policy.

class FooImpl : public InterfaceImpl<Foo, DeleteOnError> {...};

[DaveMoore, 2014-07-11 14:38:53]

Some general comments:
- I made the ApplicationConnection an arg to the impl's constructor because I
thought that impls themselves would want to connect to other impls. We never
found an actual use for this however. And in some (most?) of those they could
get it from the ApplicationImpl by getting it as their Context and accessing the
service by url. I like that this change would make that optional; the class
could require a connection and in that case the BindToRequest() implementation
could pass it.

- It's nice that in the single service case that you can infer the interface
type from the argument.

- I remain concerned about the requirement to write boilerplate BindToRequest()
implementations everywhere.

- InterfaceProviderConnector looks like an autogened name. We should try to get
Controller or Manager in there somehow.

- I find InterfaceImplDeleteOnError pretty strange. Embedding a policy decision
like this at the end of the name of a class you inherit from hides it from view.

[jamesr, 2014-07-11 19:18:34]

On 2014/07/11 01:52:19, chromium-reviews wrote:
> Perhaps we could avoid that by making the InterfaceImplState be responsible
> for deleting, after forwarding the OnConnectionError call to the instance()?

I like this idea.  I'm still not sure if I want to make the deletion policy a
template parameter (part of the type) or a data member to make implementations
more flexible..

> On Jul 10, 2014 6:33 PM, <mailto:tim@chromium.org> wrote:
> 
> > nice quick work!
> >
> >
> > https://codereview.chromium.org/380413003/diff/40001/mojo/
> > public/cpp/bindings/interface_impl.h
> > File mojo/public/cpp/bindings/interface_impl.h (right):
> >
> > https://codereview.chromium.org/380413003/diff/40001/mojo/
> > public/cpp/bindings/interface_impl.h#newcode64
> > mojo/public/cpp/bindings/interface_impl.h:64: virtual void
> > OnConnectionError() { delete this; }
> > I might be missing something, but I think one thing this changes from
> > the ServiceConnection method is that it's easy to break the
> > InterfaceImplDeleteOnError behavior if the implementation overrides
> > OnConnectionError and forgets to call the super version.. right?   With
> > ServiceConnection that (somewhat subtly) wouldn't happen, because
> > ServiceConnection was the bottom of the chain.

Yes, that's the problem with implementation inheritance.  Every class in the
inheritance chain has to have a lot of faith that every other class is doing the
right thing.

> >
> > https://codereview.chromium.org/380413003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[jamesr, 2014-07-11 19:24:48]

On 2014/07/11 14:38:53, DaveMoore wrote:
> Some general comments:
> - I made the ApplicationConnection an arg to the impl's constructor because I
> thought that impls themselves would want to connect to other impls. We never
> found an actual use for this however. And in some (most?) of those they could
> get it from the ApplicationImpl by getting it as their Context and accessing
the
> service by url. I like that this change would make that optional; the class
> could require a connection and in that case the BindToRequest() implementation
> could pass it.

Actually, several service implementations use the ApplicationConnection today
(you can see those in the patch).  Not all do, so with this patch the
ServiceImpl doesn't have to be

> 
> - It's nice that in the single service case that you can infer the interface
> type from the argument.

In the AddServiceProvider call you can infer the interface type from the
argument if the argument inherits from exactly one InterfaceProvider<> type. 
With the old scheme you couldn't AddService<T> if T implemented multiple
Interfaces which was really annoying if your implementation of an interface
wanted to be a client of some other interface (since all mojom clients are also
Interfaces and thus have ::Name_)

> 
> - I remain concerned about the requirement to write boilerplate
BindToRequest()
> implementations everywhere.

We can easily layer a few provider implementations for common cases like
'allocate a new impl of type T and bind it', but I think having the bind be
explicit in the base interface is a huge plus in this and a huge minus in the
old scheme.  The old scheme successfully hide the bind so deeply that it's
nearly impossible to figure out what is going on.  With the bind exposed, we can
now separate the concerns of finding something to implement the interface and
binding that implementation to the pipe.  We can then compose things that take
care of one concern or both together instead of having them glued together.

> 
> - InterfaceProviderConnector looks like an autogened name. We should try to
get
> Controller or Manager in there somehow.
> 
> - I find InterfaceImplDeleteOnError pretty strange. Embedding a policy
decision
> like this at the end of the name of a class you inherit from hides it from
view.

Yeah this part needs more work.

[DaveMoore, 2014-07-11 19:32:26]

On 2014/07/11 19:24:48, jamesr wrote:
> On 2014/07/11 14:38:53, DaveMoore wrote:
> > Some general comments:
> > - I made the ApplicationConnection an arg to the impl's constructor because
I
> > thought that impls themselves would want to connect to other impls. We never
> > found an actual use for this however. And in some (most?) of those they
could
> > get it from the ApplicationImpl by getting it as their Context and accessing
> the
> > service by url. I like that this change would make that optional; the class
> > could require a connection and in that case the BindToRequest()
implementation
> > could pass it.
> 
> Actually, several service implementations use the ApplicationConnection today
> (you can see those in the patch).  Not all do, so with this patch the
> ServiceImpl doesn't have to be

Agreed. This is a plus.

> 
> > 
> > - It's nice that in the single service case that you can infer the interface
> > type from the argument.
> 
> In the AddServiceProvider call you can infer the interface type from the
> argument if the argument inherits from exactly one InterfaceProvider<> type. 
> With the old scheme you couldn't AddService<T> if T implemented multiple
> Interfaces which was really annoying if your implementation of an interface
> wanted to be a client of some other interface (since all mojom clients are
also
> Interfaces and thus have ::Name_)

Agreed.

> 
> > 
> > - I remain concerned about the requirement to write boilerplate
> BindToRequest()
> > implementations everywhere.
> 
> We can easily layer a few provider implementations for common cases like
> 'allocate a new impl of type T and bind it', but I think having the bind be
> explicit in the base interface is a huge plus in this and a huge minus in the
> old scheme.  The old scheme successfully hide the bind so deeply that it's
> nearly impossible to figure out what is going on.  With the bind exposed, we
can
> now separate the concerns of finding something to implement the interface and
> binding that implementation to the pipe.  We can then compose things that take
> care of one concern or both together instead of having them glued together.

Agreed that exposing the Bind is a win.

Maybe we should bundle lifetime management with the provider implementation.
Basically "use this impl and you get the old behavior". Or implement the method
yourself and handle everything.

> 
> > 
> > - InterfaceProviderConnector looks like an autogened name. We should try to
> get
> > Controller or Manager in there somehow.
> > 
> > - I find InterfaceImplDeleteOnError pretty strange. Embedding a policy
> decision
> > like this at the end of the name of a class you inherit from hides it from
> view.
> 
> Yeah this part needs more work.

[jamesr, 2014-07-11 20:07:12]

latest patchset shows a few InterfaceProvider impls that can handle many common
cases where you just want to allocate up a class of your impl time + bind it. 
We could provide a family of these for folks who want to use them but still let
them implement InterfaceProvider themselves if they need custom behavior.

I'm still thinking about error handling, so nothing exciting there yet.

https://codereview.chromium.org/380413003/diff/60001/mojo/services/network/ma...
File mojo/services/network/main.cc (right):

https://codereview.chromium.org/380413003/diff/60001/mojo/services/network/ma...
mojo/services/network/main.cc:60:
connection->AddServiceProvider(network_service_provider_.get());
here, instead of implementing InterfaceProvider<>::BindToRequest explicitly this
class allocates a ContextInterfaceProvider which allocates + binds instances of
NetworkServiceImpls. i'm doing this as a member in order to allocate it when the
context becomes available

https://codereview.chromium.org/380413003/diff/60001/mojo/services/view_manag...
File mojo/services/view_manager/view_manager_unittest.cc (right):

https://codereview.chromium.org/380413003/diff/60001/mojo/services/view_manag...
mojo/services/view_manager/view_manager_unittest.cc:369: public
DefaultInterfaceProvider<TestViewManagerClientConnection,
the EmbedServiceLoader just wants to allocate+bind
TestViewManagerClientConnection instances in response to requests for
ViewManagerClient, so i'm inheriting from the class to pick up that function

[jamesr, 2014-07-14 21:19:18]

PS6 specifies the ownership model / error handling behavior for the instance in
the bind call at Darin's suggestion.  This makes the ownership clear and
explicit at the callsite, which look either like:

  mojo::BindToRequest(new Impl, &request); // Strongly binds Impl to the pipe so
it is deleted when the pipe goes away

or

  mojo::WeakBindToRequest(&impl, &request); // Weakly binds Impl - the ownership
is independent of the pipe

The OnChannelError function fires on the interface in either case so
implementations can do bookkeeping regardless of what the ownership model is.

This is really nice since it means that the ownership is specified at the bind
call which is normally where the allocation is being made.  It also means that
the type of the interface implementation is independent of the ownership - i.e.
you can use the same type as a strongly-bound instance or allocated it out of an
arena or ref-count or whatever other ownership scheme is appropriate for a given
situation.

See mojo/shell/profile_service_loader.cc for one example of weak binding.

I'll convert more of the AddService<> calls over to make sure I'm handling all
of the cases but this looks pretty promising to me.

[jamesr, 2014-07-14 21:21:44]

https://codereview.chromium.org/380413003/diff/90001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/context_interface_provider.h (right):

https://codereview.chromium.org/380413003/diff/90001/mojo/public/cpp/applicat...
mojo/public/cpp/application/context_interface_provider.h:17: class
ContextInterfaceProvider : public InterfaceProvider<Interface> {
this and DefaultInterfaceProvider (better name welcome!) are intended as helpers
for common situations but aren't properly speaking part of the public interface

https://codereview.chromium.org/380413003/diff/90001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/interface_impl.h (right):

https://codereview.chromium.org/380413003/diff/90001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/interface_impl.h:22: typedef Interface Implements;
this is a little bit strange but without it I'm not sure how to specify the type
'Interface' given a type that inherits from InterfaceImpl<Interface> so that
callers don't have to explicitly specify both Impl and Interface in the common
case of a type that inherits from exactly one InterfaceImpl<T>.  You can see one
use of this in ContextInterfaceProvider.  Is there a better syntax for this?

[darin (slow to review), 2014-07-14 21:34:06]

some quick thoughts...

https://codereview.chromium.org/380413003/diff/90001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/interface_provider.h (right):

https://codereview.chromium.org/380413003/diff/90001/mojo/public/cpp/applicat...
mojo/public/cpp/application/interface_provider.h:25: virtual void
BindToRequest(ApplicationConnection* connection,
I'm still a fan of using terminology like "create instance" here. The
InterfaceProvider is being asked to create an instance of a particular
interface.

BindToRequest seems wrong as the function doesn't take the thing that should be
bound. The subject of the call is unspecified. You could say
CreateInstanceAndBindToRequest. (What about if they want to use
WeakBindToRequest instead?)

I might even consider going further and renaming InterfaceProvider to
InterfaceFactory. It really is a factory of sorts.

I'm not so bothered by "Create*" normally implying a method that returns a heap
allocated object. If you think about a .mojom file, you might use "Create*" to
name functions that look very much like what I'm proposing.

  interface Foo {
    CreateBar(Bar& bar);
  };

https://codereview.chromium.org/380413003/diff/90001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/interface_impl.h (right):

https://codereview.chromium.org/380413003/diff/90001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/interface_impl.h:70: Impl* BindToPipe(
Instead of exposing the enum (which is a bit awkwardly named), how about
introducing WeakBindToPipe? Then, you can hide the enum completely. I think the
Bind method on internal_state() can just take a bool param, named
delete_on_error.

[DaveMoore, 2014-07-15 00:08:29]

https://codereview.chromium.org/380413003/diff/60001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/interface_provider.h (right):

https://codereview.chromium.org/380413003/diff/60001/mojo/public/cpp/applicat...
mojo/public/cpp/application/interface_provider.h:30: // response to requests for
interface Interface.
Can't you merge these two with a third template argument, and pick it up
automatically if passed to the constructor?

https://codereview.chromium.org/380413003/diff/90001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/default_interface_provider.h (right):

https://codereview.chromium.org/380413003/diff/90001/mojo/public/cpp/applicat...
mojo/public/cpp/application/default_interface_provider.h:15: class
DefaultInterfaceProvider : public InterfaceProvider<Interface> {
Default is still wrong, unless it's used as a default by some higher level code.
SimpleInterfaceProvider maybe?

https://codereview.chromium.org/380413003/diff/90001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/interface_provider.h (right):

https://codereview.chromium.org/380413003/diff/90001/mojo/public/cpp/applicat...
mojo/public/cpp/application/interface_provider.h:25: virtual void
BindToRequest(ApplicationConnection* connection,
I see the Interface as an abstract definition and Service as a concrete
implementation of an Interface. So this thing isn't really providing an
Interface. It's providing an implementation.

On 2014/07/14 21:34:06, darin wrote:
> I'm still a fan of using terminology like "create instance" here. The
> InterfaceProvider is being asked to create an instance of a particular
> interface.
> 
> BindToRequest seems wrong as the function doesn't take the thing that should
be
> bound. The subject of the call is unspecified. You could say
> CreateInstanceAndBindToRequest. (What about if they want to use
> WeakBindToRequest instead?)
> 
> I might even consider going further and renaming InterfaceProvider to
> InterfaceFactory. It really is a factory of sorts.
> 
> I'm not so bothered by "Create*" normally implying a method that returns a
heap
> allocated object. If you think about a .mojom file, you might use "Create*" to
> name functions that look very much like what I'm proposing.
> 
>   interface Foo {
>     CreateBar(Bar& bar);
>   };

https://codereview.chromium.org/380413003/diff/90001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/error_handler.h (right):

https://codereview.chromium.org/380413003/diff/90001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/error_handler.h:14: enum OnConnectionErrorBehavior {
DO_NOTHING, DELETE_ON_ERROR };
DO_NOTHING / DELETE or IGNORE_ERROR / DELETE_ON_ERROR

https://codereview.chromium.org/380413003/diff/90001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/interface_impl_internal.h (right):

https://codereview.chromium.org/380413003/diff/90001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/interface_impl_internal.h:94: if (behavior ==
DELETE_ON_ERROR) {
Nit: no braces

https://codereview.chromium.org/380413003/diff/90001/mojo/services/surfaces/s...
File mojo/services/surfaces/surfaces_impl.h (right):

https://codereview.chromium.org/380413003/diff/90001/mojo/services/surfaces/s...
mojo/services/surfaces/surfaces_impl.h:38: uint32_t id_namespace,
Are these changes related to this cl?

[jamesr, 2014-07-15 00:33:10]

PS8 converts everything over to using an InterfaceFactory and gets rid of
ApplicationConnection::AddService().

https://codereview.chromium.org/380413003/diff/90001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/default_interface_provider.h (right):

https://codereview.chromium.org/380413003/diff/90001/mojo/public/cpp/applicat...
mojo/public/cpp/application/default_interface_provider.h:15: class
DefaultInterfaceProvider : public InterfaceProvider<Interface> {
On 2014/07/15 00:08:29, DaveMoore wrote:
> Default is still wrong, unless it's used as a default by some higher level
code.
> SimpleInterfaceProvider maybe?

There's only one use of this class, so it's not really worth having this
boilerplate.  I merged the implementation into the only user

https://codereview.chromium.org/380413003/diff/90001/mojo/services/surfaces/s...
File mojo/services/surfaces/surfaces_impl.h (right):

https://codereview.chromium.org/380413003/diff/90001/mojo/services/surfaces/s...
mojo/services/surfaces/surfaces_impl.h:38: uint32_t id_namespace,
On 2014/07/15 00:08:29, DaveMoore wrote:
> Are these changes related to this cl?

Yes, using InterfaceProvider allows greatly simplifying how this class is
constructed.  Take a look at surfaces_service_loader before and after

[darin (slow to review), 2014-07-15 06:10:38]

Looks fantastic!

https://codereview.chromium.org/380413003/diff/130001/mojo/examples/embedded_...
File mojo/examples/embedded_app/embedded_app.cc (right):

https://codereview.chromium.org/380413003/diff/130001/mojo/examples/embedded_...
mojo/examples/embedded_app/embedded_app.cc:83:
connection->AddServiceFactory(&view_manager_client_factory_);
I really like how you are making folks call AddServiceFactory for the view
manager. It makes things much clearer.

https://codereview.chromium.org/380413003/diff/130001/mojo/public/cpp/applica...
File mojo/public/cpp/application/application_connection.h (right):

https://codereview.chromium.org/380413003/diff/130001/mojo/public/cpp/applica...
mojo/public/cpp/application/application_connection.h:25: // Then implement a
InterfaceFactory<Foo> that binds instances of FooImpl to
nit: "a" -> "an"

https://codereview.chromium.org/380413003/diff/130001/mojo/public/cpp/applica...
mojo/public/cpp/application/application_connection.h:40: void
AddServiceFactory(InterfaceFactory<Interface>* factory) {
you probably want to say somewhere that the InterfaceFactory<> instance has to
out-live the ApplicationConnection. hopefully we won't need to deal with
InterfaceFactory<> instances that need shorter than ApplicationDelegate
lifetimes.

https://codereview.chromium.org/380413003/diff/130001/mojo/public/cpp/applica...
File mojo/public/cpp/application/context_interface_factory.h (right):

https://codereview.chromium.org/380413003/diff/130001/mojo/public/cpp/applica...
mojo/public/cpp/application/context_interface_factory.h:13: // context parameter
response to requests for interface Interface.
nit: seems like a period belongs after "parameter", but then I'm having trouble
parsing the remaining bit of the comment.

https://codereview.chromium.org/380413003/diff/130001/mojo/public/cpp/applica...
mojo/public/cpp/application/context_interface_factory.h:17: class
ContextInterfaceFactory : public InterfaceFactory<Interface> {
nit: How about renaming this to InterfaceFactoryWithContext?
ContextInterfaceFactory sounds like the name of a "factory of context
interfaces", but I'm not sure what a "context interface" is. This class is
constructed with a Context* parameter, so saying that it is an "interface
factory with context" makes a bit of sense.

https://codereview.chromium.org/380413003/diff/130001/mojo/public/cpp/applica...
File mojo/public/cpp/application/interface_factory.h (right):

https://codereview.chromium.org/380413003/diff/130001/mojo/public/cpp/applica...
mojo/public/cpp/application/interface_factory.h:14: template <typename
Interface>
nit: maybe it is just a personal preference, but i like not putting a line break
when forward declaring a template type so that the line below doesn't look like
a non-template forward declaration at first glance. or, alternatively put a new
line above the "template <..>" line.

https://codereview.chromium.org/380413003/diff/130001/mojo/public/cpp/applica...
File mojo/public/cpp/application/lib/service_connector.h (right):

https://codereview.chromium.org/380413003/diff/130001/mojo/public/cpp/applica...
mojo/public/cpp/application/lib/service_connector.h:48:
factory_->Create(application_connection_, request.Pass());
note: you can also use MakeRequest<Interface>(client_handle.Pass()) here if you
like:

  factory_->Create(application_connection_,
                   MakeRequest<Interface>(client_handle.Pass()));

https://codereview.chromium.org/380413003/diff/130001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/interface_impl.h (right):

https://codereview.chromium.org/380413003/diff/130001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/interface_impl.h:22: typedef Interface Implements;
The name of this typedef bugs me a bit. Interface is a noun, but Implements is a
verb. A more verbose choice might be ImplementedInterface. Given how
infrequently this is used, maybe that is the way to go?

Another option is to just rename the template arg and let the typedef be
Interface. "The interface of the InterfaceImpl is InterfaceImpl::Interface."
That sort of works too.

https://codereview.chromium.org/380413003/diff/130001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/interface_impl.h:100: Impl* BindToProxy(
I guess this should be renamed WeakBindToProxy. Maybe we should have both
BindToProxy (strong ownership of instance) and WeakBindToProxy?

https://codereview.chromium.org/380413003/diff/130001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/interface_impl.h:131: // not bound to the pipe. If the
pipe is closed, the instance's OnChannelError
nit: OnChannelError -> OnConnectionError

https://codereview.chromium.org/380413003/diff/130001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/interface_impl_internal.h (right):

https://codereview.chromium.org/380413003/diff/130001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/interface_impl_internal.h:16: 
nit: no new line here is OK, right?

https://codereview.chromium.org/380413003/diff/130001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/interface_impl_internal.h:94: if (delete_on_error)
It might be useful to add some debug assertions that OnConnectionError() doesn't
self-delete when it shouldn't. This could be accomplished with a pointer to bool
member variable that we set to a stack variable inside OnConnectionError before
calling out to the instance.

https://codereview.chromium.org/380413003/diff/130001/mojo/services/native_vi...
File mojo/services/native_viewport/native_viewport_service.cc (right):

https://codereview.chromium.org/380413003/diff/130001/mojo/services/native_vi...
mojo/services/native_viewport/native_viewport_service.cc:159: public
ContextInterfaceFactory<NativeViewportImpl, mojo::shell::Context> {
nit: you are already inside the mojo:: namespace, so no need for that here.

https://codereview.chromium.org/380413003/diff/130001/mojo/services/network/m...
File mojo/services/network/main.cc (right):

https://codereview.chromium.org/380413003/diff/130001/mojo/services/network/m...
mojo/services/network/main.cc:37: mojo::NetworkContext> NetworkProvider;
nit: NetworkProvider -> NetworkFactory?

https://codereview.chromium.org/380413003/diff/130001/mojo/services/surfaces/...
File mojo/services/surfaces/surfaces_service_application.h (right):

https://codereview.chromium.org/380413003/diff/130001/mojo/services/surfaces/...
mojo/services/surfaces/surfaces_service_application.h:17: class Surfaces;
nit: looks like this forward decl isn't needed.

[jamesr, 2014-07-15 18:07:23]

Thanks for the review!  I still have (at least) one lifetime issue related to
ViewManagerClientImpl that I need to sort out but most other tests pass now.

https://codereview.chromium.org/380413003/diff/130001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/interface_impl.h (right):

https://codereview.chromium.org/380413003/diff/130001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/interface_impl.h:22: typedef Interface Implements;
On 2014/07/15 06:10:38, darin wrote:
> The name of this typedef bugs me a bit. Interface is a noun, but Implements is
a
> verb. A more verbose choice might be ImplementedInterface. Given how
> infrequently this is used, maybe that is the way to go?

yeah, it's used in two places total so I went with ImplementedInterface.  I like
having the template arg be called 'Interface' since that's what most folks will
see.
> 
> Another option is to just rename the template arg and let the typedef be
> Interface. "The interface of the InterfaceImpl is InterfaceImpl::Interface."
> That sort of works too.

https://codereview.chromium.org/380413003/diff/130001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/interface_impl.h:100: Impl* BindToProxy(
On 2014/07/15 06:10:38, darin wrote:
> I guess this should be renamed WeakBindToProxy. Maybe we should have both
> BindToProxy (strong ownership of instance) and WeakBindToProxy?

Turns out I do need a WeakBindToProxy so I made this version strong and added
that

https://codereview.chromium.org/380413003/diff/130001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/interface_impl_internal.h (right):

https://codereview.chromium.org/380413003/diff/130001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/interface_impl_internal.h:94: if (delete_on_error)
On 2014/07/15 06:10:38, darin wrote:
> It might be useful to add some debug assertions that OnConnectionError()
doesn't
> self-delete when it shouldn't. This could be accomplished with a pointer to
bool
> member variable that we set to a stack variable inside OnConnectionError
before
> calling out to the instance.

I added a !NDEBUG-only boolean member that I set before this delete and assert()
that it's set if delete_on_error_ is set in the destructor of this class.  It's
caught some issues locally already, so it's definitely useful

[darin (slow to review), 2014-07-15 18:43:15]

On 2014/07/15 18:07:23, jamesr wrote:
> Thanks for the review!  I still have (at least) one lifetime issue related to
> ViewManagerClientImpl that I need to sort out but most other tests pass now.
> 
>
https://codereview.chromium.org/380413003/diff/130001/mojo/public/cpp/binding...
> File mojo/public/cpp/bindings/interface_impl.h (right):
> 
>
https://codereview.chromium.org/380413003/diff/130001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/interface_impl.h:22: typedef Interface Implements;
> On 2014/07/15 06:10:38, darin wrote:
> > The name of this typedef bugs me a bit. Interface is a noun, but Implements
is
> a
> > verb. A more verbose choice might be ImplementedInterface. Given how
> > infrequently this is used, maybe that is the way to go?
> 
> yeah, it's used in two places total so I went with ImplementedInterface.  I
like
> having the template arg be called 'Interface' since that's what most folks
will
> see.

OK

> > 
> > Another option is to just rename the template arg and let the typedef be
> > Interface. "The interface of the InterfaceImpl is InterfaceImpl::Interface."
> > That sort of works too.
> 
>
https://codereview.chromium.org/380413003/diff/130001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/interface_impl.h:100: Impl* BindToProxy(
> On 2014/07/15 06:10:38, darin wrote:
> > I guess this should be renamed WeakBindToProxy. Maybe we should have both
> > BindToProxy (strong ownership of instance) and WeakBindToProxy?
> 
> Turns out I do need a WeakBindToProxy so I made this version strong and added
> that

OK

> 
>
https://codereview.chromium.org/380413003/diff/130001/mojo/public/cpp/binding...
> File mojo/public/cpp/bindings/lib/interface_impl_internal.h (right):
> 
>
https://codereview.chromium.org/380413003/diff/130001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/lib/interface_impl_internal.h:94: if
(delete_on_error)
> On 2014/07/15 06:10:38, darin wrote:
> > It might be useful to add some debug assertions that OnConnectionError()
> doesn't
> > self-delete when it shouldn't. This could be accomplished with a pointer to
> bool
> > member variable that we set to a stack variable inside OnConnectionError
> before
> > calling out to the instance.
> 
> I added a !NDEBUG-only boolean member that I set before this delete and
assert()
> that it's set if delete_on_error_ is set in the destructor of this class. 
It's
> caught some issues locally already, so it's definitely useful

Sounds good.

LGTM

[darin (slow to review), 2014-07-15 18:44:08]

https://codereview.chromium.org/380413003/diff/150001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/interface_impl.h (right):

https://codereview.chromium.org/380413003/diff/150001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/interface_impl.h:110: // Same as BindToProxy but does
not delete instance after a channel error.
nit: it'd be good to standardize on a comment for WeakBindTo* and use it on all
three.

[jamesr, 2014-07-16 06:56:58]

There's a tricky issue here demonstrated by service_manager_unittests.  Since
the ApplicationConnection no longer retains ownership of the impls (it can't
since it doesn't seem the impls, just InterfaceRequests), it's not clear how to
shut down everything associated with a connection when the app goes away.  The
ServiceManagerTest fixture shows this problem in a few ways:

1.) Several of the tests bind Test[AB]Impls with a pointer to an object
(TesterContext) allocated on the stack in the test body.  Since these impls no
longer go away with the loader synchronously, they corrupt stack memory in their
destructors.  This is pretty easy to paper over by making the TesterContext
outlive the message loop so the error handlers can run but..

2.) ServiceManagerTest.BDeleted assumes that when it closes the application (by
resetting the loader which has a scoped_ptr<ApplicationImpl> that this will
close out the pipes to all services connected to the application.  I'm not sure
exactly how this should happen.

I'm out tomorrow (wednesday) but will be back on this Thursday.  Any pointers
greatly appreciated!

[tim (not reviewing), 2014-07-16 17:57:10]

Re: your second question: Before what was happening was that ServiceConnector
was acting as a Factory, *and* was also keeping track of every instance it
created so that it could delete them when the owning ApplicationConnection went
out of scope. Deleting that instance would destroy the InterfaceImplState's
router, which is what wound up closing the pipes. 

You can't do that now because the InterfaceImpl might delete itself
OnConnectionError and your factory would be none the wiser; it would have no
idea whether the pointers it was tracking were already garbage or not.  

One option that keeps it similar to before might be to have ~InterfaceImplState
(or, maybe better, ~ServiceRegistry) notify the tracking registry (factory) that
it is being destroyed. The factory could then remove that instance from it's
tracking list to avoid double-free.

https://codereview.chromium.org/380413003/diff/150001/mojo/public/cpp/applica...
File mojo/public/cpp/application/application_connection.h (right):

https://codereview.chromium.org/380413003/diff/150001/mojo/public/cpp/applica...
mojo/public/cpp/application/application_connection.h:36: // The InterfaceFactory
must outlive the ApplicationConnection.
I'm a bit confused here. I don't see any changes to ServiceRegistry in this
patch, which means that the ServiceConnectorBase that is passed to
AddServiceConnector is actually *owned* by the ApplicationConnection.

https://codereview.chromium.org/380413003/diff/150001/mojo/public/cpp/applica...
File mojo/public/cpp/application/lib/service_connector.h (right):

https://codereview.chromium.org/380413003/diff/150001/mojo/public/cpp/applica...
mojo/public/cpp/application/lib/service_connector.h:23: virtual
~ServiceConnectorBase();
Are these defined anywhere?

[jamesr, 2014-07-17 21:49:43]

On 2014/07/16 17:57:10, timsteele wrote:
> Re: your second question: Before what was happening was that ServiceConnector
> was acting as a Factory, *and* was also keeping track of every instance it
> created so that it could delete them when the owning ApplicationConnection
went
> out of scope. Deleting that instance would destroy the InterfaceImplState's
> router, which is what wound up closing the pipes. 

Aha! The router is the bit that I was missing.

> 
> You can't do that now because the InterfaceImpl might delete itself
> OnConnectionError and your factory would be none the wiser; it would have no
> idea whether the pointers it was tracking were already garbage or not.

Right.  I think that instead of tracking interfaces we probably should be
tracking routers and maybe unhook those from InterfaceImpls when the application
goes away.

> 
> One option that keeps it similar to before might be to have
~InterfaceImplState
> (or, maybe better, ~ServiceRegistry) notify the tracking registry (factory)
that
> it is being destroyed. The factory could then remove that instance from it's
> tracking list to avoid double-free.

[jamesr, 2014-07-17 22:22:50]

After talking about this with Dave we decided that we probably want to have two
behaviors:

1.) If your application wants to weakly bind to the pipe, then it is responsible
for deleting all of the interface implementations (probably in response to an
ApplicationDelegate::Shutdown() call from Tim's shutdown doc).

2.) If your application wants to strongly bind to a pipe, we should additionally
do some bookkeeping at the application level to destroy your impls when the
application is shutting down.  That way for the common case you don't have to
write any additional code.

The /bindings/ vs /application/ split makes this a bit more complicated.  Maybe
the strong bind needs to live in /application/ and the /bindings/ code will only
deal with weak binding, since it doesn't know about shutdown?  I'll try some
things out and see if I end up with piles of goo.

Thanks again for the comments, they're really helping me understand this stuff
better.

https://codereview.chromium.org/380413003/diff/150001/mojo/public/cpp/applica...
File mojo/public/cpp/application/application_connection.h (right):

https://codereview.chromium.org/380413003/diff/150001/mojo/public/cpp/applica...
mojo/public/cpp/application/application_connection.h:36: // The InterfaceFactory
must outlive the ApplicationConnection.
On 2014/07/16 17:57:09, timsteele wrote:
> I'm a bit confused here. I don't see any changes to ServiceRegistry in this
> patch, which means that the ServiceConnectorBase that is passed to
> AddServiceConnector is actually *owned* by the ApplicationConnection.

Right, and the service connector maintains a raw pointer to the
InterfaceFactory.  Thus the InterfaceFactory has to live at least as long as the
ApplicationConnection.  If we need a way to have the InterfaceFactory die first
then it needs to somehow make sure that the ApplicationConnection does not
retain a raw pointer to it (i.e. add a RemoveService() or something like that).

https://codereview.chromium.org/380413003/diff/150001/mojo/public/cpp/applica...
File mojo/public/cpp/application/lib/service_connector.h (right):

https://codereview.chromium.org/380413003/diff/150001/mojo/public/cpp/applica...
mojo/public/cpp/application/lib/service_connector.h:23: virtual
~ServiceConnectorBase();
On 2014/07/16 17:57:09, timsteele wrote:
> Are these defined anywhere?

Yes, in service_connector_base.cc

[jamesr, 2014-07-18 06:48:44]

darin@ - PTAL.  I probably need some more spit+polish in a few places but
overall I'm getting happy with this.

PS10 redoes things a little bit again but this one actually passes tests which
is pretty nice.  The main change is to support the (fairly reasonable) behavior
that when an application is shut down all strongly-bound implementations should
probably be torn down as well.  In order to do this, the patch introduces a
ServiceFactory<> and ServiceRequest<> to provide the strong binding behavior. 
The ServiceRequest<> is just like an InterfaceRequest<> except that when bound
to strongly (which is the default) it also registers itself with the
ApplicationConnection's implementation in order to be notified on shutdown. 
Code that binds weakly to a ServiceRequest<> or that binds directly to a
pipe/proxy/InterfaceRequest has to manage its own lifetime

After looking at this more I don't think it makes as much sense for a
mojo::BindToPipe to bind its lifetime to the underlying pipe since there's no
real way to write shutdown code unless the implementation keeps track of all of
the pipes it has bound to at which point it might as well keep track of the
instances as well.  To do this lifetime bracketing we need some higher level
notion of 'these things are associated with each other and can be shut down at
the same time' which is what ApplicationConnection gives you.

One perhaps subtle thing to note here is that if there are no pipe errors before
shutdown all of the impls will be deleted on the thread that the ApplicationImpl
is destroyed on (which is the same as the previous behavior) but if a pipe error
occurs the impl will be destroyed on the thread that the pipe error is noticed
on (i.e. the thread that the impl is bound to).  This is the same with and
without this patch but might be worth looking at more closely.

[darin (slow to review), 2014-07-21 21:15:28]

On 2014/07/18 06:48:44, jamesr wrote:
> darin@ - PTAL.  I probably need some more spit+polish in a few places but
> overall I'm getting happy with this.
> 
> PS10 redoes things a little bit again but this one actually passes tests which
> is pretty nice.  The main change is to support the (fairly reasonable)
behavior
> that when an application is shut down all strongly-bound implementations
should
> probably be torn down as well.  In order to do this, the patch introduces a
> ServiceFactory<> and ServiceRequest<> to provide the strong binding behavior. 
> The ServiceRequest<> is just like an InterfaceRequest<> except that when bound
> to strongly (which is the default) it also registers itself with the
> ApplicationConnection's implementation in order to be notified on shutdown. 
> Code that binds weakly to a ServiceRequest<> or that binds directly to a
> pipe/proxy/InterfaceRequest has to manage its own lifetime

Hmm... I was really hoping we could avoid making a strong distinction between
service requests and general interface requests. It seems like general interface
requests might want to similarly have their lifetime bound to the pipe <or> the
App. That seems like a generally useful thing.


> After looking at this more I don't think it makes as much sense for a
> mojo::BindToPipe to bind its lifetime to the underlying pipe since there's no
> real way to write shutdown code unless the implementation keeps track of all
of
> the pipes it has bound to at which point it might as well keep track of the
> instances as well.  To do this lifetime bracketing we need some higher level
> notion of 'these things are associated with each other and can be shut down at
> the same time' which is what ApplicationConnection gives you.
> 
> One perhaps subtle thing to note here is that if there are no pipe errors
before
> shutdown all of the impls will be deleted on the thread that the
ApplicationImpl
> is destroyed on (which is the same as the previous behavior) but if a pipe
error
> occurs the impl will be destroyed on the thread that the pipe error is noticed
> on (i.e. the thread that the impl is bound to).  This is the same with and
> without this patch but might be worth looking at more closely.

Hmm... The binding system is not thread-safe. When you bind an instance to a
pipe,
you'd better only use that instance on that same thread. If instances are bound
on
background threads, then we could have a real problem. It sounds like service
requests would never be bound on background threads? (We might actually want
that
capability in the future, but it seems like something we can punt on for now.)

[jamesr, 2014-07-22 00:46:21]

After talking at this with Darin some more we're thinking that adding this layer
of handle tracking in addition to the tracking that handlewatcher does as a
message loop destruction observer (and runloop could do, in theory) isn't all
that valuable since any real application should be running on its own thread. 
So, instead, I'll go back to an earlier version with an InterfaceFactory and the
bind/weakbind semantics on public/cpp/bindings/interface_impl.h.  This means
that impls bound to InterfaceRequest<>s can get the same lifetime semantics
without needing to register with any public/cpp/application/ logic.

This means that all handles being waited on will fire their OnConnectionError
when the message loop is destroyed.  The shutdown sequence will be:

1.) Tear down the MessageLoop (or RunLoop) and fire connection error handlers on
all pipes being waited on in the thread
2.) Tear down the ApplicationDelegate
3.) Stop the thread running the application main
4.) Unload the library

Several of the service_manager_unittests try to load up multiple apps on the
same thread and shut them down independently, but that won't work in this scheme
and is pretty unrealistic.  We can probably rewrite these to load up apps on
different threads and then shut down the threads, but that could be a bit of a
pain.  I'll see how bad it looks but think it might make sense to land this
without all of those tests fully converted since right now we're
double-freeing/use-after-freeing quite a bit in the unit tests which makes it
hard to enable the tests on more bots.

[darin (slow to review), 2014-07-22 03:36:25]

On 2014/07/22 00:46:21, jamesr wrote:
> After talking at this with Darin some more we're thinking that adding this
layer
> of handle tracking in addition to the tracking that handlewatcher does as a
> message loop destruction observer (and runloop could do, in theory) isn't all
> that valuable since any real application should be running on its own thread. 
> So, instead, I'll go back to an earlier version with an InterfaceFactory and
the
> bind/weakbind semantics on public/cpp/bindings/interface_impl.h.  This means
> that impls bound to InterfaceRequest<>s can get the same lifetime semantics
> without needing to register with any public/cpp/application/ logic.
> 
> This means that all handles being waited on will fire their OnConnectionError
> when the message loop is destroyed.  The shutdown sequence will be:
> 
> 1.) Tear down the MessageLoop (or RunLoop) and fire connection error handlers
on
> all pipes being waited on in the thread
> 2.) Tear down the ApplicationDelegate
> 3.) Stop the thread running the application main
> 4.) Unload the library
> 
> Several of the service_manager_unittests try to load up multiple apps on the
> same thread and shut them down independently, but that won't work in this
scheme
> and is pretty unrealistic.  We can probably rewrite these to load up apps on
> different threads and then shut down the threads, but that could be a bit of a
> pain.  I'll see how bad it looks but think it might make sense to land this
> without all of those tests fully converted since right now we're
> double-freeing/use-after-freeing quite a bit in the unit tests which makes it
> hard to enable the tests on more bots.

As mentioned in person, I'm OK disabling these tests temporarily. This CL is
already quite large and complex. I don't see a problem with re-working the
tests in a follow-up CL.

-Darin

[darin (slow to review), 2014-07-22 21:25:47]

LGTM

https://codereview.chromium.org/380413003/diff/210001/mojo/shell/network_serv...
File mojo/shell/network_service_loader.h (right):

https://codereview.chromium.org/380413003/diff/210001/mojo/shell/network_serv...
mojo/shell/network_service_loader.h:20: class ProfileService;
nit: this class no longer exists

[darin (slow to review), 2014-07-22 21:26:52]

https://codereview.chromium.org/380413003/diff/230001/mojo/public/cpp/applica...
File mojo/public/cpp/application/lib/mojo_main_standalone.cc (right):

https://codereview.chromium.org/380413003/diff/230001/mojo/public/cpp/applica...
mojo/public/cpp/application/lib/mojo_main_standalone.cc:15: // We have to shut
down the RunLoop before destroying the
I think you want similar treatment in mojo_main_chromium.cc.

[jamesr, 2014-07-22 22:58:26]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2014-07-22 22:58:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamesr@chromium.org/380413003/250001

[jamesr, 2014-07-22 23:42:21]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2014-07-22 23:44:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamesr@chromium.org/380413003/270001

[jamesr, 2014-07-23 01:19:55]

The CQ bit was unchecked by jamesr@chromium.org

[jamesr, 2014-07-23 01:19:55]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2014-07-23 01:20:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamesr@chromium.org/380413003/270001

[commit-bot: I haz the power, 2014-07-23 02:47:31]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-07-23 03:27:32]

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

[commit-bot: I haz the power, 2014-07-23 03:27:34]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)

[jamesr, 2014-07-23 04:20:16]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2014-07-23 04:23:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamesr@chromium.org/380413003/290001

[jamesr, 2014-07-23 05:11:35]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2014-07-23 05:12:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamesr@chromium.org/380413003/310001

[commit-bot: I haz the power, 2014-07-23 10:00:36]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-07-23 10:20:45]

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

[commit-bot: I haz the power, 2014-07-23 10:20:49]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[jamesr, 2014-07-23 17:23:22]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2014-07-23 17:24:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamesr@chromium.org/380413003/310001

[commit-bot: I haz the power, 2014-07-23 18:07:50]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-07-23 19:06:19]

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

[commit-bot: I haz the power, 2014-07-23 19:06:20]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[jamesr, 2014-07-23 19:50:22]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2014-07-23 19:53:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamesr@chromium.org/380413003/330001

[jamesr, 2014-07-23 20:10:02]

The CQ bit was unchecked by jamesr@chromium.org

[jamesr, 2014-07-23 20:17:17]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2014-07-23 20:20:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamesr@chromium.org/380413003/350001

[commit-bot: I haz the power, 2014-07-24 03:26:43]

Change committed as 285139

[tim (not reviewing), 2014-07-24 04:03:33]

congrats!
On Jul 23, 2014 8:26 PM, <commit-bot@chromium.org> wrote:

> Change committed as 285139
>
> https://codereview.chromium.org/380413003/
>

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