Internals for testing NetInfov3 code

Used for testing NetInfov3 code in upcoming CL https://codereview.chromium.org/291203002

Design doc: https://docs.google.com/a/chromium.org/document/d/1LTk9uVMGi4kurzcF5ellsAJReTF31fFJMHrQwSVtBjc/

BUG=368358
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=174718

[jkarlin, 2014-05-20 17:22:15]

Thanks!

[jkarlin, 2014-05-22 15:32:53]

Ping adamk

[adamk, 2014-05-22 15:40:32]

Sorry, the "DO NOT LAND" warning had me scared to even look at it :)

lgtm since the only thing I ask for is in another repo (TestRunner).

https://codereview.chromium.org/295003003/diff/20001/Source/core/testing/Inte...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/295003003/diff/20001/Source/core/testing/Inte...
Source/core/testing/Internals.cpp:2358:
networkStateNotifier().setWebConnectionType(webtype);
It looks to me like this might leak state between tests that happen to run in
the same renderer. If so, please add some resetting code to TestRunner::Reset()
to make sure each test starts out with the same value:

https://code.google.com/p/chromium/codesearch#chromium/src/content/shell/rend...

https://codereview.chromium.org/295003003/diff/20001/Source/core/testing/Inte...
Source/core/testing/Internals.cpp:2360: }
Nit: Please add a blank line between these two braces (and give the namespace
one a comment while you're there).

[adamk, 2014-05-22 15:42:36]

Also do be sure to remove the DO NOT LAND warning from the top of the CL
description, as that would end up as the top of the git commit...

[jkarlin, 2014-05-22 17:38:01]

https://codereview.chromium.org/295003003/diff/20001/Source/core/testing/Inte...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/295003003/diff/20001/Source/core/testing/Inte...
Source/core/testing/Internals.cpp:2358:
networkStateNotifier().setWebConnectionType(webtype);
On 2014/05/22 15:40:32, adamk wrote:
> It looks to me like this might leak state between tests that happen to run in
> the same renderer. If so, please add some resetting code to
TestRunner::Reset()
> to make sure each test starts out with the same value:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/shell/rend...

Good point that it changes state between runs.  One would expect the network
connection type to change with the testing environment anyway.  So layout tests
that use navigator.connection.type must be agnostic to the initial type.

https://codereview.chromium.org/295003003/diff/20001/Source/core/testing/Inte...
Source/core/testing/Internals.cpp:2360: }
On 2014/05/22 15:40:32, adamk wrote:
> Nit: Please add a blank line between these two braces (and give the namespace
> one a comment while you're there).

Done.

[jkarlin, 2014-05-23 10:37:34]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-05-23 10:37:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jkarlin@chromium.org/295003003/40001

[commit-bot: I haz the power, 2014-05-23 10:37:52]

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

[commit-bot: I haz the power, 2014-05-23 10:37:52]

Failed to apply patch for Source/core/testing/Internals.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/testing/Internals.cpp
  Hunk #1 FAILED at 105.
  Hunk #2 succeeded at 129 (offset 1 line).
  Hunk #3 succeeded at 2341 with fuzz 1 (offset 6 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
Source/core/testing/Internals.cpp.rej

Patch:       Source/core/testing/Internals.cpp
Index: Source/core/testing/Internals.cpp
diff --git a/Source/core/testing/Internals.cpp
b/Source/core/testing/Internals.cpp
index
a48c5ac62dfd1132fd53c4f591b29f5a5215a523..4bca0006ecbdf41a2911d9578ab3698ca304309d
100644
--- a/Source/core/testing/Internals.cpp
+++ b/Source/core/testing/Internals.cpp
@@ -105,6 +105,7 @@
 #include "core/page/Chrome.h"
 #include "core/page/ChromeClient.h"
 #include "core/page/EventHandler.h"
+#include "core/page/NetworkStateNotifier.h"
 #include "core/page/Page.h"
 #include "core/page/PagePopupController.h"
 #include "core/page/PrintContext.h"
@@ -128,6 +129,7 @@
 #include "platform/graphics/filters/FilterOperations.h"
 #include "platform/weborigin/SchemeRegistry.h"
 #include "public/platform/Platform.h"
+#include "public/platform/WebConnectionType.h"
 #include "public/platform/WebGraphicsContext3D.h"
 #include "public/platform/WebGraphicsContext3DProvider.h"
 #include "public/platform/WebLayer.h"
@@ -2334,4 +2336,26 @@ String Internals::textSurroundingNode(Node* node, int x,
int y, unsigned long ma
     return surroundingText.content();
 }
 
+void Internals::setNetworkConnectionInfo(const String& type, ExceptionState&
exceptionState)
+{
+    blink::WebConnectionType webtype;
+    if (type == "cellular") {
+        webtype = blink::ConnectionTypeCellular;
+    } else if (type == "bluetooth") {
+        webtype = blink::ConnectionTypeBluetooth;
+    } else if (type == "ethernet") {
+        webtype = blink::ConnectionTypeEthernet;
+    } else if (type == "wifi") {
+        webtype = blink::ConnectionTypeWifi;
+    } else if (type == "other") {
+        webtype = blink::ConnectionTypeOther;
+    } else if (type == "none") {
+        webtype = blink::ConnectionTypeNone;
+    } else {
+        exceptionState.throwDOMException(NotFoundError,
ExceptionMessages::failedToEnumerate("connection type", type));
+        return;
+    }
+    networkStateNotifier().setWebConnectionType(webtype);
 }
+
+} // namespace WebCore

