Improve PasswordProtectionService and PasswordProtectionRequest

components/safe_browsing/password_protection/password_protection_request.h

Index: components/safe_browsing/password_protection/password_protection_request.h
diff --git a/components/safe_browsing/password_protection/password_protection_request.h b/components/safe_browsing/password_protection/password_protection_request.h
index 52be0c11eb444afd7f49c03d47cda3df0ca10d26..f90eb74dee993c21a06529290eafac97f2ee61f7 100644
--- a/components/safe_browsing/password_protection/password_protection_request.h
+++ b/components/safe_browsing/password_protection/password_protection_request.h
@@ -7,7 +7,9 @@
 
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "base/task/cancelable_task_tracker.h"
 #include "components/safe_browsing/password_protection/password_protection_service.h"
+#include "content/public/browser/browser_thread.h"
 #include "net/url_request/url_fetcher.h"
 #include "net/url_request/url_fetcher_delegate.h"
 #include "net/url_request/url_request_status.h"
@@ -19,7 +21,9 @@ namespace safe_browsing {
 // A request for checking if an unfamiliar login form or a password reuse event
 // is safe. PasswordProtectionRequest objects are owned by
 // PasswordProtectionService indicated by |password_protection_service_|.
-class PasswordProtectionRequest : public net::URLFetcherDelegate {
+class PasswordProtectionRequest
+    : public base::RefCountedThreadSafe<PasswordProtectionRequest>,
+      public net::URLFetcherDelegate {
  public:
   // The outcome of the request. These values are used for UMA.
   // DO NOT CHANGE THE ORDERING OF THESE VALUES.
@@ -39,14 +43,12 @@ class PasswordProtectionRequest : public net::URLFetcherDelegate {
     MAX_OUTCOME
   };
 
-  PasswordProtectionRequest(const GURL& main_frame_url,
-                            LoginReputationClientRequest::TriggerType type,
-                            bool is_extended_reporting,
-                            bool is_incognito,
-                            base::WeakPtr<PasswordProtectionService> pps,
-                            int request_timeout_in_ms);
-
-  ~PasswordProtectionRequest() override;
+  PasswordProtectionRequest(
+      const GURL& main_frame_url,
+      LoginReputationClientRequest::TriggerType type,
+      std::unique_ptr<PasswordProtectionFrames> pending_password_frames,
+      base::WeakPtr<PasswordProtectionService> pps,
+      int request_timeout_in_ms);
 
   base::WeakPtr<PasswordProtectionRequest> GetWeakPtr() {
     return weakptr_factory_.GetWeakPtr();
@@ -66,12 +68,19 @@ class PasswordProtectionRequest : public net::URLFetcherDelegate {
 
   GURL main_frame_url() const { return main_frame_url_; }
 
-  bool is_incognito() const { return is_incognito_; }
+ protected:
+  ~PasswordProtectionRequest() override;
 
  private:
+  friend class base::RefCountedThreadSafe<PasswordProtectionRequest>;
+
+  void CheckWhitelistOnUIThread();
+
   // If |main_frame_url_| matches whitelist, call Finish() immediately;
-  // otherwise call CheckCachedVerdicts().
-  void OnWhitelistCheckDone(bool match_whitelist);
+  // otherwise call CheckCachedVerdicts(). Called on UI thread.
+  // |match_whitelist| points to the boolean value set on IO thread by

[lpz, 2017/04/18 14:43:27]

nit: further to Nathan's comment, can we explain more about *why*
match_whitelist is a pointer? As a newbie, and after looking at the actual code
of CheckCsdWhitelistOnIOThread, I can kind of glean that it's something to do
with passing stuff between threads, but mentioning something about how ownership
is done via base::Owned bit might help too?

[Jialiu Lin, 2017/04/18 20:38:04]

Done.

+  // PasswordProtectionService::CheckCsdWhitelistOnIOThread.
+  void OnWhitelistCheckDone(const bool* match_whitelist);
 
   // Looks up cached verdicts. If verdict is already cached, call SendRequest();
   // otherwise call Finish().
@@ -90,19 +99,13 @@ class PasswordProtectionRequest : public net::URLFetcherDelegate {
   void Finish(RequestOutcome outcome,
               std::unique_ptr<LoginReputationClientResponse> response);
 
-  void CheckWhitelistsOnUIThread();
-
   // Main frame URL of the login form.
   GURL main_frame_url_;
 
   // If this request is for unfamiliar login page or for a password reuse event.
   const LoginReputationClientRequest::TriggerType request_type_;
 
-  // If user is opted-in Safe Browsing Extended Reporting.
-  const bool is_extended_reporting_;
-
-  // If current session is in incognito mode.
-  const bool is_incognito_;
+  std::unique_ptr<PasswordProtectionFrames> pending_password_frames_;
 
   // When request is sent.
   base::TimeTicks request_start_time_;
@@ -111,14 +114,22 @@ class PasswordProtectionRequest : public net::URLFetcherDelegate {
   std::unique_ptr<net::URLFetcher> fetcher_;
 
   // The PasswordProtectionService instance owns |this|.
+  // Can only be accessed on UI thread.
   base::WeakPtr<PasswordProtectionService> password_protection_service_;
 
+  // Safe Browsing database manager used to look up CSD whitelist.
+  // Can only be accessed on IO thread.
+  scoped_refptr<SafeBrowsingDatabaseManager> database_manager_;
+
   // If we haven't receive response after this period of time, we cancel this
   // request.
   const int request_timeout_in_ms_;
 
   std::unique_ptr<LoginReputationClientRequest> request_proto_;
 
+  // Needed for canceling tasks posted to different threads.
+  base::CancelableTaskTracker tracker_;
+
   base::WeakPtrFactory<PasswordProtectionRequest> weakptr_factory_;
   DISALLOW_COPY_AND_ASSIGN(PasswordProtectionRequest);
 };