Return errors from the Mojo network service using an enum.

Return errors from the Mojo network service using an enum.

This allows consumers of the network service to act upon returned error
codes without referring to //net/base/net_errors.h.

Of course, this is essentially a fork of net/base/net_error_list.h that
we should keep in sync with the original. But at least we're handling
this within the network service, once for all concumers.

BUG=456130

[ppi, 2015-02-17 15:45:51]

ppi@chromium.org changed reviewers:
+ jamesr@chromium.org

[ppi, 2015-02-17 15:48:27]

Hi James,

wdyt?

[jamesr, 2015-02-18 18:43:07]

lgtm

https://codereview.chromium.org/933973002/diff/20001/mojo/services/network/ne...
File mojo/services/network/net_adapters.cc (right):

https://codereview.chromium.org/933973002/diff/20001/mojo/services/network/ne...
mojo/services/network/net_adapters.cc:112: error->code =
static_cast<NetworkCode>(error_code);
can you add a set of static_assert<>s that the enum values in mojo match the
ones in net/error/... ? that way if the error codes get out of sync there will
be a compile-time error instead of a mismatched enum value

https://codereview.chromium.org/933973002/diff/20001/mojo/services/network/pu...
File mojo/services/network/public/interfaces/network_error.mojom (right):

https://codereview.chromium.org/933973002/diff/20001/mojo/services/network/pu...
mojo/services/network/public/interfaces/network_error.mojom:12: // Corresponds
to net/base/net_error_list.h.
this isn't a helpful comment in the mojom as consumers of this mojom file
generally won't have a net/base/net_error_list.h to compare against

https://codereview.chromium.org/933973002/diff/20001/mojo/services/network/ud...
File mojo/services/network/udp_socket_apptest.cc (right):

https://codereview.chromium.org/933973002/diff/20001/mojo/services/network/ud...
mojo/services/network/udp_socket_apptest.cc:347:
EXPECT_EQ(mojo::NETWORK_CODE_OK, callback1.result()->code);
this code is in namespace mojo - just saying EXPECT_EQ(NETWORK_CODE_OK...)
should work

[ppi, 2015-02-19 14:16:14]

New patchsets have been uploaded after l-g-t-m from jamesr@chromium.org

[ppi, 2015-02-19 14:22:18]

ppi@chromium.org changed reviewers:
+ qsr@chromium.org

[ppi, 2015-02-19 14:22:19]

Thanks, James!

I somehow managed to miss half of the error codes in the original ps, fixed in
patch set 3.

+Ben, could you tal?

https://codereview.chromium.org/933973002/diff/20001/mojo/services/network/ne...
File mojo/services/network/net_adapters.cc (right):

https://codereview.chromium.org/933973002/diff/20001/mojo/services/network/ne...
mojo/services/network/net_adapters.cc:112: error->code =
static_cast<NetworkCode>(error_code);
On 2015/02/18 18:43:06, jamesr wrote:
> can you add a set of static_assert<>s that the enum values in mojo match the
> ones in net/error/... ? that way if the error codes get out of sync there will
> be a compile-time error instead of a mismatched enum value

Neat! Done.

https://codereview.chromium.org/933973002/diff/20001/mojo/services/network/pu...
File mojo/services/network/public/interfaces/network_error.mojom (right):

https://codereview.chromium.org/933973002/diff/20001/mojo/services/network/pu...
mojo/services/network/public/interfaces/network_error.mojom:12: // Corresponds
to net/base/net_error_list.h.
On 2015/02/18 18:43:06, jamesr wrote:
> this isn't a helpful comment in the mojom as consumers of this mojom file
> generally won't have a net/base/net_error_list.h to compare against

Done.

https://codereview.chromium.org/933973002/diff/20001/mojo/services/network/ud...
File mojo/services/network/udp_socket_apptest.cc (right):

https://codereview.chromium.org/933973002/diff/20001/mojo/services/network/ud...
mojo/services/network/udp_socket_apptest.cc:347:
EXPECT_EQ(mojo::NETWORK_CODE_OK, callback1.result()->code);
On 2015/02/18 18:43:06, jamesr wrote:
> this code is in namespace mojo - just saying EXPECT_EQ(NETWORK_CODE_OK...)
> should work

Done.

[qsr, 2015-02-19 14:27:00]

https://codereview.chromium.org/933973002/diff/40001/mojo/services/network/ne...
File mojo/services/network/net_adapters.cc (right):

https://codereview.chromium.org/933973002/diff/40001/mojo/services/network/ne...
mojo/services/network/net_adapters.cc:112: // error codes from net/net_errors.h.
Could you use the include + macro mechanism to do this?

[ppi, 2015-02-19 15:36:09]

https://codereview.chromium.org/933973002/diff/40001/mojo/services/network/ne...
File mojo/services/network/net_adapters.cc (right):

https://codereview.chromium.org/933973002/diff/40001/mojo/services/network/ne...
mojo/services/network/net_adapters.cc:112: // error codes from net/net_errors.h.
On 2015/02/19 14:27:00, qsr wrote:
> Could you use the include + macro mechanism to do this?

Done. As discussed in person, it's not clear if tying our mojom to the
net_error_list is optimal. Sent an email to mojo-dev and net-dev to see what
people think.

[jamesr, 2015-02-19 22:54:32]

Sounds like we probably do want a different error space, but since we're
currently just exposing the net error list replacing the untyped int32 with an
enum seems like a reasonable intermediate state.  Alternately we could drop the
code for now and just pass a bool + string until we know what to really use.

[ppi, 2015-02-23 17:55:22]

Thanks, James.

I think we should go for dropping the uint8 altogether, so that we don't grow
consumers that depend on this API. Unless someone protests here on on the
mailing list thread, I will put together a patch.

Wdyat about renaming the class from NetworkError to NetworkResult (as we're
using it for operations that succeed, too)?

[jamesr, 2015-02-23 18:01:45]

On 2015/02/23 17:55:22, ppi wrote:
> Thanks, James.
> 
> I think we should go for dropping the uint8 altogether, so that we don't grow
> consumers that depend on this API. Unless someone protests here on on the
> mailing list thread, I will put together a patch.

Sounds good.


> 
> Wdyat about renaming the class from NetworkError to NetworkResult (as we're
> using it for operations that succeed, too)?

How would failure be indicated?

[ppi, 2015-02-23 18:04:23]

By a boolean flag as you suggested. It's just that it'd be

struct NetworkResult {
  bool succeeded;
  string? description;
};

instead of

struct NetworkError {
  bool succeeded;
  string? description;
};

wdyt?

[jamesr, 2015-02-23 18:47:33]

and it wouldn't be nullable? seems almost equivalent similar to having

struct NetworkError {
  string? description;
};

and having the field be nullable in the various response types that use it, but
I think either way works.