[jkarlin, 2014-05-23 11:10:36]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-05-23 11:10:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jkarlin@chromium.org/295003003/60001

[commit-bot: I haz the power, 2014-05-23 12:39:49]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/9532)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/8891)

[commit-bot: I haz the power, 2014-05-23 14:07:00]

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

[commit-bot: I haz the power, 2014-05-23 14:07:00]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/8898)

[jkarlin, 2014-05-23 14:16:19]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-05-23 14:16:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jkarlin@chromium.org/295003003/60001

[commit-bot: I haz the power, 2014-05-23 15:42:58]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/8912)

[commit-bot: I haz the power, 2014-05-23 17:05:47]

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

[commit-bot: I haz the power, 2014-05-23 17:05:47]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/8924)

[jkarlin, 2014-05-23 17:13:55]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-05-23 17:14:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jkarlin@chromium.org/295003003/60001

[commit-bot: I haz the power, 2014-05-23 18:36:23]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/8941)

[commit-bot: I haz the power, 2014-05-23 19:58:22]

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

[commit-bot: I haz the power, 2014-05-23 19:58:22]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/8964)

[jkarlin, 2014-05-23 21:30:29]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-05-23 21:31:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jkarlin@chromium.org/295003003/60001

[commit-bot: I haz the power, 2014-05-23 22:09:33]

Change committed as 174718

[adamk, 2014-05-23 22:33:57]

https://codereview.chromium.org/295003003/diff/20001/Source/core/testing/Inte...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/295003003/diff/20001/Source/core/testing/Inte...
Source/core/testing/Internals.cpp:2358:
networkStateNotifier().setWebConnectionType(webtype);
On 2014/05/22 17:38:02, jkarlin wrote:
> On 2014/05/22 15:40:32, adamk wrote:
> > It looks to me like this might leak state between tests that happen to run
in
> > the same renderer. If so, please add some resetting code to
> TestRunner::Reset()
> > to make sure each test starts out with the same value:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/shell/rend...
> 
> Good point that it changes state between runs.  One would expect the network
> connection type to change with the testing environment anyway.  So layout
tests
> that use navigator.connection.type must be agnostic to the initial type.

Not sure what you mean by "One would expect the network connection type to
change"...we've had problems with test flakiness where someone got a test to
pass when initially landed and then test ordering changes later caused it to
fail. Given that there's a default when the process starts up (at least in
layout tests), I'd really like that to be enforced by the test runner.

Or are you saying that due to the state of the machine running these tests, the
connection type will change? Hmm, I could imagine that. But in that case adding
an Internals API seems possibly flaky (if the OS decides to send some event that
causes us to update our connection type).

[jkarlin, 2014-05-27 13:43:14]

On 2014/05/23 22:33:57, adamk wrote:
>
https://codereview.chromium.org/295003003/diff/20001/Source/core/testing/Inte...
> File Source/core/testing/Internals.cpp (right):
> 
>
https://codereview.chromium.org/295003003/diff/20001/Source/core/testing/Inte...
> Source/core/testing/Internals.cpp:2358:
> networkStateNotifier().setWebConnectionType(webtype);
> On 2014/05/22 17:38:02, jkarlin wrote:
> > On 2014/05/22 15:40:32, adamk wrote:
> > > It looks to me like this might leak state between tests that happen to run
> in
> > > the same renderer. If so, please add some resetting code to
> > TestRunner::Reset()
> > > to make sure each test starts out with the same value:
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/shell/rend...
> > 
> > Good point that it changes state between runs.  One would expect the network
> > connection type to change with the testing environment anyway.  So layout
> tests
> > that use navigator.connection.type must be agnostic to the initial type.
> 
> Not sure what you mean by "One would expect the network connection type to
> change"...we've had problems with test flakiness where someone got a test to
> pass when initially landed and then test ordering changes later caused it to
> fail. Given that there's a default when the process starts up (at least in
> layout tests), I'd really like that to be enforced by the test runner.
> 
> Or are you saying that due to the state of the machine running these tests,
the
> connection type will change?

Yes, once crrev.com/298803006 lands the connection type will be set to the
host's type.  Assuming anything about this type in a test is a mistake.

> Hmm, I could imagine that. But in that case adding
> an Internals API seems possibly flaky (if the OS decides to send some event
that
> causes us to update our connection type).

True, if the host changes connection type in the middle of a layout test that
uses monitors navigator.connection then a flake could occur. It looks like the
previous NetInfo implementation also had this potential flake
(crrev.com/13820033). The likelihood of hitting the flake is probably quite low
but it still should be taken care of.  I've filed crbug.com/377736.