Use NetworkConnectionObserver to trigger connect failure notifications

Use NetworkConnectionObserver to trigger connect failure notifications

Currently we trigger network connect failure notifications when a call
from ui::NetworkConnect::ConnectToNetwork() fails.

The problem with this approach is that connect calls from elsewhere
(e.g. through the networkingPrivate API) will not trigger notifications.

This CL makes NetworkStateNotifier a NetworkConnectionObserver and
triggers the failure notification on ConnectFailed events instead of the
NetworkConnect failure callback so that other failures will also
generate notifications.

This does not affect spawining of new UI (e.g. network configuration UI)
on failure (when appropriate) since that is expected to be handled by any
new UI that uses the networkingPrivate API.

BUG=434112
Committed: https://crrev.com/2a54d25447a000fc782ea3a8190f18913ad645b9
Cr-Commit-Position: refs/heads/master@{#312879}

[stevenjb, 2014-11-26 00:47:31]

stevenjb@chromium.org changed reviewers:
+ michaelpg@chromium.org, pneubeck@chromium.org

[stevenjb, 2014-11-26 00:47:32]

pneubeck@ - Apologies again for continuing to bombard you with network UI CLs,
but there really isn't anyone else especially familiar with this code who is
still working on it.

michaelpg@ - I know you're not really familiar with this code yet, but we need
more devs who are, and it will be a good exercise in determining whether or not
it makes at least some sense to an unfamiliar dev. Please try to take a look,
especially at the code in ui/ and don't hesitate to question anything.

[stevenjb, 2014-12-16 18:18:25]

ping

[pneubeck (no reviews), 2015-01-04 16:57:00]

mostly nits. apart from that lgtm.

https://codereview.chromium.org/762633002/diff/1/chromeos/chromeos.gyp
File chromeos/chromeos.gyp (right):

https://codereview.chromium.org/762633002/diff/1/chromeos/chromeos.gyp#newcod...
chromeos/chromeos.gyp:343: 'network/network_connection_observer.cc',
do you have to update the BUILD.gn file too?

https://codereview.chromium.org/762633002/diff/1/chromeos/network/network_con...
File chromeos/network/network_connection_handler.h (right):

https://codereview.chromium.org/762633002/diff/1/chromeos/network/network_con...
chromeos/network/network_connection_handler.h:85: static const char
kErrorConnectFailed[];
the constant name should probably be a bit more concrete like
kErrorUnknownConnectFailure or kErrorUnexpectedConnectFailure ?
similarly for Disconnect

https://codereview.chromium.org/762633002/diff/1/chromeos/network/network_con...
chromeos/network/network_connection_handler.h:209: void InvokeErrorCallback(
function name should mention that this is specific to a Connect request compared
to Disconnect.
Maybe 'NotifyConnectError' and above 'NotifyConnectSuccess' ?

https://codereview.chromium.org/762633002/diff/1/chromeos/network/network_con...
File chromeos/network/network_connection_observer.h (right):

https://codereview.chromium.org/762633002/diff/1/chromeos/network/network_con...
chromeos/network/network_connection_observer.h:14: class DictionaryValue;
nit: not used

https://codereview.chromium.org/762633002/diff/1/chromeos/network/network_con...
chromeos/network/network_connection_observer.h:44:
DISALLOW_ASSIGN(NetworkConnectionObserver);
since this class is not abstract you should also disallow the copy constructor.
This will require that you explicitly define a constructor.

https://codereview.chromium.org/762633002/diff/1/ui/chromeos/network/network_...
File ui/chromeos/network/network_connect.cc (right):

https://codereview.chromium.org/762633002/diff/1/ui/chromeos/network/network_...
ui/chromeos/network/network_connect.cc:204: error_name ==
NetworkConnectionHandler::kErrorConnecting) {
is this a change compared to before? not sure whether users can run into long
running attempts of shill to connect which they cannot abort anymore except
through disabling wifi or going to the settings page.

https://codereview.chromium.org/762633002/diff/1/ui/chromeos/network/network_...
ui/chromeos/network/network_connect.cc:228: // Only show a configure dialog if
there was a ConnectFailed error.
'if there was a ConnectFailed error' is not very informative.
maybe 'if there was an unexpected error during the connect attempt' ?

https://codereview.chromium.org/762633002/diff/1/ui/chromeos/network/network_...
ui/chromeos/network/network_connect.cc:232: // Notifications on connect failure
are handled by NetworkStateNotifier.
should this be a comment at the top of the function?

https://codereview.chromium.org/762633002/diff/1/ui/chromeos/network/network_...
File ui/chromeos/network/network_state_notifier.cc (right):

https://codereview.chromium.org/762633002/diff/1/ui/chromeos/network/network_...
ui/chromeos/network/network_state_notifier.cc:116: // Only show a notification
for recognized errors. Other failures are expected
What means 'recognized'? kErrorConnectFailed does not sound like a recognized
error. Does it really belong here?

If this if condition should filter for errors that can be handled by
NetworkConnectImpl::GetShillErrorString then NetworkConnectImpl should expose
this check.

[stevenjb, 2015-01-09 18:18:03]

I was going to try to get this in before the branch, but I think I will wait so
that we get testing/feedback early in the cycle. Nothing for 41 is blocked on it
that I am aware of.
PTAL

https://codereview.chromium.org/762633002/diff/1/chromeos/chromeos.gyp
File chromeos/chromeos.gyp (right):

https://codereview.chromium.org/762633002/diff/1/chromeos/chromeos.gyp#newcod...
chromeos/chromeos.gyp:343: 'network/network_connection_observer.cc',
On 2015/01/04 16:57:00, pneubeck wrote:
> do you have to update the BUILD.gn file too?

Nope, .gn imports chromeos_sources from this file.

https://codereview.chromium.org/762633002/diff/1/chromeos/network/network_con...
File chromeos/network/network_connection_handler.h (right):

https://codereview.chromium.org/762633002/diff/1/chromeos/network/network_con...
chromeos/network/network_connection_handler.h:85: static const char
kErrorConnectFailed[];
On 2015/01/04 16:57:00, pneubeck wrote:
> the constant name should probably be a bit more concrete like
> kErrorUnknownConnectFailure or kErrorUnexpectedConnectFailure ?
> similarly for Disconnect

The failure could be for any number of reasons, I'm not sure that 'Unknown' or
'Unexpected' add any clarity here, and may imply more than they suggest. I'd
rather leave these as-is.

https://codereview.chromium.org/762633002/diff/1/chromeos/network/network_con...
chromeos/network/network_connection_handler.h:209: void InvokeErrorCallback(
On 2015/01/04 16:57:00, pneubeck wrote:
> function name should mention that this is specific to a Connect request
compared
> to Disconnect.
> Maybe 'NotifyConnectError' and above 'NotifyConnectSuccess' ?

-> InvokeConnect{Success|Error}Callback

https://codereview.chromium.org/762633002/diff/1/chromeos/network/network_con...
File chromeos/network/network_connection_observer.h (right):

https://codereview.chromium.org/762633002/diff/1/chromeos/network/network_con...
chromeos/network/network_connection_observer.h:14: class DictionaryValue;
On 2015/01/04 16:57:00, pneubeck wrote:
> nit: not used

Done.

https://codereview.chromium.org/762633002/diff/1/chromeos/network/network_con...
chromeos/network/network_connection_observer.h:44:
DISALLOW_ASSIGN(NetworkConnectionObserver);
On 2015/01/04 16:57:00, pneubeck wrote:
> since this class is not abstract you should also disallow the copy
constructor.
> This will require that you explicitly define a constructor.

Done.

https://codereview.chromium.org/762633002/diff/1/ui/chromeos/network/network_...
File ui/chromeos/network/network_connect.cc (right):

https://codereview.chromium.org/762633002/diff/1/ui/chromeos/network/network_...
ui/chromeos/network/network_connect.cc:204: error_name ==
NetworkConnectionHandler::kErrorConnecting) {
On 2015/01/04 16:57:00, pneubeck wrote:
> is this a change compared to before? not sure whether users can run into long
> running attempts of shill to connect which they cannot abort anymore except
> through disabling wifi or going to the settings page.

This handles the case where we attempt to connect to network A while it is
connecting, e.g. by having two settings UI windows open (or a bug in the UI).
The user will be notified (or the appropriate UI  will be spawned) when the
initial attempt fails or succeeds. Notifying them again just adds noise /
confusion.

https://codereview.chromium.org/762633002/diff/1/ui/chromeos/network/network_...
ui/chromeos/network/network_connect.cc:228: // Only show a configure dialog if
there was a ConnectFailed error.
On 2015/01/04 16:57:00, pneubeck wrote:
> 'if there was a ConnectFailed error' is not very informative.
> maybe 'if there was an unexpected error during the connect attempt' ?

Again, 'ConnectFailed' could be caused by a number of things, which (at this
point) we do not differentiate. I don't think that "unexpected" or "unknown"
adds any clarity here. I will try to clarify the comment a bit though.

https://codereview.chromium.org/762633002/diff/1/ui/chromeos/network/network_...
ui/chromeos/network/network_connect.cc:232: // Notifications on connect failure
are handled by NetworkStateNotifier.
On 2015/01/04 16:57:00, pneubeck wrote:
> should this be a comment at the top of the function?

This explains why we don't do anything else in this function. I will clarify
this comment and add a note above the function.

https://codereview.chromium.org/762633002/diff/1/ui/chromeos/network/network_...
File ui/chromeos/network/network_state_notifier.cc (right):

https://codereview.chromium.org/762633002/diff/1/ui/chromeos/network/network_...
ui/chromeos/network/network_state_notifier.cc:116: // Only show a notification
for recognized errors. Other failures are expected
On 2015/01/04 16:57:00, pneubeck wrote:
> What means 'recognized'? kErrorConnectFailed does not sound like a recognized
> error. Does it really belong here?
> 
> If this if condition should filter for errors that can be handled by
> NetworkConnectImpl::GetShillErrorString then NetworkConnectImpl should expose
> this check.

s/recognized/certain.

Clarified the comment for kErrorConnectFailed.

[pneubeck (no reviews), 2015-01-09 19:26:35]

changes lgtm

[stevenjb, 2015-01-22 21:27:15]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2015-01-22 21:27:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/762633002/60001

[commit-bot: I haz the power, 2015-01-22 21:33:50]

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

[commit-bot: I haz the power, 2015-01-22 21:33:51]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win8_chromium_gn_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gn_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[stevenjb, 2015-01-23 01:25:41]

The CQ bit was checked by stevenjb@chromium.org

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

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

[commit-bot: I haz the power, 2015-01-23 03:29:16]

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

[commit-bot: I haz the power, 2015-01-23 03:29:17]

Exceeded time limit waiting for builds to trigger.

[stevenjb, 2015-01-23 18:14:21]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2015-01-23 18:15:04]

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

[commit-bot: I haz the power, 2015-01-23 18:17:33]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-23 18:19:33]

Patchset 5 (id:??) landed as
https://crrev.com/2a54d25447a000fc782ea3a8190f18913ad645b9
Cr-Commit-Position: refs/heads/master@{#312879}