[New Tab Page] Change the mechanism to intercept online NTP errors

chrome/browser/ui/search/new_tab_page_interceptor.cc

+// 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 "chrome/browser/ui/search/new_tab_page_interceptor.h"
+
+#include "base/metrics/histogram.h"
+#include "chrome/browser/search/search.h"
+#include "chrome/common/url_constants.h"
+#include "content/public/browser/browser_thread.h"
+#include "net/http/http_status_code.h"
+#include "net/url_request/url_request.h"
+#include "net/url_request/url_request_job.h"
+#include "net/url_request/url_request_redirect_job.h"
+
+namespace {
+
+// Log a histogram for NTP load failure.
+void RecordNTPLoadFailure() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  UMA_HISTOGRAM_ENUMERATION("InstantExtended.CacheableNTPLoad",
+                            chrome::CACHEABLE_NTP_LOAD_FAILED,
+                            chrome::CACHEABLE_NTP_LOAD_MAX);
+}
+
+}  // namespace
+
+NewTabPageInterceptor::NewTabPageInterceptor(const GURL& new_tab_url)
+    : new_tab_url_(new_tab_url), weak_factory_(this) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+}
+
+NewTabPageInterceptor::~NewTabPageInterceptor() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+}
+
+base::WeakPtr<NewTabPageInterceptor> NewTabPageInterceptor::GetWeakPtr() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  return weak_factory_.GetWeakPtr();
+}
+
+void NewTabPageInterceptor::SetNewTabPageURL(const GURL& new_tab_page_url) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  new_tab_url_ = new_tab_page_url;
+}
+
+net::URLRequestJob* NewTabPageInterceptor::MaybeInterceptRequest(
+    net::URLRequest* request,
+    net::NetworkDelegate* network_delegate) const {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  return nullptr;
+}
+
+net::URLRequestJob* NewTabPageInterceptor::MaybeInterceptResponse(
+    net::URLRequest* request,
+    net::NetworkDelegate* network_delegate) const {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  if ((request->url() != new_tab_url_) ||
+      (request->url() == GURL(chrome::kChromeSearchLocalNtpUrl))) {

[mmenke, 2015/01/15 22:35:30]

optional:  The second comparison left me scratching my head.  Suggest second
comparison may be a little clearer as:

new_tab_url_ == GURL(chrome::kChromeSearchLocalNtpUrl)

With maybe a comment.


Alternatively, think making these the last checks instead of the first, putting
it right next to where we make the URLRequestRedirectJob, would work as well.

[Mathieu, 2015/01/22 19:21:58]

Done with your first comment.

I think the point with putting them at the top is that it would be the first
thing we check, since we expect it to trigger for all navigations.

+    return nullptr;
+  }
+  // User has canceled this navigation so it shouldn't be redirected.
+  if (request->status().status() == net::URLRequestStatus::CANCELED) {

[mmenke, 2015/01/15 22:35:30]

Should also check for status() == net::URLRequestStatus::FAILED && error() ==
ERR_ABORTED here.

[Mathieu, 2015/01/22 19:21:58]

Done.

+    return nullptr;
+  }
+
+  if (!request->status().is_success() ||
+      request->GetResponseCode() == net::HTTP_NO_CONTENT ||
+      request->GetResponseCode() >= 400) {

[mmenke, 2015/01/15 22:35:30]

optional:  Early return is generally preferred.

[Mathieu, 2015/01/22 19:21:58]

Done.

+    content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+                                     base::Bind(&RecordNTPLoadFailure));

[mmenke, 2015/01/15 22:35:30]

Histograms are threadsafe, so no reason to post a task here.

[Mathieu, 2015/01/22 19:21:58]

Done.

+    return new net::URLRequestRedirectJob(
+        request, network_delegate, GURL(chrome::kChromeSearchLocalNtpUrl),
+        net::URLRequestRedirectJob::REDIRECT_307_TEMPORARY_REDIRECT,
+        "NTP Request Interceptor");
+  }
+  return nullptr;
+}