Don't show network config UI when networkingPrivate.startConenct fails

Don't show network config UI when networkingPrivate.startConenct fails

If networkingPrivate.startConnect fails the API should not attempt showing
network configuration dialog (as it currently does for certain errors, e.g.
when the network passphrase is wrong). The API should let clients handle
the error themselves. Plus it makes expectations about network connection
state after startConnect returns clearer.

The behaviour is kept only for requests from web ui context, as network
config UI on Chrome OS still depends on this behavior.

BUG=703839
Review-Url: https://codereview.chromium.org/2814023002
Cr-Commit-Position: refs/heads/master@{#464123}
Committed: https://chromium.googlesource.com/chromium/src/+/70efae18e35db3dd25e00c2dfc205b7a3b72e76e

[tbarzic, 2017-04-12 00:45:44]

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

[commit-bot: I haz the power, 2017-04-12 00:46:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbarzic, 2017-04-12 00:58:26]

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

[commit-bot: I haz the power, 2017-04-12 00:58:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 01:44:35]

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

[commit-bot: I haz the power, 2017-04-12 01:44:36]

Dry run: This issue passed the CQ dry run.

[tbarzic, 2017-04-12 01:50:16]

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

[stevenjb, 2017-04-12 02:09:46]

Is there any urgency on this? I will be working on integrating network
configuration with Settings over the next few weeks, at which point we should be
able to fix this for real.

[tbarzic, 2017-04-12 17:15:05]

On 2017/04/12 02:09:46, stevenjb wrote:
> Is there any urgency on this? I will be working on integrating network
> configuration with Settings over the next few weeks, at which point we should
be
> able to fix this for real.

It's not really urgent, but I think it would be nice to have in 59, especially
for kiosk apps.

And the change is not that big - it just restricts the current workaround to
calls from webui (I had to move the workaround code from
networking_private_chromeos.cc to networking_private_api.cc, so I can get
information about the context type).

[stevenjb, 2017-04-12 17:27:46]

On 2017/04/12 17:15:05, tbarzic wrote:
> On 2017/04/12 02:09:46, stevenjb wrote:
> > Is there any urgency on this? I will be working on integrating network
> > configuration with Settings over the next few weeks, at which point we
should
> be
> > able to fix this for real.
> 
> It's not really urgent, but I think it would be nice to have in 59, especially
> for kiosk apps.
> 
> And the change is not that big - it just restricts the current workaround to
> calls from webui (I had to move the workaround code from
> networking_private_chromeos.cc to networking_private_api.cc, so I can get
> information about the context type).

Right now I am a bit paranoid of any changes that might affect the Settings UI
at this point since we are releasing MD Settings in 59...

[tbarzic, 2017-04-12 17:58:12]

On 2017/04/12 17:27:46, stevenjb wrote:
> On 2017/04/12 17:15:05, tbarzic wrote:
> > On 2017/04/12 02:09:46, stevenjb wrote:
> > > Is there any urgency on this? I will be working on integrating network
> > > configuration with Settings over the next few weeks, at which point we
> should
> > be
> > > able to fix this for real.
> > 
> > It's not really urgent, but I think it would be nice to have in 59,
especially
> > for kiosk apps.
> > 
> > And the change is not that big - it just restricts the current workaround to
> > calls from webui (I had to move the workaround code from
> > networking_private_chromeos.cc to networking_private_api.cc, so I can get
> > information about the context type).
> 
> Right now I am a bit paranoid of any changes that might affect the Settings UI
> at this point since we are releasing MD Settings in 59...

This should be fairly low risk for Settings UI (as the behavior for webui is
unchanged), but OK.

[stevenjb, 2017-04-12 18:14:57]

https://codereview.chromium.org/2814023002/diff/20001/extensions/browser/api/...
File extensions/browser/api/networking_private/networking_private_api.cc
(right):

https://codereview.chromium.org/2814023002/diff/20001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_api.cc:606: if
(source_context_type() == Feature::WEBUI_CONTEXT) {
This seems fine, but I am unfamiliar with the code, thus my caution.

https://codereview.chromium.org/2814023002/diff/20001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_api.cc:613: }
Did you verify that the dialog shows up on connect failure in OOBE, login
screen, and post login?

[tbarzic, 2017-04-12 18:50:36]

https://codereview.chromium.org/2814023002/diff/20001/extensions/browser/api/...
File extensions/browser/api/networking_private/networking_private_api.cc
(right):

https://codereview.chromium.org/2814023002/diff/20001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_api.cc:606: if
(source_context_type() == Feature::WEBUI_CONTEXT) {
On 2017/04/12 18:14:57, stevenjb wrote:
> This seems fine, but I am unfamiliar with the code, thus my caution.

this checks whether startConnect function was called from web ui rather than an
extension/app.

https://codereview.chromium.org/2814023002/diff/20001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_api.cc:613: }
On 2017/04/12 18:14:57, stevenjb wrote:
> Did you verify that the dialog shows up on connect failure in OOBE, login
> screen, and post login?

yes I have

[stevenjb, 2017-04-12 18:52:13]

On 2017/04/12 18:50:36, tbarzic wrote:
>
https://codereview.chromium.org/2814023002/diff/20001/extensions/browser/api/...
> File extensions/browser/api/networking_private/networking_private_api.cc
> (right):
> 
>
https://codereview.chromium.org/2814023002/diff/20001/extensions/browser/api/...
> extensions/browser/api/networking_private/networking_private_api.cc:606: if
> (source_context_type() == Feature::WEBUI_CONTEXT) {
> On 2017/04/12 18:14:57, stevenjb wrote:
> > This seems fine, but I am unfamiliar with the code, thus my caution.
> 
> this checks whether startConnect function was called from web ui rather than
an
> extension/app.
> 
>
https://codereview.chromium.org/2814023002/diff/20001/extensions/browser/api/...
> extensions/browser/api/networking_private/networking_private_api.cc:613: }
> On 2017/04/12 18:14:57, stevenjb wrote:
> > Did you verify that the dialog shows up on connect failure in OOBE, login
> > screen, and post login?
> 
> yes I have

OK, lgtm...

[tbarzic, 2017-04-12 19:22:36]

The CQ bit was checked by tbarzic@chromium.org

[commit-bot: I haz the power, 2017-04-12 19:23:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 20:01:31]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492024956515490,
"parent_rev": "35927811f7b1d449675cbad2de6952844a93ef3e", "commit_rev":
"70efae18e35db3dd25e00c2dfc205b7a3b72e76e"}

[commit-bot: I haz the power, 2017-04-12 21:09:45]

Description was changed from

==========
Don't show network config UI when networkingPrivate.startConenct fails

If networkingPrivate.startConnect fails the API should not attempt showing
network configuration dialog (as it currently does for certain errors, e.g.
when the network passphrase is wrong). The API should let clients handle
the error themselves. Plus it makes expectations about network connection
state after startConnect returns clearer.

The behaviour is kept only for requests from web ui context, as network
config UI on Chrome OS still depends on this behavior.

BUG=703839
==========

to

==========
Don't show network config UI when networkingPrivate.startConenct fails

If networkingPrivate.startConnect fails the API should not attempt showing
network configuration dialog (as it currently does for certain errors, e.g.
when the network passphrase is wrong). The API should let clients handle
the error themselves. Plus it makes expectations about network connection
state after startConnect returns clearer.

The behaviour is kept only for requests from web ui context, as network
config UI on Chrome OS still depends on this behavior.

BUG=703839

Review-Url: https://codereview.chromium.org/2814023002
Cr-Commit-Position: refs/heads/master@{#464123}
Committed:
https://chromium.googlesource.com/chromium/src/+/70efae18e35db3dd25e00c2dfc20...
==========

[commit-bot: I haz the power, 2017-04-12 21:09:51]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/70efae18e35db3dd25e00c2dfc20...