Expose SPDY pushes in DevTools

chrome/browser/devtools/devtools_sanity_browsertest.cc

Index: chrome/browser/devtools/devtools_sanity_browsertest.cc
diff --git a/chrome/browser/devtools/devtools_sanity_browsertest.cc b/chrome/browser/devtools/devtools_sanity_browsertest.cc
index 2613d684f3773d15e91d2251abbd2152cc64ca04..861c35381b075a4fa6906896d68d92fa45365c92 100644
--- a/chrome/browser/devtools/devtools_sanity_browsertest.cc
+++ b/chrome/browser/devtools/devtools_sanity_browsertest.cc
@@ -64,6 +64,9 @@
 #include "extensions/common/value_builder.h"
 #include "net/dns/mock_host_resolver.h"
 #include "net/test/spawned_test_server/spawned_test_server.h"
+#include "net/url_request/url_request_context.h"
+#include "net/url_request/url_request_filter.h"
+#include "net/url_request/url_request_http_job.h"
 #include "third_party/WebKit/public/web/WebInputEvent.h"
 #include "ui/compositor/compositor_switches.h"
 #include "ui/gl/gl_switches.h"
@@ -94,6 +97,9 @@ const char kNavigateBackTestPage[] =
 const char kWindowOpenTestPage[] = "files/devtools/window_open.html";
 const char kLatencyInfoTestPage[] = "files/devtools/latency_info.html";
 const char kChunkedTestPage[] = "chunked";
+const char kPushTestPage[] = "files/devtools/push_test_page.html";
+// The resource is not really pushed, but mock url request job pretends it is.
+const char kPushTestResource[] = "files/devtools/image.png";
 const char kSlowTestPage[] =
     "chunked?waitBeforeHeaders=100&waitBetweenChunks=100&chunksNumber=2";
 const char kSharedWorkerTestPage[] =
@@ -165,6 +171,61 @@ void SwitchToExtensionPanel(DevToolsWindow* window,
   SwitchToPanel(window, (prefix + panel_name).c_str());
 }
 
