Network Setup from Shark to Remora.

Network Setup from Shark to Remora via Bluetooth.

BUG=552591
Committed: https://crrev.com/ca9c8150df20f27a6da0526bbf4e7b417629e103
Cr-Commit-Position: refs/heads/master@{#358732}

[xdai1, 2015-11-05 22:55:05]

Description was changed from

==========
Network Setup.

BUG=
==========

to

==========
Network Setup from Shark to Remora via Bluetooth.

BUG=
==========

[xdai1, 2015-11-05 22:58:12]

xdai@chromium.org changed reviewers:
+ achuith@chromium.org

[xdai1, 2015-11-05 22:58:13]

achuith@, could you help to review the CL please? This CL can pass the unsecured
network configuration from Shark to Remora. I think this could be useful later
in the bootstrapping setup.

[xdai1, 2015-11-05 22:58:32]

Patchset #1 (id:1) has been deleted

[achuithb, 2015-11-05 23:10:47]

On 2015/11/05 22:58:13, xdai1 wrote:
> achuith@, could you help to review the CL please? This CL can pass the
unsecured
> network configuration from Shark to Remora. I think this could be useful later
> in the bootstrapping setup.

Please file a tracking bug with details and reference in this CL.

[achuithb, 2015-11-05 23:26:12]

achuith@chromium.org changed reviewers:
+ stevenjb@chromium.org

[achuithb, 2015-11-05 23:26:12]

Steven: could you please look at the changes in network_screen.cc

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/helper.cc (right):

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/helper.cc:178: LOG(ERROR) << "Failed to create
network configuration: " << error_name;
Should we be showing the network error screen here?

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/helper.h (right):

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/helper.h:26: }  // namespace content
I think we don't do this when the namespace is small - it's obvious what the
closing brace is for, and the comment just adds clutter.

