Abstract code filtering URLs added to the history

Delegate to the embedder the decision to filter URLs to add to the history
through the HistoryClient defaulting to registering all URLs (in the base
class implementation but also when there is no HistoryClient during tests).

BUG=390953
TBR=jochen@chromium.org

Review URL: https://codereview.chromium.org/971423002

Cr-Commit-Position: refs/heads/master@{#319071}

chrome/browser/history/chrome_history_client.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/history/chrome_history_client.h"
 
 #include "base/logging.h"
+#include "chrome/browser/history/history_utils.h"
 #include "chrome/browser/ui/profile_error_dialog.h"
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/grit/chromium_strings.h"
@@ -62,6 +63,10 @@ void ChromeHistoryClient::GetBookmarks(
   }
 }
 
+bool ChromeHistoryClient::CanAddURL(const GURL& url) {
+  return CanAddURLToHistory(url);
+}
+
 void ChromeHistoryClient::NotifyProfileError(sql::InitStatus init_status) {
   ShowProfileErrorDialog(
       PROFILE_ERROR_HISTORY,

chrome/browser/history/chrome_history_client.h

@@ -26,6 +26,7 @@ class ChromeHistoryClient : public history::HistoryClient {
   void BlockUntilBookmarksLoaded() override;
   bool IsBookmarked(const GURL& url) override;
   void GetBookmarks(std::vector<history::URLAndTitle>* bookmarks) override;
+  bool CanAddURL(const GURL& url) override;
   void NotifyProfileError(sql::InitStatus init_status) override;
   bool ShouldReportDatabaseError() override;
 #if defined(OS_ANDROID)

chrome/browser/history/history_service.cc

@@ -31,8 +31,6 @@
 #include "chrome/browser/history/history_backend.h"
 #include "chrome/browser/history/in_memory_history_backend.h"
 #include "chrome/browser/history/in_memory_url_index.h"
-#include "chrome/common/url_constants.h"
-#include "components/dom_distiller/core/url_constants.h"
 #include "components/history/core/browser/download_row.h"
 #include "components/history/core/browser/history_client.h"
 #include "components/history/core/browser/history_database_params.h"
@@ -396,7 +394,7 @@ void HistoryService::AddPage(const history::HistoryAddPageArgs& add_page_args) {
   // large part of history (think iframes for ads) and we never display them in
   // history UI. We will still add manual subframes, which are ones the user
   // has clicked on to get.
-  if (!CanAddURL(add_page_args.url))
+  if (history_client_ && !history_client_->CanAddURL(add_page_args.url))
     return;
 
   // Inform VisitedDelegate of all links and redirects.
@@ -422,7 +420,7 @@ void HistoryService::AddPageNoVisitForBookmark(const GURL& url,
                                                const base::string16& title) {
   DCHECK(thread_) << "History service being called after cleanup";
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (!CanAddURL(url))
+  if (history_client_ && !history_client_->CanAddURL(url))
     return;
 
   ScheduleTask(PRIORITY_NORMAL,
@@ -460,7 +458,7 @@ void HistoryService::AddPageWithDetails(const GURL& url,
   DCHECK(thread_) << "History service being called after cleanup";
   DCHECK(thread_checker_.CalledOnValidThread());
   // Filter out unwanted URLs.
-  if (!CanAddURL(url))
+  if (history_client_ && !history_client_->CanAddURL(url))
     return;
 
   // Inform VisitDelegate of the URL.
@@ -620,7 +618,7 @@ void HistoryService::MergeFavicon(
     const gfx::Size& pixel_size) {
   DCHECK(thread_) << "History service being called after cleanup";
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (!CanAddURL(page_url))
+  if (history_client_ && !history_client_->CanAddURL(page_url))
     return;
 
   ScheduleTask(
@@ -636,7 +634,7 @@ void HistoryService::SetFavicons(
     const std::vector<SkBitmap>& bitmaps) {
   DCHECK(thread_) << "History service being called after cleanup";
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (!CanAddURL(page_url))
+  if (history_client_ && !history_client_->CanAddURL(page_url))
     return;
 
   ScheduleTask(PRIORITY_NORMAL,
@@ -977,31 +975,6 @@ void HistoryService::ScheduleTask(SchedulePriority priority,
   thread_->message_loop()->PostTask(FROM_HERE, task);
 }
 
-// static
-bool HistoryService::CanAddURL(const GURL& url) {
-  if (!url.is_valid())
-    return false;
-
-  // TODO: We should allow kChromeUIScheme URLs if they have been explicitly
-  // typed.  Right now, however, these are marked as typed even when triggered
-  // by a shortcut or menu action.
-  if (url.SchemeIs(url::kJavaScriptScheme) ||
-      url.SchemeIs(content::kChromeDevToolsScheme) ||
-      url.SchemeIs(content::kChromeUIScheme) ||
-      url.SchemeIs(content::kViewSourceScheme) ||
-      url.SchemeIs(chrome::kChromeNativeScheme) ||
-      url.SchemeIs(chrome::kChromeSearchScheme) ||
-      url.SchemeIs(dom_distiller::kDomDistillerScheme))
-    return false;
-
-  // Allow all about: and chrome: URLs except about:blank, since the user may
-  // like to see "chrome://memory/", etc. in their history and autocomplete.
-  if (url == GURL(url::kAboutBlankURL))
-    return false;
-
-  return true;
-}
-
 base::WeakPtr<HistoryService> HistoryService::AsWeakPtr() {
   DCHECK(thread_checker_.CalledOnValidThread());
   return weak_ptr_factory_.GetWeakPtr();

chrome/browser/history/history_service.h

@@ -487,10 +487,6 @@ class HistoryService : public syncer::SyncableService, public KeyedService {
   void AddPagesWithDetails(const history::URLRows& info,
                            history::VisitSource visit_source);
 
-  // Returns true if this looks like the type of URL we want to add to the
-  // history. We filter out some URLs such as JavaScript.
-  static bool CanAddURL(const GURL& url);
-
   base::WeakPtr<HistoryService> AsWeakPtr();
 
   // syncer::SyncableService implementation.
@@ -778,19 +774,19 @@ class HistoryService : public syncer::SyncableService, public KeyedService {
   base::ThreadChecker thread_checker_;
 
   // The thread used by the history service to run complicated operations.
-  // |thread_| is NULL once |Cleanup| is NULL.
+  // |thread_| is null once Cleanup() is called.
   base::Thread* thread_;
 
   // This class has most of the implementation and runs on the 'thread_'.
   // You MUST communicate with this class ONLY through the thread_'s
   // message_loop().
   //
-  // This pointer will be NULL once Cleanup() has been called, meaning no
+  // This pointer will be null once Cleanup() has been called, meaning no
   // more calls should be made to the history thread.
   scoped_refptr<history::HistoryBackend> history_backend_;
 
   // A cache of the user-typed URLs kept in memory that is used by the
-  // autocomplete system. This will be NULL until the database has been created
+  // autocomplete system. This will be null until the database has been created
   // on the background thread.
   // TODO(mrossetti): Consider changing ownership. See http://crbug.com/138321
   scoped_ptr<history::InMemoryHistoryBackend> in_memory_backend_;

chrome/browser/history/history_utils.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/history/history_utils.h"
+
+#include "chrome/common/url_constants.h"
+#include "components/dom_distiller/core/url_constants.h"
+#include "url/gurl.h"
+
+bool CanAddURLToHistory(const GURL& url) {
+  if (!url.is_valid())
+    return false;
+
+  // TODO: We should allow kChromeUIScheme URLs if they have been explicitly
+  // typed.  Right now, however, these are marked as typed even when triggered
+  // by a shortcut or menu action.
+  if (url.SchemeIs(url::kJavaScriptScheme) ||
+      url.SchemeIs(content::kChromeDevToolsScheme) ||
+      url.SchemeIs(content::kChromeUIScheme) ||
+      url.SchemeIs(content::kViewSourceScheme) ||
+      url.SchemeIs(chrome::kChromeNativeScheme) ||
+      url.SchemeIs(chrome::kChromeSearchScheme) ||
+      url.SchemeIs(dom_distiller::kDomDistillerScheme))
+    return false;
+
+  // Allow all about: and chrome: URLs except about:blank, since the user may
+  // like to see "chrome://memory/", etc. in their history and autocomplete.
+  if (url == GURL(url::kAboutBlankURL))
+    return false;
+
+  return true;
+}

chrome/browser/history/history_utils.h

+// 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.
+
+#ifndef CHROME_BROWSER_HISTORY_HISTORY_UTILS_H_
+#define CHROME_BROWSER_HISTORY_HISTORY_UTILS_H_
+
+class GURL;
+
+// Returns true if this looks like the type of URL that should be added to the
+// history. This filters out URLs such a JavaScript.
+bool CanAddURLToHistory(const GURL& url);
+
+#endif  // CHROME_BROWSER_HISTORY_HISTORY_UTILS_H_

chrome/browser/history/top_sites_impl.cc

@@ -24,6 +24,7 @@
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/history/history_backend.h"
 #include "chrome/browser/history/history_service_factory.h"
+#include "chrome/browser/history/history_utils.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/pref_names.h"
 #include "components/history/core/browser/history_db_task.h"
@@ -149,7 +150,7 @@ bool TopSitesImpl::SetPageThumbnail(const GURL& url,
     }
   }
 
-  if (!HistoryService::CanAddURL(url))
+  if (!CanAddURLToHistory(url))
     return false;  // It's not a real webpage.
 
   scoped_refptr<base::RefCountedBytes> thumbnail_data;
@@ -188,7 +189,7 @@ bool TopSitesImpl::SetPageThumbnailToJPEGBytes(
     }
   }
 
-  if (!HistoryService::CanAddURL(url))
+  if (!CanAddURLToHistory(url))
     return false;  // It's not a real webpage.
 
   if (add_temp_thumbnail) {
@@ -743,7 +744,7 @@ void TopSitesImpl::Observe(int type,
       if (!load_details)
         return;
       const GURL& url = load_details->entry->GetURL();
-      if (!cache_->IsKnownURL(url) && HistoryService::CanAddURL(url)) {
+      if (!cache_->IsKnownURL(url) && CanAddURLToHistory(url)) {
         // To avoid slamming history we throttle requests when the url updates.
         // To do otherwise negatively impacts perf tests.
         RestartQueryForTopSitesTimer(GetUpdateDelay());

chrome/browser/thumbnails/thumbnail_service_impl.cc

@@ -7,7 +7,7 @@
 #include "base/command_line.h"
 #include "base/memory/ref_counted_memory.h"
 #include "base/time/time.h"
-#include "chrome/browser/history/history_service.h"
+#include "chrome/browser/history/history_utils.h"
 #include "chrome/browser/history/top_sites_factory.h"
 #include "chrome/browser/thumbnails/content_based_thumbnailing_algorithm.h"
 #include "chrome/browser/thumbnails/simple_thumbnail_crop.h"
@@ -99,7 +99,7 @@ bool ThumbnailServiceImpl::ShouldAcquirePageThumbnail(const GURL& url) {
     return false;
 
   // Skip if the given URL is not appropriate for history.
-  if (!HistoryService::CanAddURL(url))
+  if (!CanAddURLToHistory(url))
     return false;
   // Skip if the top sites list is full, and the URL is not known.
   if (local_ptr->IsNonForcedFull() && !local_ptr->IsKnownURL(url))

chrome/chrome_browser.gypi

@@ -1543,6 +1543,8 @@
       'browser/history/history_service_factory.h',
       'browser/history/history_tab_helper.cc',
       'browser/history/history_tab_helper.h',
+      'browser/history/history_utils.cc',
+      'browser/history/history_utils.h',
       'browser/history/in_memory_history_backend.cc',
       'browser/history/in_memory_history_backend.h',
       'browser/history/in_memory_url_index.cc',

components/history/core/browser/history_client.cc

@@ -19,6 +19,10 @@ bool HistoryClient::IsBookmarked(const GURL& url) {
 void HistoryClient::GetBookmarks(std::vector<URLAndTitle>* bookmarks) {
 }
 
+bool HistoryClient::CanAddURL(const GURL& url) {
+  return true;
+}
+
 void HistoryClient::NotifyProfileError(sql::InitStatus init_status) {
 }
 

components/history/core/browser/history_client.h

@@ -52,6 +52,10 @@ class HistoryClient : public KeyedService {
   // If not on the main thread, then BlockUntilBookmarksLoaded must be called.
   virtual void GetBookmarks(std::vector<URLAndTitle>* bookmarks);
 
+  // Returns true if this look like the type of URL that should be added to the
+  // history.
+  virtual bool CanAddURL(const GURL& url);
+
   // Notifies the embedder that there was a problem reading the database.
   //
   // Must be called from the main thread.