ARC: net.mojom passphrase field should be nullable.

ARC: net.mojom passphrase field should be nullable.

It is expected to be nullable in the both sides of connection.
For Chromium side, the field is null-checked here.
 https://code.google.com/p/chromium/codesearch#chromium/src/components/arc/net/\
     arc_net_host_impl.cc&q=passphrase.is_null()&sq=package:chromium&l=224

This CL adjust the Mojo definition to the actual usage.

BUG=b/28752817

Committed: https://crrev.com/6b6f16a897e85e3044b74d678d5171867eefd817
Cr-Commit-Position: refs/heads/master@{#393999}

[kinaba, 2016-05-13 06:55:25]

kinaba@chromium.org changed reviewers:
+ abhishekbh@google.com, dcheng@chromium.org

[kinaba, 2016-05-13 06:55:25]

PTAL.


dcheng@, is this way of changing to nullable fine?
Or should I seriously guard it by versioning?

As far as I tried the both of mismatching case,

"Sender string --> Receiver string?" just worked fine.
"Sender string? --> Receiver string" worked when the value is non-null.
"Sender string? --> Receiver string" somehow gracefully failed when null is
sent; exception raised at Sender side.

The last case is no worse than current situation--the current sender is buggy
and sending |null| as non-nullable string and failing (b/28752817).

[dcheng, 2016-05-13 07:06:19]

dcheng@chromium.org changed reviewers:
+ yzshen@chromium.org

[dcheng, 2016-05-13 07:06:19]

+yzshen as the bindings expert for the versioning question

[yzshen1, 2016-05-16 19:36:59]

On 2016/05/13 07:06:19, dcheng wrote:
> +yzshen as the bindings expert for the versioning question

Sorry I didn't see this thread earlier.


> "Sender string --> Receiver string?" just worked fine.
Yes.

> "Sender string? --> Receiver string" worked when the value is non-null.
Yes.

> "Sender string? --> Receiver string" somehow gracefully failed when null is
sent; exception raised at Sender side.
It will cause validation failure at the Receiver side, which will disconnect
this message pipe. The Sender will observe the effect that the pipe is closed.

[yzshen1, 2016-05-16 19:39:06]

It is fine to make such changes if you don't care pipe disconnection in the last
case.


If you want to make it backward compatible. You could do:
struct ConfiguredNetworkDetails {
  string passphrase;
  bool autoconnect;
  [MinVersion=1] bool has_passphrase;
};

[dcheng, 2016-05-16 19:41:38]

In any case, the mojom LGTM. It's up to you if you're OK with the compatibility
semantics.

[kinaba, 2016-05-17 00:28:22]

On 2016/05/16 19:36:59, yzshen1 wrote:
> On 2016/05/13 07:06:19, dcheng wrote:
> > +yzshen as the bindings expert for the versioning question
> 
> Sorry I didn't see this thread earlier.
> 
> 
> > "Sender string --> Receiver string?" just worked fine.
> Yes.
> 
> > "Sender string? --> Receiver string" worked when the value is non-null.
> Yes.
> 
> > "Sender string? --> Receiver string" somehow gracefully failed when null is
> sent; exception raised at Sender side.
> It will cause validation failure at the Receiver side, which will disconnect
> this message pipe. The Sender will observe the effect that the pipe is closed.

Thanks for the clarification!
Yep, pipe disconnection in the last case is fine for us; we've already been
closing
the connection by us own on sender side error as well, so the things won't
change much.

Considering that, let me go with the current simplest way. Thanks for your help.

[kinaba, 2016-05-17 00:28:38]

The CQ bit was checked by kinaba@chromium.org

[commit-bot: I haz the power, 2016-05-17 00:29:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1973233002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1973233002/1

[commit-bot: I haz the power, 2016-05-17 01:04:55]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-05-17 01:06:04]

Description was changed from

==========
ARC: net.mojom passphrase field should be nullable.

It is expected to be nullable in the both sides of connection.
For Chromium side, the field is null-checked here.
 https://code.google.com/p/chromium/codesearch#chromium/src/components/arc/net/\
     arc_net_host_impl.cc&q=passphrase.is_null()&sq=package:chromium&l=224

This CL adjust the Mojo definition to the actual usage.

BUG=b/28752817
==========

to

==========
ARC: net.mojom passphrase field should be nullable.

It is expected to be nullable in the both sides of connection.
For Chromium side, the field is null-checked here.
 https://code.google.com/p/chromium/codesearch#chromium/src/components/arc/net/\
     arc_net_host_impl.cc&q=passphrase.is_null()&sq=package:chromium&l=224

This CL adjust the Mojo definition to the actual usage.

BUG=b/28752817

Committed: https://crrev.com/6b6f16a897e85e3044b74d678d5171867eefd817
Cr-Commit-Position: refs/heads/master@{#393999}
==========

[commit-bot: I haz the power, 2016-05-17 01:06:06]

Patchset 1 (id:??) landed as
https://crrev.com/6b6f16a897e85e3044b74d678d5171867eefd817
Cr-Commit-Position: refs/heads/master@{#393999}