I would revert the changes in this file.

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/controller_pairing_screen.h (right):

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/controller_pairing_screen.h:26: // Set
remora network from Shark.
s/Shark/shark for consistency.

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/network_screen.cc (right):

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/network_screen.cc:251: if
(security.compare(onc::wifi::kSecurityNone) != 0) {
don't need {}

Why not 
if (security != onc::wifi:kSecurityNone)

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/network_screen.cc:255: const
std::vector<uint8_t> raw_ssid = network_state->raw_ssid();
Why not GetHexSsid?
https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/network/n...

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/network_screen.h (right):

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/network_screen.h:94: std::string
GetConnectedWifiNetwork();
I think it might be better to pass in a std::string* out_onc_spec instead?

https://codereview.chromium.org/1428353002/diff/20001/components/pairing/prot...
File components/pairing/proto_decoder.cc (right):

https://codereview.chromium.org/1428353002/diff/20001/components/pairing/prot...
components/pairing/proto_decoder.cc:142: if
(!message.SerializeToString(&serialized_proto)) {
Don't need {}

[achuithb, 2015-11-05 23:26:30]

We really need some tests for this shark/remora code :(

[achuithb, 2015-11-05 23:26:39]

The CQ bit was checked by achuith@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-05 23:27:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1428353002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1428353002/20001

[stevenjb, 2015-11-05 23:48:25]

Please set a BUG= for this

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/helper.h (right):

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/helper.h:26: }  // namespace content
On 2015/11/05 23:26:12, achuithb wrote:
> I think we don't do this when the namespace is small - it's obvious what the
> closing brace is for, and the comment just adds clutter.
> 
> I would revert the changes in this file.

+1

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/network_screen.cc (right):

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/network_screen.cc:251: if
(security.compare(onc::wifi::kSecurityNone) != 0) {
On 2015/11/05 23:26:12, achuithb wrote:
> don't need {}
> 
> Why not 
> if (security != onc::wifi:kSecurityNone)

+1

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/network_screen.cc:252: return
std::string();
Eventually, we could support WEP/WPA/PSK networks (which just require a
passphrase) by requesting the password from the host. We don't need to do this
now, but we should make sure that the code structure will be able to support
that.

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/network_screen.cc:255: const
std::vector<uint8_t> raw_ssid = network_state->raw_ssid();
On 2015/11/05 23:26:12, achuithb wrote:
> Why not GetHexSsid?
>
https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/network/n...

+1

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/network_screen.cc:258:
scoped_ptr<base::DictionaryValue> coped_onc(new base::DictionaryValue());
s/coped/copied/

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.cc:1050: if (shark_controller_)
{
if (!shark_controller_)
  return;

[commit-bot: I haz the power, 2015-11-05 23:51:13]

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

[commit-bot: I haz the power, 2015-11-05 23:51:13]

Dry run: This issue passed the CQ dry run.

[xdai1, 2015-11-06 23:16:26]

Description was changed from

==========
Network Setup from Shark to Remora via Bluetooth.

BUG=
==========

to

==========
Network Setup from Shark to Remora via Bluetooth.

BUG=552591
==========

[xdai1, 2015-11-06 23:23:02]

achuithb@, stevenjb@, I've addressed the comments, please take another look,
thanks for the review!

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/helper.cc (right):

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/helper.cc:178: LOG(ERROR) << "Failed to create
network configuration: " << error_name;
On 2015/11/05 23:26:12, achuithb wrote:
> Should we be showing the network error screen here?

Done.

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/helper.h (right):

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/helper.h:26: }  // namespace content
On 2015/11/05 23:26:12, achuithb wrote:
> I think we don't do this when the namespace is small - it's obvious what the
> closing brace is for, and the comment just adds clutter.
> 
> I would revert the changes in this file.

Done.

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/controller_pairing_screen.h (right):

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/controller_pairing_screen.h:26: // Set
remora network from Shark.
On 2015/11/05 23:26:12, achuithb wrote:
> s/Shark/shark for consistency.

Done.

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/network_screen.cc (right):

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/network_screen.cc:251: if
(security.compare(onc::wifi::kSecurityNone) != 0) {
On 2015/11/05 23:26:12, achuithb wrote:
> don't need {}
> 
> Why not 
> if (security != onc::wifi:kSecurityNone)

Done.

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/network_screen.cc:252: return
std::string();
On 2015/11/05 23:48:25, stevenjb wrote:
> Eventually, we could support WEP/WPA/PSK networks (which just require a
> passphrase) by requesting the password from the host. We don't need to do this
> now, but we should make sure that the code structure will be able to support
> that.

The Hotrod shark/remora (a.k.a, controller/host) project has been suspended. I'm
just trying to reuse some host side code for the new bootstrapping project. So I
think we only need to make sure that the code structure of the host side in Cros
can support creating and applying ONC network configuration to itself (which is
done by NetworkScreen::CreateAndApplyNetworkFromOnc() function), because the ONC
network configuration will be passed to the host Cros device from an Android
App. And the Android App probably can get the password by asking it from the
admin anyway. The shark side in Cros is basically for testing purpose to make
sure the entire flow work as intended. It would be great if we can also support
the secured network for the shark side, but I think it might not a high priority
task. Wdyt?

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/network_screen.cc:255: const
std::vector<uint8_t> raw_ssid = network_state->raw_ssid();
On 2015/11/05 23:48:25, stevenjb wrote:
> On 2015/11/05 23:26:12, achuithb wrote:
> > Why not GetHexSsid?
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/network/n...
> 
> +1

Done. But it seems more complicated than just using raw_ssid directly.

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/network_screen.cc:258:
scoped_ptr<base::DictionaryValue> coped_onc(new base::DictionaryValue());
On 2015/11/05 23:48:25, stevenjb wrote:
> s/coped/copied/

Done.

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/network_screen.h (right):

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/network_screen.h:94: std::string
GetConnectedWifiNetwork();
On 2015/11/05 23:26:12, achuithb wrote:
> I think it might be better to pass in a std::string* out_onc_spec instead?

Done.

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.cc:1050: if (shark_controller_)
{
On 2015/11/05 23:48:25, stevenjb wrote:
> if (!shark_controller_)
>   return;

Done.

https://codereview.chromium.org/1428353002/diff/20001/components/pairing/prot...
File components/pairing/proto_decoder.cc (right):

https://codereview.chromium.org/1428353002/diff/20001/components/pairing/prot...
components/pairing/proto_decoder.cc:142: if
(!message.SerializeToString(&serialized_proto)) {
On 2015/11/05 23:26:12, achuithb wrote:
> Don't need {}

Done.

[xdai1, 2015-11-06 23:23:33]

On 2015/11/05 23:26:30, achuithb wrote:
> We really need some tests for this shark/remora code :(

Can I add the test code in a following up CL?

[stevenjb, 2015-11-06 23:32:13]

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/network_screen.cc (right):

https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/network_screen.cc:255: const
std::vector<uint8_t> raw_ssid = network_state->raw_ssid();
On 2015/11/06 23:23:02, xdai1 wrote:
> On 2015/11/05 23:48:25, stevenjb wrote:
> > On 2015/11/05 23:26:12, achuithb wrote:
> > > Why not GetHexSsid?
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/network/n...
> > 
> > +1
> 
> Done. But it seems more complicated than just using raw_ssid directly.

Sorry, the suggestion was to use network_state->GetHexSsid() which returns a
string.

[xdai1, 2015-11-06 23:55:30]

On 2015/11/06 23:32:13, stevenjb wrote:
>
https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
> File chrome/browser/chromeos/login/screens/network_screen.cc (right):
> 
>
https://codereview.chromium.org/1428353002/diff/20001/chrome/browser/chromeos...
> chrome/browser/chromeos/login/screens/network_screen.cc:255: const
> std::vector<uint8_t> raw_ssid = network_state->raw_ssid();
> On 2015/11/06 23:23:02, xdai1 wrote:
> > On 2015/11/05 23:48:25, stevenjb wrote:
> > > On 2015/11/05 23:26:12, achuithb wrote:
> > > > Why not GetHexSsid?
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/network/n...
> > > 
> > > +1
> > 
> > Done. But it seems more complicated than just using raw_ssid directly.
> 
> Sorry, the suggestion was to use network_state->GetHexSsid() which returns a
> string.

I see. Please take another look, thanks!

[stevenjb, 2015-11-07 00:03:04]

lgtm

[achuithb, 2015-11-09 23:17:10]

The CQ bit was checked by achuith@chromium.org to run a CQ dry run

[achuithb, 2015-11-09 23:17:28]

lgtm.

Please think about tests

https://codereview.chromium.org/1428353002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/network_screen.cc (right):

https://codereview.chromium.org/1428353002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/network_screen.cc:255: std::string
hex_ssid = network_state->GetHexSsid();
nit: const

[commit-bot: I haz the power, 2015-11-09 23:19:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1428353002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1428353002/60001

[xdai1, 2015-11-10 00:11:01]

Thanks for the review! I will add the test in a following CL.

[xdai1, 2015-11-10 00:23:31]

The CQ bit was checked by xdai@chromium.org

[xdai1, 2015-11-10 00:23:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
achuith@chromium.org, stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/1428353002/#ps80001
(title: "Address achuithb@'s comment.")

[commit-bot: I haz the power, 2015-11-10 00:26:34]

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

[dzhioev (left Google), 2015-11-10 00:33:10]

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

[dzhioev (left Google), 2015-11-10 00:33:11]

https://codereview.chromium.org/1428353002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/network_screen.cc (right):

https://codereview.chromium.org/1428353002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/network_screen.cc:236: void
NetworkScreen::GetConnectedWifiNetwork(std::string* out_onc_spec) {
My two cents: why this method is a part of NetworkScreen? NetworkScreen class is
responsible for the UI dialog, and it should not be used as a network info
provider for other classes.

[xdai1, 2015-11-10 00:48:43]

On 2015/11/10 00:33:11, dzhioev wrote:
>
https://codereview.chromium.org/1428353002/diff/80001/chrome/browser/chromeos...
> File chrome/browser/chromeos/login/screens/network_screen.cc (right):
> 
>
https://codereview.chromium.org/1428353002/diff/80001/chrome/browser/chromeos...
> chrome/browser/chromeos/login/screens/network_screen.cc:236: void
> NetworkScreen::GetConnectedWifiNetwork(std::string* out_onc_spec) {
> My two cents: why this method is a part of NetworkScreen? NetworkScreen class
is
> responsible for the UI dialog, and it should not be used as a network info
> provider for other classes.

Thanks for the suggestion! The reason I put the function here is I think the
Wifi network information is gotten from shark's NetworkScreen page. I think
you're right. How about I do something similar like
NetworkScreen::CreateAndConnectNetworkFromOnc() function: put the implementation
in NetworkStateHelper and call it from here?

[commit-bot: I haz the power, 2015-11-10 01:30:25]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-11-10 01:31:28]

Patchset 4 (id:??) landed as
https://crrev.com/ca9c8150df20f27a6da0526bbf4e7b417629e103
Cr-Commit-Position: refs/heads/master@{#358732}

[dzhioev (left Google), 2015-11-10 23:20:31]

On 2015/11/10 00:48:43, xdai1 wrote:
> On 2015/11/10 00:33:11, dzhioev wrote:
> >
>
https://codereview.chromium.org/1428353002/diff/80001/chrome/browser/chromeos...
> > File chrome/browser/chromeos/login/screens/network_screen.cc (right):
> > 
> >
>
https://codereview.chromium.org/1428353002/diff/80001/chrome/browser/chromeos...
> > chrome/browser/chromeos/login/screens/network_screen.cc:236: void
> > NetworkScreen::GetConnectedWifiNetwork(std::string* out_onc_spec) {
> > My two cents: why this method is a part of NetworkScreen? NetworkScreen
class
> is
> > responsible for the UI dialog, and it should not be used as a network info
> > provider for other classes.
> 
> Thanks for the suggestion! The reason I put the function here is I think the
> Wifi network information is gotten from shark's NetworkScreen page. I think
> you're right. How about I do something similar like
> NetworkScreen::CreateAndConnectNetworkFromOnc() function: put the
implementation
> in NetworkStateHelper and call it from here?

I think NetworkScreen class should be involved in this flow at all. Despite its
name,
he has very little to do with a network configuration. The UI element for
changing
networks (network dropdown) communicates directly with NetworkDropdownHandler
which
through several levels of abstraction talks with lower level classes which do a
real work
(such as NetworkStateHandler or NetworkConnectionHandler). Adding
NetworkScreen::CreateNetworkFromOnc here was not a correct decision as well.
I strongly insist of moving all the ONC-related code (what ONC stands for BTW?)
out
from NetworkScreen. On the other hand, I'm not sure where this code should be
moved
to, because the current architecture of shark/remora UI code is little bit
messed.
I think we need to figure out how to make it right. Can we meet in person next
Tuesday to
discuss it?

[xdai1, 2015-11-11 06:30:35]

On 2015/11/10 23:20:31, dzhioev wrote:
> On 2015/11/10 00:48:43, xdai1 wrote:
> > On 2015/11/10 00:33:11, dzhioev wrote:
> > >
> >
>
https://codereview.chromium.org/1428353002/diff/80001/chrome/browser/chromeos...
> > > File chrome/browser/chromeos/login/screens/network_screen.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1428353002/diff/80001/chrome/browser/chromeos...
> > > chrome/browser/chromeos/login/screens/network_screen.cc:236: void
> > > NetworkScreen::GetConnectedWifiNetwork(std::string* out_onc_spec) {
> > > My two cents: why this method is a part of NetworkScreen? NetworkScreen
> class
> > is
> > > responsible for the UI dialog, and it should not be used as a network info
> > > provider for other classes.
> > 
> > Thanks for the suggestion! The reason I put the function here is I think the
> > Wifi network information is gotten from shark's NetworkScreen page. I think
> > you're right. How about I do something similar like
> > NetworkScreen::CreateAndConnectNetworkFromOnc() function: put the
> implementation
> > in NetworkStateHelper and call it from here?
> 
> I think NetworkScreen class should be involved in this flow at all. Despite
its
> name,
> he has very little to do with a network configuration. The UI element for
> changing
> networks (network dropdown) communicates directly with NetworkDropdownHandler
> which
> through several levels of abstraction talks with lower level classes which do
a
> real work
> (such as NetworkStateHandler or NetworkConnectionHandler). Adding
> NetworkScreen::CreateNetworkFromOnc here was not a correct decision as well.
> I strongly insist of moving all the ONC-related code (what ONC stands for
BTW?)
> out
> from NetworkScreen. On the other hand, I'm not sure where this code should be
> moved
> to, because the current architecture of shark/remora UI code is little bit
> messed.
> I think we need to figure out how to make it right. Can we meet in person next
> Tuesday to
> discuss it?

Sure, I would love to! And very appreciate your help. Let's talk it offline to
schedule a time for this:)