Mojo: Add ErrorHandler to RemotePtr

Mojo: Add ErrorHandler to RemotePtr

BUG=325321
R=davemoore@chromium.org

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

[DaveMoore, 2014-01-31 00:28:02]

https://codereview.chromium.org/150713002/diff/40001/mojo/public/bindings/err...
File mojo/public/bindings/error_handler.h (right):

https://codereview.chromium.org/150713002/diff/40001/mojo/public/bindings/err...
mojo/public/bindings/error_handler.h:10: class ErrorHandler {
Is this intended to be the interface we use for all Errors, or just bindings? If
the later, the naming should be more specific.

https://codereview.chromium.org/150713002/diff/40001/mojo/public/bindings/err...
mojo/public/bindings/error_handler.h:13: virtual void OnError() = 0;
If we want an instance to watch for errors on multiple connections we need to
pass something to indicate where the error is.

https://codereview.chromium.org/150713002/diff/40001/mojo/public/bindings/lib...
File mojo/public/bindings/lib/connector.cc (right):

https://codereview.chromium.org/150713002/diff/40001/mojo/public/bindings/lib...
mojo/public/bindings/lib/connector.cc:60: if (result == MOJO_RESULT_OK) {
Nit: no need for braces.

https://codereview.chromium.org/150713002/diff/40001/mojo/public/bindings/lib...
File mojo/public/bindings/lib/connector.h (right):

https://codereview.chromium.org/150713002/diff/40001/mojo/public/bindings/lib...
mojo/public/bindings/lib/connector.h:32: ErrorHandler* error_handler = NULL,
Why pass it as an arg instead of something you can set? Can errors happen during
construction? It might even be better to allow ErrorObservers instead of just
one handler (unless there's some action you expect the handler to take).

[darin (slow to review), 2014-01-31 00:41:12]

https://codereview.chromium.org/150713002/diff/40001/mojo/public/bindings/err...
File mojo/public/bindings/error_handler.h (right):

https://codereview.chromium.org/150713002/diff/40001/mojo/public/bindings/err...
mojo/public/bindings/error_handler.h:10: class ErrorHandler {
On 2014/01/31 00:28:02, DaveMoore wrote:
> Is this intended to be the interface we use for all Errors, or just bindings?
If
> the later, the naming should be more specific.

Just the bindings for now. It is where it lives. I haven't considered other use
cases in the mojo:: namespace. Can you think of any?

This is where having something like base::Bind would be handy ;-)

https://codereview.chromium.org/150713002/diff/40001/mojo/public/bindings/err...
mojo/public/bindings/error_handler.h:13: virtual void OnError() = 0;
On 2014/01/31 00:28:02, DaveMoore wrote:
> If we want an instance to watch for errors on multiple connections we need to
> pass something to indicate where the error is.

Hmm, I hadn't considered that use case. I was just thinking about what we need
for Service<>. For Service<>, this interface is sufficient. I can send you
another CL of what I was imagining for Service<>, but it would basically be
Service<> implementing this interface and on error, it would initiate the
destruction of |this|.

https://codereview.chromium.org/150713002/diff/40001/mojo/public/bindings/lib...
File mojo/public/bindings/lib/connector.cc (right):

https://codereview.chromium.org/150713002/diff/40001/mojo/public/bindings/lib...
mojo/public/bindings/lib/connector.cc:60: if (result == MOJO_RESULT_OK) {
On 2014/01/31 00:28:02, DaveMoore wrote:
> Nit: no need for braces.

I always add braces when there is an else clause. It seems the style guide is
not prescriptive here.

https://codereview.chromium.org/150713002/diff/40001/mojo/public/bindings/lib...
File mojo/public/bindings/lib/connector.h (right):

https://codereview.chromium.org/150713002/diff/40001/mojo/public/bindings/lib...
mojo/public/bindings/lib/connector.h:32: ErrorHandler* error_handler = NULL,
On 2014/01/31 00:28:02, DaveMoore wrote:
> Why pass it as an arg instead of something you can set? Can errors happen
during
> construction? It might even be better to allow ErrorObservers instead of just
> one handler (unless there's some action you expect the handler to take).

True, it would be fine to add a setter for this on Connector. SetErrorHandler to
mirror SetIncomingReceiver.

I don't want to have a similar SetErrorHandler on RemotePtr though since that
could lead to user confusion. If they call .reset() on RemotePtr, then they need
to understand that any ErrorHandler that was previously set would have been
lost. Forcing ErrorHandler as a ctor arg to RemotePtr or as an arg to .reset()
helps avoid this confusion.

I'm not sure of a use case for multiple observers. If we need that, then we
could implement an ErrorHandler that has a list of ErrorHandlers to broadcast
the event to.

[DaveMoore, 2014-01-31 00:45:14]

lgtm

[darin (slow to review), 2014-01-31 00:53:39]

Committed patchset #4 manually as r248094 (presubmit successful).