Add the client for accessing Subresource Filter only list.

components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc

Index: components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc
diff --git a/components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc b/components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc
new file mode 100644
index 0000000000000000000000000000000000000000..de80f1b9e3dab3775683c083be31fe4debfda777
--- /dev/null
+++ b/components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc
@@ -0,0 +1,145 @@
+// Copyright (c) 2017 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 "components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h"
+
+#include "base/memory/ptr_util.h"

[engedy, 2017/03/10 14:39:32]

nit: I think this is not used.

[melandory, 2017/03/15 13:41:36]

Done.

+#include "base/timer/timer.h"
+#include "components/safe_browsing_db/v4_local_database_manager.h"
+#include "components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/navigation_handle.h"
+#include "content/public/browser/web_contents.h"
+
+namespace {
+
+// Maximum time in milliseconds to wait for the safe browsing service to
+// verify a URL. After this amount of time the outstanding check will be
+// aborted, and the URL will be treated as if it doesn't belong to the
+// Subresource Filter only list.
+const int kCheckUrlTimeoutMs = 5000;
+
+}  // namespace
+
+namespace subresource_filter {
+
+class SubresourceFilterSafeBrowsingActivationThrottle::ActivationClient

[engedy, 2017/03/10 14:39:32]

nit: Given this is the client of the SB database, how about calling the class
SBDatabaseClient, its instance |database_client|?

[melandory, 2017/03/15 13:41:36]

Done.

+    : public safe_browsing::SafeBrowsingDatabaseManager::Client {
+ public:
+  ActivationClient(
+      const scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager>&
+          database_manager,
+      base::WeakPtr<SubresourceFilterSafeBrowsingActivationThrottle> throttle,
+      scoped_refptr<base::SingleThreadTaskRunner> task_runner)

[engedy, 2017/03/10 14:39:32]

nit: s//callback_task_runner/g

Alternatively: use content::BrowserThread::PostTask(content::BrowserThread::UI,
...) directly, if it does not make testing really hard.

[melandory, 2017/03/15 13:41:36]

Done.

+      : database_manager_(database_manager),
+        throttle_(throttle),
+        task_runner_(task_runner) {
+    thread_checker_.DetachFromThread();
+  }
+
+  ~ActivationClient() override {
+    DCHECK(thread_checker_.CalledOnValidThread());

[engedy, 2017/03/10 14:39:32]

Given that this class not only needs to be called from one thread, but that
thread has to be the IO thread, let's replace the ThreadChecker with:

DCHECK_CURRENTLY_ON(content::BrowserThread::IO);

[engedy, 2017/03/10 14:39:32]

Need to call CancelCheck here as well in case we get destroyed while both the SB
check and the the timer is still running. Not sure if this could happen, I can
image it could if the WebContents is closed while we are DERER'ing.

[melandory, 2017/03/15 13:41:36]

Done.

[melandory, 2017/03/15 13:41:36]

Done.

+  }
+
+  void CheckUrlOnIO(const GURL& url) {
+    DCHECK(thread_checker_.CalledOnValidThread());

[engedy, 2017/03/10 14:39:32]

nit: Add DCHECK(url_being_checked.is_empty()); to declare no re-entrancy.

[melandory, 2017/03/15 13:41:36]

Done.

+    if (database_manager_->CheckUrlForSubresourceFilter(url, this)) {

[engedy, 2017/03/10 14:39:32]

The comment above this method is a little unclear, but it seems like we need to
handle both true/false return values here?

[melandory, 2017/03/15 13:41:36]

Yep. It actually should be other way around. Unit test are coming (but separate
patch and they actually caught that)

+      url_being_checked_ = url;
+      task_runner_->PostTask(

[engedy, 2017/03/10 14:39:32]

nit: Could we just call into OnCheckBrowserUrlResult from here? Stopping a
non-started timer is defined as a no-op.

[melandory, 2017/03/15 13:41:36]

Done.

+          FROM_HERE,
+          base::Bind(&subresource_filter::
+                         SubresourceFilterSafeBrowsingActivationThrottle::
+                             OnCheckUrlResultOnUI,
+                     throttle_, url, safe_browsing::SB_THREAT_TYPE_SAFE,
+                     safe_browsing::ThreatPatternType::NONE));
+      timer_.Start(FROM_HERE,
+                   base::TimeDelta::FromMilliseconds(kCheckUrlTimeoutMs), this,
+                   &SubresourceFilterSafeBrowsingActivationThrottle::
+                       ActivationClient::OnCheckUrlTimeout);
+    }
+  }
+
+  void OnCheckBrowseUrlResult(
+      const GURL& url,
+      safe_browsing::SBThreatType threat_type,
+      const safe_browsing::ThreatMetadata& metadata) override {
+    DCHECK(thread_checker_.CalledOnValidThread());
+    timer_.Stop();  // Cancel the timeout timer.
+    task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&SubresourceFilterSafeBrowsingActivationThrottle::
+                       OnCheckUrlResultOnUI,
+                   throttle_, url, threat_type, metadata.threat_pattern_type));
+  }
+
+  // Callback for when the safe browsing check has taken longer than
+  // kCheckUrlTimeoutMs.
+  void OnCheckUrlTimeout() {
+    DCHECK(thread_checker_.CalledOnValidThread());
+    database_manager_->CancelCheck(this);
+
+    OnCheckBrowseUrlResult(url_being_checked_,
+                           safe_browsing::SB_THREAT_TYPE_SAFE,
+                           safe_browsing::ThreatMetadata());
+  }
+
+ private:
+  scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> database_manager_;
+  // Timer to abort the safe browsing check if it takes too long.