+class PushTimesMockURLRequestJob : public net::URLRequestHttpJob {

[mmenke, 2016/04/08 16:21:36]

I'd rather this not inherit from URLRequestHttpJob, which it should be noted is
not intended for public consumption (See the "NET_EXPORT_PRIVATE" as a hint). 
We should just inherit from URLRequestMockHTTPJob (Which does require we give
the file path to the mock job).

[caseq, 2016/04/08 22:39:28]

Done. Note I also took a liberty to nuke another subclass of it,
URLRequestNonsecureConnection as a drive by.

+ public:
+  PushTimesMockURLRequestJob(net::URLRequest* request,
+                             net::NetworkDelegate* network_delegate)
+      : net::URLRequestHttpJob(request,
+                               network_delegate,
+                               request->context()->http_user_agent_settings()) {
+  }
+
+  void GetLoadTimingInfo(net::LoadTimingInfo* load_timing_info) const override {
+    net::URLRequestHttpJob::GetLoadTimingInfo(load_timing_info);
+    if (push_end_.is_null())
+      push_end_ = base::TimeTicks::Now();
+    load_timing_info->push_start =
+        request()->creation_time() - base::TimeDelta::FromMilliseconds(10);
+    load_timing_info->push_end = push_end_;
+  }
+
+ private:
+  mutable base::TimeTicks push_end_;
+  DISALLOW_COPY_AND_ASSIGN(PushTimesMockURLRequestJob);
+};
+
+class TestInterceptor : public net::URLRequestInterceptor {
+ public:
+  explicit TestInterceptor(const GURL& url) : url_(url) {}
+
+  // Registers |this| with the URLRequestFilter, which takes ownership of it.
+  void Register() {

[mmenke, 2016/04/08 16:21:36]

Suggest making this static, and having it create the TestInterceptor, and give
the filter ownership of it.  scoped_ptr(this) is just weird.

[caseq, 2016/04/08 22:39:28]

Done.

+    EXPECT_TRUE(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    net::URLRequestFilter::GetInstance()->AddUrlInterceptor(
+        url_, scoped_ptr<net::URLRequestInterceptor>(this));

[mmenke, 2016/04/08 16:21:36]

scoped_ptr<net::URLRequestInterceptor> -> make_scoped_ptr?

[caseq, 2016/04/08 22:39:28]

Done.

+  }
+
+  // Unregisters |this| with the URLRequestFilter, which should then delete
+  // |this|.
+  void Unregister() {

[mmenke, 2016/04/08 16:21:36]

Again, suggest making this static.  Does mean you'll need to pass in URL again,
or just clear all handlers.

[caseq, 2016/04/08 22:39:28]

Done.

+    EXPECT_TRUE(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    net::URLRequestFilter::GetInstance()->RemoveUrlHandler(url_);
+  }
+
+  // net::URLRequestJobFactory::ProtocolHandler implementation:
+  net::URLRequestJob* MaybeInterceptRequest(
+      net::URLRequest* request,
+      net::NetworkDelegate* network_delegate) const override {
+    EXPECT_TRUE(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    return new PushTimesMockURLRequestJob(request, network_delegate);
+  }
+
+ private:
+  GURL url_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestInterceptor);
+};
+
 }  // namespace
 
 class DevToolsSanityTest : public InProcessBrowserTest {
@@ -942,6 +1003,23 @@ IN_PROC_BROWSER_TEST_F(DevToolsSanityTest, TestNetworkRawHeadersText) {
   RunTest("testNetworkRawHeadersText", kChunkedTestPage);
 }
 
+// Tests raw headers text.
+IN_PROC_BROWSER_TEST_F(DevToolsSanityTest, TestNetworkPushTime) {
+  OpenDevToolsWindow(kPushTestPage, false);
+  GURL push_url = spawned_test_server()->GetURL(kPushTestResource);
+  TestInterceptor* test_interceptor = new TestInterceptor(push_url);
+  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+                          base::Bind(&TestInterceptor::Register,
+                                     base::Unretained(test_interceptor)));
+
+  DispatchOnTestSuite(window_, "testPushTimes", push_url.spec().c_str());

[mmenke, 2016/04/08 16:21:36]

Can we check that the push time is less that send_start...Or better, get a time
before the navigation starts, subtract 10 seconds from it, and check that the
push time is before the earliest normally reported time (navigation start?  Some
other start time?  Not too familiar with the timing APIs)

[caseq, 2016/04/08 22:39:28]

That's exactly what happens in testPushTime() -- have you had a chance to look
at Tests.js?
Well, as for navigation start -- I would expect it to always come before the
push time unless the page is being reloaded, since we would normally push only
after the main resource was requested and the connection was opened.

[mmenke, 2016/04/11 17:45:52]

Wait....So a chrome/browser/devtools/ test is using test code in
third_party/WebKit/Source/devtools/front_end/Tests.js?  That seems very unusual.
 Chrome browsertest-specific Javascript usually lives in chrome/test/data.

I think someone else should review the wiring to the Javascript test stuff here.

[caseq, 2016/04/13 21:48:45]

It's been this way since about the time we introduced browser tests for
devtools. The original motivation is that the JS code there is tightly coupled
to the devtools front-end and less coupled to the test invocations on the
browser side. It's less of a matter since blink repo merge, but I wouldn't want
to change this in this CL.
Understood -- I think dgozman@ has good expertise there and he's already on the
list of reviewers.

[mmenke, 2016/04/14 15:50:33]

The question isn't what's common, or what we expect, but what's possible.  Bugs
often crop up in obscure cases that are unexpected, but still possible.

[mmenke, 2016/04/14 15:50:33]

Happy to defer to him on that, but he should take another look, now that you've
added tests.

+
+  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+                          base::Bind(&TestInterceptor::Unregister,
+                                     base::Unretained(test_interceptor)));
+  CloseDevToolsWindow();
+}
+
 // Tests that console messages are not duplicated on navigation back.
 #if defined(OS_WIN)
 // Flaking on windows swarm try runs: crbug.com/409285.