Use networkingPrivate.startConnect

Use networkingPrivate.startConnect

This adds checks for unconfigured or non-activated networks to internet_details.js since it no longer uses the checks in network_connect.cc.
It also moves some notification handling from network_connect.cc to network_state_notifier.cc so that it does not rely on network_connect.cc.

BUG=430115
Committed: https://crrev.com/e22e5bcbad2cea941993b32e6cc3b082822a42fc
Cr-Commit-Position: refs/heads/master@{#325108}

[stevenjb, 2015-04-01 00:19:56]

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

[stevenjb, 2015-04-01 00:20:48]

https://codereview.chromium.org/1043343002/diff/1/ui/chromeos/network/network...
File ui/chromeos/network/network_connect.cc (left):

https://codereview.chromium.org/1043343002/diff/1/ui/chromeos/network/network...
ui/chromeos/network/network_connect.cc:216: }
This early exit isn't necessary and just confuses things.

https://codereview.chromium.org/1043343002/diff/1/ui/chromeos/network/network...
ui/chromeos/network/network_connect.cc:222: }
This shouldn't happen, we check for connected networks in the UI.

https://codereview.chromium.org/1043343002/diff/1/ui/chromeos/network/network...
ui/chromeos/network/network_connect.cc:230: }
This shouldn't happen, we check the 'Configured' state before attempting a
connect. If it does we will still see an error notification (handled in
NetworkStateNotifier).

https://codereview.chromium.org/1043343002/diff/1/ui/chromeos/network/network...
ui/chromeos/network/network_connect.cc:236: }
Same here.

https://codereview.chromium.org/1043343002/diff/1/ui/chromeos/network/network...
ui/chromeos/network/network_connect.cc:242:
HandleUnconfiguredNetwork(service_path);
Edge case. The Settings UI will no longer automatically open the configuration
UI when this happens, but a notification will still occur, which seems fine.

https://codereview.chromium.org/1043343002/diff/1/ui/chromeos/network/network...
ui/chromeos/network/network_connect.cc:258:
network_state_notifier_->RemoveConnectNotification();
Moved to NetworkStateNotifier any time a new connection attempt occurs.

[armansito, 2015-04-01 00:41:54]

