[Android WebView] Call shouldInterceptRequest on a background thread

android_webview/browser/aw_request_handler.cc

Index: android_webview/browser/aw_request_handler.cc
diff --git a/android_webview/browser/aw_request_handler.cc b/android_webview/browser/aw_request_handler.cc
new file mode 100644
index 0000000000000000000000000000000000000000..986c54f4a6c2d48d84b0d6c3c5da34b75ca3d695
--- /dev/null
+++ b/android_webview/browser/aw_request_handler.cc
@@ -0,0 +1,78 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "android_webview/browser/aw_request_handler.h"
+
+#include "android_webview/browser/aw_contents_io_thread_client.h"
+#include "android_webview/browser/aw_web_resource_response.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/resource_request_info.h"
+#include "net/url_request/url_request.h"
+#include "net/url_request/url_request_job.h"
+
+namespace android_webview {
+
+AwRequestHandler::AwRequestHandler() {
+}
+
+AwRequestHandler::~AwRequestHandler() {
+}

[mmenke, 2015/09/29 15:56:05]

nit:  Definition order should match declaration order.

[mnaganov (inactive), 2015/09/29 23:45:35]

Not relevant anymore.

+
+AndroidStreamReaderURLRequestJob* AwRequestHandler::MaybeInterceptRequest(
+    net::URLRequest* request,
+    net::NetworkDelegate* network_delegate) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  // MaybeInterceptRequest can be called several times for the same request,
+  // for example because a call to URLRequestJob::NotifyRestartRequired has
+  // been made.
+  if (job_.get()) return nullptr;

[mmenke, 2015/09/29 15:56:05]

nit:  .get() not needed.

[mmenke, 2015/09/29 15:56:05]

This should be split on to two lines.

[mnaganov (inactive), 2015/09/29 23:45:35]

Not relevant anymore.

[mnaganov (inactive), 2015/09/29 23:45:35]

Done.

+
+  if (!QueryForAwWebResourceResponse(request))
+    return nullptr;
+  job_ = new AndroidStreamReaderURLRequestJob(request, network_delegate);

[mmenke, 2015/09/29 15:56:05]

We're planning on getting rid of refcounting for URLRequestJobs, because
refcounting is, in general, a really bad idea.  Please don't hold onto a
reference to the job.

[mnaganov (inactive), 2015/09/29 23:45:35]

Done. Now only a weak reference to the job is held.

+  return job_.get();
+}
+
+bool AwRequestHandler::QueryForAwWebResourceResponse(net::URLRequest* request) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+  int render_process_id, render_frame_id;
+  if (!content::ResourceRequestInfo::GetRenderFrameForRequest(
+          request, &render_process_id, &render_frame_id)) {
+    return false;
+  }
+
+  scoped_ptr<AwContentsIoThreadClient> io_thread_client =
+      AwContentsIoThreadClient::FromID(render_process_id, render_frame_id);
+
+  if (!io_thread_client.get())
+    return false;
+
+  GURL referrer(request->referrer());
+  if (referrer.is_valid() &&
+      (!request->is_pending() || request->is_redirecting())) {
+    request->SetExtraRequestHeaderByName(net::HttpRequestHeaders::kReferer,
+                                         referrer.spec(), true);
+  }
+  io_thread_client->ShouldInterceptRequestAsync(
+      request,
+      base::Bind(&AwRequestHandler::QueryForAwWebResourceResponseDone, this));
+  return true;
+}
+
+void AwRequestHandler::QueryForAwWebResourceResponseDone(
+    scoped_ptr<AwWebResourceResponse> response) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  if (!job_.get()) return;

[mmenke, 2015/09/29 15:56:05]

job_ can't be NULL here - this should really just be a DCHECK.

[mnaganov (inactive), 2015/09/29 23:45:35]

Not relevant anymore.

+
+  if (response) {
+    // The job will own the AwWebResourceResponse.
+    AwWebResourceResponse::JobDeliverResponseFromEmbedder(job_.get(),
+                                                          response.Pass());

[mmenke, 2015/09/29 15:56:05]

optional:  This seems a bit roundabout.  We're calling directly into the job
everywhere else.  Seems to make more sense to just pass the response to the job
directly, and have it create the StreamReaderJobDelegateImpl itself (Could even
make StreamReaderJobDelegateImpl public, and have it call the constructor
directly, rather than a static method create it, though I haven't dug into the
reason for the extra method).

[mnaganov (inactive), 2015/09/29 23:45:35]

Right. Perhaps there have been historical reasons, but it looks like now we can
change the code to avoid that. Thanks for the suggestion!

+  } else {
+    job_->DeliverResponseFromNetwork();

[mmenke, 2015/09/29 15:56:05]

BUG:  The request could be cancelled by this point.  You should probably just
use a weak pointer in your bind call.

[mnaganov (inactive), 2015/09/29 23:45:35]

Done.

+  }
+}
+
+}  // namespace android_webview