[engedy, 2017/03/10 14:39:32]

nit: blank line above comment

[melandory, 2017/03/15 13:41:36]

Done.

+  base::OneShotTimer timer_;
+  GURL url_being_checked_;
+  base::ThreadChecker thread_checker_;
+
+  base::WeakPtr<SubresourceFilterSafeBrowsingActivationThrottle> throttle_;
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(ActivationClient);
+};
+
+SubresourceFilterSafeBrowsingActivationThrottle::
+    SubresourceFilterSafeBrowsingActivationThrottle(
+        content::NavigationHandle* handle,
+        const scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager>&
+            database_manager)
+    : NavigationThrottle(handle),
+      task_runner_(content::BrowserThread::GetTaskRunnerForThread(
+          content::BrowserThread::IO)),
+      activation_client_(
+          new SubresourceFilterSafeBrowsingActivationThrottle::ActivationClient(
+              database_manager,
+              AsWeakPtr(),
+              base::ThreadTaskRunnerHandle::Get()),
+          base::OnTaskRunnerDeleter(task_runner_)) {}
+
+SubresourceFilterSafeBrowsingActivationThrottle::
+    ~SubresourceFilterSafeBrowsingActivationThrottle() {}
+
+content::NavigationThrottle::ThrottleCheckResult
+SubresourceFilterSafeBrowsingActivationThrottle::WillProcessResponse() {
+  task_runner_->PostTask(

[engedy, 2017/03/10 14:39:32]

nit: Could post directly to
content::BrowserThread::PostTask(content::BrowserThread::IO, ...) if does not
make testing hard.

[melandory, 2017/03/15 13:41:36]

I like to keep task_runner if you don't have strong feeling.

+      FROM_HERE, base::Bind(&SubresourceFilterSafeBrowsingActivationThrottle::
+                                ActivationClient::CheckUrlOnIO,
+                            base::Unretained(activation_client_.get()),
+                            navigation_handle()->GetURL()));
+  return content::NavigationThrottle::ThrottleCheckResult::DEFER;
+}
+
+void SubresourceFilterSafeBrowsingActivationThrottle::OnCheckUrlResultOnUI(
+    const GURL& url,
+    safe_browsing::SBThreatType threat_type,
+    safe_browsing::ThreatPatternType pattern_type) {
+  content::WebContents* web_contents = navigation_handle()->GetWebContents();
+  if (web_contents) {
+    using subresource_filter::ContentSubresourceFilterDriverFactory;
+    ContentSubresourceFilterDriverFactory* driver_factory =
+        ContentSubresourceFilterDriverFactory::FromWebContents(web_contents);
+    DCHECK(driver_factory);
+
+    driver_factory->OnMainResourceMatchedSafeBrowsingBlacklist(
+        url, std::vector<GURL>(), threat_type, pattern_type);
+  }
+  navigation_handle()->Resume();
+}
+
+}  //  namespace subresource_filter