https://codereview.chromium.org/1043343002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/1043343002/diff/20001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/internet_detail.js:1172: if
(!onc.getActiveValue('Connectable') || (detailsPage.type_ == 'VPN') ||
Won't this lead to a call to 'configureNetwork' if getActiveValue('Connectable')
returns false, regardless of the technology type? Is this intentional?

[stevenjb, 2015-04-01 20:51:19]

Patchset #3 (id:40001) has been deleted

[stevenjb, 2015-04-01 20:53:39]

I did some more extensive testing and found a case where the JS UI didn't
correctly know to show the configuration UI after a bad passphrase because
Shill.Error is transient. I made some small changes to use
NetworkState.last_error_ instead so that we can use similar logic in the JS UI.

PTAL

https://codereview.chromium.org/1043343002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/1043343002/diff/20001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/internet_detail.js:1172: if
(!onc.getActiveValue('Connectable') || (detailsPage.type_ == 'VPN') ||
On 2015/04/01 00:41:54, armansito wrote:
> Won't this lead to a call to 'configureNetwork' if
getActiveValue('Connectable')
> returns false, regardless of the technology type? Is this intentional?

In theory that should be correct; if 'Connectable' is false, for any network
type, then Shill requires additional configuration to connect. In practice we
only support configuration for WiFi/WiMAX/VPN, so I will separate out the logic
to make this more clear.

[stevenjb, 2015-04-03 17:39:26]

ping

[michaelpg, 2015-04-03 17:54:12]

https://codereview.chromium.org/1043343002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/1043343002/diff/60001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/internet_detail.js:1203: }
nit: newline after

https://codereview.chromium.org/1043343002/diff/60001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/internet_detail.js:1205:
PageManager.closeOverlay();
how does this work? I click "log in", hit one of the above cases, a View pops
up. After I close that View do we still need this dialog open?

[stevenjb, 2015-04-06 20:27:12]

PTAL

https://codereview.chromium.org/1043343002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/1043343002/diff/60001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/internet_detail.js:1203: }
On 2015/04/03 17:54:11, michaelpg wrote:
> nit: newline after

Done.

https://codereview.chromium.org/1043343002/diff/60001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/internet_detail.js:1205:
PageManager.closeOverlay();
On 2015/04/03 17:54:11, michaelpg wrote:
> how does this work? I click "log in", hit one of the above cases, a View pops
> up. After I close that View do we still need this dialog open?

Good catch. We should call closeOverlay regardless. I moved the new logic into a
separate function to do that.

[michaelpg, 2015-04-06 21:20:27]

chrome/browser lgtm

[stevenjb, 2015-04-08 20:53:03]

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

[stevenjb, 2015-04-08 20:53:03]

+pbeubeck@ now that you're back

[stevenjb, 2015-04-10 15:59:49]

ping

[pneubeck (no reviews), 2015-04-13 19:16:11]

https://codereview.chromium.org/1043343002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/1043343002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/options/chromeos/internet_detail.js:1196:
DetailsInternetPage.configureOrConnect = function() {
can you explain whether this has the same behavior for unconfigured networks? or
is this code not reached for unconfigured networks?

https://codereview.chromium.org/1043343002/diff/140001/chromeos/network/netwo...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/1043343002/diff/140001/chromeos/network/netwo...
chromeos/network/network_state.cc:234: std::string error_state =
GetErrorState();
This change seems to imply that dictionary returned by GetStateProperties does
not match a subset of GetProperties on the service anymore.
This could (or will very likely?) lead to surprises in the future because of
inconsistencies of the to exposures of a service.

https://codereview.chromium.org/1043343002/diff/140001/chromeos/network/netwo...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/1043343002/diff/140001/chromeos/network/netwo...
chromeos/network/network_state.h:129: // Retirns |error_| if valid, otherwise
returns |last_error_|.
typo: Retirns -> Returns

https://codereview.chromium.org/1043343002/diff/140001/chromeos/network/netwo...
File chromeos/network/network_util.cc (right):

https://codereview.chromium.org/1043343002/diff/140001/chromeos/network/netwo...
chromeos/network/network_util.cc:225: std::string error_state =
network_state->GetErrorState();
this deviates from the GetStateProperties behavior, where you check for
error_state.empty() before setting.

we probably should add a unit test that compares these two translation flows:
Shill properties -> NetworkState -> TranslateNetworkStateToONC (calling
GetStateProperties)
Shill properties -> TranslateShillPropertiesToONC (also getting the NetworkState
that's calling GetStateProperties)

https://codereview.chromium.org/1043343002/diff/140001/chromeos/network/netwo...
File chromeos/network/network_util.h (right):

https://codereview.chromium.org/1043343002/diff/140001/chromeos/network/netwo...
chromeos/network/network_util.h:113: 
can you rename it to include 'Service'?
ShillProperties is very broad.
E.g. ShillService instead of ShillProperties.

https://codereview.chromium.org/1043343002/diff/140001/chromeos/network/netwo...
chromeos/network/network_util.h:114: CHROMEOS_EXPORT
scoped_ptr<base::DictionaryValue> TranslateShillPropertiesToONC(
should have a comment, e.g. "Like TranslateShillServiceToONCPart() for
onc::kNetworkWithStateSignature and additionally integrates cached error state
from the NetworkState of service_path."

as usual, I dislike the hidden dependency on NSH. Instead of a service path,
this function should take a (const NetworkState&) or const* if it's optional.

https://codereview.chromium.org/1043343002/diff/140001/components/device_even...
File components/device_event_log/device_event_log_impl.cc (right):

https://codereview.chromium.org/1043343002/diff/140001/components/device_even...
components/device_event_log/device_event_log_impl.cc:160: const bool show_level
= log_entry.log_level != LOG_LEVEL_ERROR;
unrelated?

[stevenjb, 2015-04-13 21:16:32]

https://codereview.chromium.org/1043343002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/1043343002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/options/chromeos/internet_detail.js:1196:
DetailsInternetPage.configureOrConnect = function() {
On 2015/04/13 19:16:11, pneubeck wrote:
> can you explain whether this has the same behavior for unconfigured networks?
or
> is this code not reached for unconfigured networks?

The intention is that this will identify unconfigured networks ('Connectable' ==
false), and networks in an error state, e.g. with a pad passphrase, and show the
configure dialog for those. I'll add a comment.

https://codereview.chromium.org/1043343002/diff/140001/chromeos/network/netwo...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/1043343002/diff/140001/chromeos/network/netwo...
chromeos/network/network_state.cc:234: std::string error_state =
GetErrorState();
On 2015/04/13 19:16:11, pneubeck wrote:
> This change seems to imply that dictionary returned by GetStateProperties does
> not match a subset of GetProperties on the service anymore.
> This could (or will very likely?) lead to surprises in the future because of
> inconsistencies of the to exposures of a service.

Move the logic to ShillToONCTranslator.

https://codereview.chromium.org/1043343002/diff/140001/chromeos/network/netwo...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/1043343002/diff/140001/chromeos/network/netwo...
chromeos/network/network_state.h:129: // Retirns |error_| if valid, otherwise
returns |last_error_|.
On 2015/04/13 19:16:11, pneubeck wrote:
> typo: Retirns -> Returns

Done.

https://codereview.chromium.org/1043343002/diff/140001/chromeos/network/netwo...
File chromeos/network/network_util.cc (right):

https://codereview.chromium.org/1043343002/diff/140001/chromeos/network/netwo...
chromeos/network/network_util.cc:225: std::string error_state =
network_state->GetErrorState();
On 2015/04/13 19:16:11, pneubeck wrote:
> this deviates from the GetStateProperties behavior, where you check for
> error_state.empty() before setting.
> 
> we probably should add a unit test that compares these two translation flows:
> Shill properties -> NetworkState -> TranslateNetworkStateToONC (calling
> GetStateProperties)
> Shill properties -> TranslateShillPropertiesToONC (also getting the
NetworkState
> that's calling GetStateProperties)

I was trying to avoid adding a dependency on NSH to ShillToONCTranslator, but
upon reflection I think it is better just to add that dependency instead.

https://codereview.chromium.org/1043343002/diff/140001/chromeos/network/netwo...
File chromeos/network/network_util.h (right):

https://codereview.chromium.org/1043343002/diff/140001/chromeos/network/netwo...
chromeos/network/network_util.h:113: 
On 2015/04/13 19:16:11, pneubeck wrote:
> can you rename it to include 'Service'?
> ShillProperties is very broad.
> E.g. ShillService instead of ShillProperties.

Removed instead.

https://codereview.chromium.org/1043343002/diff/140001/chromeos/network/netwo...
chromeos/network/network_util.h:114: CHROMEOS_EXPORT
scoped_ptr<base::DictionaryValue> TranslateShillPropertiesToONC(
On 2015/04/13 19:16:11, pneubeck wrote:
> should have a comment, e.g. "Like TranslateShillServiceToONCPart() for
> onc::kNetworkWithStateSignature and additionally integrates cached error state
> from the NetworkState of service_path."
> 
> as usual, I dislike the hidden dependency on NSH. Instead of a service path,
> this function should take a (const NetworkState&) or const* if it's optional.

I added an explicit dependency on NSH in ShillToONCTranslator instead.

https://codereview.chromium.org/1043343002/diff/140001/components/device_even...
File components/device_event_log/device_event_log_impl.cc (right):

https://codereview.chromium.org/1043343002/diff/140001/components/device_even...
components/device_event_log/device_event_log_impl.cc:160: const bool show_level
= log_entry.log_level != LOG_LEVEL_ERROR;
On 2015/04/13 19:16:11, pneubeck wrote:
> unrelated?

Yes, but trivial (I found it while debugging).

[pneubeck (no reviews), 2015-04-14 09:32:16]

https://codereview.chromium.org/1043343002/diff/160001/chromeos/network/onc/o...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/1043343002/diff/160001/chromeos/network/onc/o...
chromeos/network/onc/onc_translator_shill_to_onc.cc:50: if
(!NetworkHandler::IsInitialized())
On 2015/04/13 21:16:32, stevenjb wrote:
> On 2015/04/13 19:16:11, pneubeck wrote:
> > as usual, I dislike the hidden dependency on NSH. Instead of a service path,
> > this function should take a (const NetworkState&) or const* if it's
optional.
> 
> I added an explicit dependency on NSH in ShillToONCTranslator instead.

This is not explicit though.
If at all, please pass in the dependency explicitly.
Less dependency in form of a NetworkState argument is very much preferable over
the huge dependency on NSH, which is much much harder to fake/mock in tests.

[stevenjb, 2015-04-14 13:04:33]

On 2015/04/14 09:32:16, pneubeck wrote:
>
https://codereview.chromium.org/1043343002/diff/160001/chromeos/network/onc/o...
> File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):
> 
>
https://codereview.chromium.org/1043343002/diff/160001/chromeos/network/onc/o...
> chromeos/network/onc/onc_translator_shill_to_onc.cc:50: if
> (!NetworkHandler::IsInitialized())
> On 2015/04/13 21:16:32, stevenjb wrote:
> > On 2015/04/13 19:16:11, pneubeck wrote:
> > > as usual, I dislike the hidden dependency on NSH. Instead of a service
path,
> > > this function should take a (const NetworkState&) or const* if it's
> optional.
> > 
> > I added an explicit dependency on NSH in ShillToONCTranslator instead.
> 
> This is not explicit though.
> If at all, please pass in the dependency explicitly.
> Less dependency in form of a NetworkState argument is very much preferable
over
> the huge dependency on NSH, which is much much harder to fake/mock in tests.

Since ShillToONCTranslator is not a persistent class, any "passing" of NSH we do
is just (imho) shuffling globals around. Did you have a specific approach in
mind so that we don't go back and forth a bunch on this?

[stevenjb, 2015-04-14 16:23:20]

I think this is probably/hopefully what you were looking for.
PTAL

[pneubeck (no reviews), 2015-04-14 17:37:15]

thanks for getting rid of the implicit dependency.
lgtm with some more comments.

https://codereview.chromium.org/1043343002/diff/200001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/networking_private/chromeos/test.js
(right):

https://codereview.chromium.org/1043343002/diff/200001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:246:
chrome.networkingPrivate.getVisibleNetworks(
would the ErrorState (from shill error or lasterror) appear in the results here
as well?
if so, and if it's intended, you could modify this test case to check that
behavior as well.

https://codereview.chromium.org/1043343002/diff/200001/chromeos/network/onc/o...
File chromeos/network/onc/onc_translator.h (right):

https://codereview.chromium.org/1043343002/diff/200001/chromeos/network/onc/o...
chromeos/network/onc/onc_translator.h:46: CHROMEOS_EXPORT
please update the comment to explain that |network_state| can be null, if not,
information like error state are used for the translation.

[stevenjb, 2015-04-14 18:27:12]

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

[stevenjb, 2015-04-14 18:27:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelpg@chromium.org, pneubeck@chromium.org
Link to the patchset: https://codereview.chromium.org/1043343002/#ps240001
(title: "Fix tests, add comment")

[stevenjb, 2015-04-14 18:27:17]

https://codereview.chromium.org/1043343002/diff/200001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/networking_private/chromeos/test.js
(right):

https://codereview.chromium.org/1043343002/diff/200001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:246:
chrome.networkingPrivate.getVisibleNetworks(
On 2015/04/14 17:37:15, pneubeck wrote:
> would the ErrorState (from shill error or lasterror) appear in the results
here
> as well?
> if so, and if it's intended, you could modify this test case to check that
> behavior as well.
Actually, I am going to change the code that ErrorState is only set if non-empty
to avoid having to add a lot of ErrorState: '' throughout this file
(get{Managed}Properties would have it as well). I think that is cleaner and
easier. I'll make sure that behavior is documented in the spec.

https://codereview.chromium.org/1043343002/diff/200001/chromeos/network/onc/o...
File chromeos/network/onc/onc_translator.h (right):

https://codereview.chromium.org/1043343002/diff/200001/chromeos/network/onc/o...
chromeos/network/onc/onc_translator.h:46: CHROMEOS_EXPORT
On 2015/04/14 17:37:15, pneubeck wrote:
> please update the comment to explain that |network_state| can be null, if not,
> information like error state are used for the translation.

Done.

[commit-bot: I haz the power, 2015-04-14 18:27:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1043343002/240001

[commit-bot: I haz the power, 2015-04-14 19:43:18]

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

[commit-bot: I haz the power, 2015-04-14 19:43:20]

This issue passed the CQ dry run.

[stevenjb, 2015-04-14 19:45:17]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2015-04-14 19:45:55]

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

[commit-bot: I haz the power, 2015-04-14 19:50:28]

Committed patchset #12 (id:240001)

[commit-bot: I haz the power, 2015-04-14 19:51:39]

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

[Mark P, 2015-04-14 22:54:56]

A revert of this CL (patchset #12 id:240001) has been created in
https://codereview.chromium.org/1085993002/ by mpearson@chromium.org.

The reason for reverting is: Causes browser_test failure on Win7 bot.
https://build.chromium.org/p/chromium.win/builders/Win%207%20Tests%20x64%20(1)

[ RUN      ] WebNavigationApiTest.Failures
[4444:4508:0414/144708:WARNING:data_reduction_proxy_config.cc(319)] SPDY proxy
OFF at startup
[4444:2028:0414/144708:ERROR:bluetooth_adapter_win.cc(102)] NOT IMPLEMENTED
[180:3848:0414/144709:WARNING:raw_channel_win.cc(473)] WriteFile: Error (0x5)
while retrieving error. (0xE8)
[180:3848:0414/144709:WARNING:channel.cc(549)] Failed to send message to ack
remove remote endpoint (local ID 2147483650, remote ID 3)
[180:3848:0414/144709:WARNING:channel.cc(315)] RawChannel write error
[180:3848:0414/144709:WARNING:raw_channel_win.cc(473)] WriteFile: Error (0x3B01)
while retrieving error. (0xE8)
[180:3848:0414/144709:WARNING:channel.cc(549)] Failed to send message to ack
remove remote endpoint (local ID 1, remote ID 1)
[180:3848:0414/144709:WARNING:channel.cc(315)] RawChannel write error
[4444:2028:0414/144709:INFO:CONSOLE(0)] "[SUCCESS] nonExistentIframe", source:
chrome-extension://ollhgkgdokpphndfogkepkmblokamlkf/test_failures.html (0)
[4444:2028:0414/144710:INFO:CONSOLE(0)] "[SUCCESS] nonExistentIframeNavigation",
source: chrome-extension://ollhgkgdokpphndfogkepkmblokamlkf/test_failures.html
(0)
[4444:2028:0414/144710:INFO:CONSOLE(0)] "[FAIL] cancel: Received unexpected
event
'onCompleted':{"frameId":0,"processId":0,"tabId":0,"timeStamp":0,"url":"chrome-extension://ollhgkgdokpphndfogkepkmblokamlkf/e.html"}
Error
    at captureEvent
(chrome-extension://ollhgkgdokpphndfogkepkmblokamlkf/framework.js:194:17)
    at chrome-extension://ollhgkgdokpphndfogkepkmblokamlkf/framework.js:219:5
    at EventImpl.dispatchToListener (extensions::event_bindings:395:22)
    at Event.publicClass.(anonymous function) [as dispatchToListener]
(extensions::utils:94:26)
    at EventImpl.dispatch_ (extensions::event_bindings:379:35)
    at dispatchArgs (extensions::event_bindings:247:26)
    at dispatchEvent (extensions::event_bindings:256:7)", source:
chrome-extension://ollhgkgdokpphndfogkepkmblokamlkf/test_failures.html (0)
[4444:2028:0414/144710:INFO:CONSOLE(0)] "[SUCCESS] nonExistent", source:
chrome-extension://ollhgkgdokpphndfogkepkmblokamlkf/test_failures.html (0)
c:\b\build\slave\win_x64_builder\build\src\chrome\browser\extensions\api\web_navigation\web_navigation_apitest.cc(468):
error: Value of: RunExtensionTest("webnavigation/failures")
  Actual: false
Expected: true
Failed 1 of 4 tests
[4444:2028:0414/144710:ERROR:browser_thread.h(263)] DeleteSoon failed on thread
0
[  FAILED  ] WebNavigationApiTest.Failures, where TypeParam =  and GetParam() = 
(2465 ms)
.

[pneubeck (no reviews), 2015-04-15 07:46:45]

https://codereview.chromium.org/1043343002/diff/240001/components/onc/docs/on...
File components/onc/docs/onc_spec.html (right):

https://codereview.chromium.org/1043343002/diff/240001/components/onc/docs/on...
components/onc/docs/onc_spec.html:724: hex - number& gt;
that looks unintentional

[stevenjb, 2015-04-16 17:19:20]

https://codereview.chromium.org/1043343002/diff/240001/components/onc/docs/on...
File components/onc/docs/onc_spec.html (right):

https://codereview.chromium.org/1043343002/diff/240001/components/onc/docs/on...
components/onc/docs/onc_spec.html:724: hex - number& gt;
On 2015/04/15 07:46:45, pneubeck wrote:
> that looks unintentional

Hm, odd. Don't know where that came from. Fixed in re-apply of patch.

[stevenjb, 2015-04-16 17:25:39]

On 2015/04/14 22:54:56, Mark P wrote:
> A revert of this CL (patchset #12 id:240001) has been created in
> https://codereview.chromium.org/1085993002/ by mailto:mpearson@chromium.org.
> 
> The reason for reverting is: Causes browser_test failure on Win7 bot.
> https://build.chromium.org/p/chromium.win/builders/Win%207%20Tests%20x64%20(1)
> 
> [ RUN      ] WebNavigationApiTest.Failures
> [4444:4508:0414/144708:WARNING:data_reduction_proxy_config.cc(319)] SPDY proxy
> OFF at startup
> [4444:2028:0414/144708:ERROR:bluetooth_adapter_win.cc(102)] NOT IMPLEMENTED
> [180:3848:0414/144709:WARNING:raw_channel_win.cc(473)] WriteFile: Error (0x5)
> while retrieving error. (0xE8)
> [180:3848:0414/144709:WARNING:channel.cc(549)] Failed to send message to ack
> remove remote endpoint (local ID 2147483650, remote ID 3)
> [180:3848:0414/144709:WARNING:channel.cc(315)] RawChannel write error
> [180:3848:0414/144709:WARNING:raw_channel_win.cc(473)] WriteFile: Error
(0x3B01)
> while retrieving error. (0xE8)
> [180:3848:0414/144709:WARNING:channel.cc(549)] Failed to send message to ack
> remove remote endpoint (local ID 1, remote ID 1)
> [180:3848:0414/144709:WARNING:channel.cc(315)] RawChannel write error
> [4444:2028:0414/144709:INFO:CONSOLE(0)] "[SUCCESS] nonExistentIframe", source:
> chrome-extension://ollhgkgdokpphndfogkepkmblokamlkf/test_failures.html (0)
> [4444:2028:0414/144710:INFO:CONSOLE(0)] "[SUCCESS]
nonExistentIframeNavigation",
> source: chrome-extension://ollhgkgdokpphndfogkepkmblokamlkf/test_failures.html
> (0)
> [4444:2028:0414/144710:INFO:CONSOLE(0)] "[FAIL] cancel: Received unexpected
> event
>
'onCompleted':{"frameId":0,"processId":0,"tabId":0,"timeStamp":0,"url":"chrome-extension://ollhgkgdokpphndfogkepkmblokamlkf/e.html"}
> Error
>     at captureEvent
> (chrome-extension://ollhgkgdokpphndfogkepkmblokamlkf/framework.js:194:17)
>     at chrome-extension://ollhgkgdokpphndfogkepkmblokamlkf/framework.js:219:5
>     at EventImpl.dispatchToListener (extensions::event_bindings:395:22)
>     at Event.publicClass.(anonymous function) [as dispatchToListener]
> (extensions::utils:94:26)
>     at EventImpl.dispatch_ (extensions::event_bindings:379:35)
>     at dispatchArgs (extensions::event_bindings:247:26)
>     at dispatchEvent (extensions::event_bindings:256:7)", source:
> chrome-extension://ollhgkgdokpphndfogkepkmblokamlkf/test_failures.html (0)
> [4444:2028:0414/144710:INFO:CONSOLE(0)] "[SUCCESS] nonExistent", source:
> chrome-extension://ollhgkgdokpphndfogkepkmblokamlkf/test_failures.html (0)
>
c:\b\build\slave\win_x64_builder\build\src\chrome\browser\extensions\api\web_navigation\web_navigation_apitest.cc(468):
> error: Value of: RunExtensionTest("webnavigation/failures")
>   Actual: false
> Expected: true
> Failed 1 of 4 tests
> [4444:2028:0414/144710:ERROR:browser_thread.h(263)] DeleteSoon failed on
thread
> 0
> [  FAILED  ] WebNavigationApiTest.Failures, where TypeParam =  and GetParam()
= 
> (2465 ms)
> .

For future reference, a link to the specific failure (not just the bot) is
helpful. It was:
https://build.chromium.org/p/chromium.win/builders/Win%207%20Tests%20x64%20%2...

It appears that failure was not actually related; it resolved itself before the
revert was committed. Which is good because it's hard to imagine how this would
have generated a Win7 specific failure.

Will re-land this today.