Improve PasswordProtectionService and PasswordProtectionRequest

components/safe_browsing/password_protection/password_protection_request.cc

Index: components/safe_browsing/password_protection/password_protection_request.cc
diff --git a/components/safe_browsing/password_protection/password_protection_request.cc b/components/safe_browsing/password_protection/password_protection_request.cc
index e7db6e05543e8d8f801d6e1dd65cec38f4d59a11..fb002da658f36b1c9b585a61e975d1f52b38e8af 100644
--- a/components/safe_browsing/password_protection/password_protection_request.cc
+++ b/components/safe_browsing/password_protection/password_protection_request.cc
@@ -7,7 +7,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/metrics/histogram_macros.h"
 #include "components/data_use_measurement/core/data_use_user_data.h"
-#include "content/public/browser/browser_thread.h"
+#include "components/safe_browsing_db/database_manager.h"
 #include "net/base/escape.h"
 #include "net/base/load_flags.h"
 #include "net/base/url_util.h"
@@ -20,15 +20,14 @@ namespace safe_browsing {
 PasswordProtectionRequest::PasswordProtectionRequest(
     const GURL& main_frame_url,
     LoginReputationClientRequest::TriggerType type,
-    bool is_extended_reporting,
-    bool is_incognito,
+    std::unique_ptr<PasswordProtectionFrameList> pending_password_frames,
     base::WeakPtr<PasswordProtectionService> pps,
     int request_timeout_in_ms)
     : main_frame_url_(main_frame_url),
       request_type_(type),
-      is_extended_reporting_(is_extended_reporting),
-      is_incognito_(is_incognito),
+      pending_password_frames_(std::move(pending_password_frames)),
       password_protection_service_(pps),
+      database_manager_(password_protection_service_->database_manager()),
       request_timeout_in_ms_(request_timeout_in_ms),
       weakptr_factory_(this) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
@@ -42,33 +41,34 @@ void PasswordProtectionRequest::Start() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   // Initially we only send ping for Safe Browsing Extended Reporting users when
   // they are not in incognito mode. We may loose these conditions later.
-  if (is_incognito_) {
+  if (password_protection_service_->IsIncognito()) {
     Finish(RequestOutcome::INCOGNITO, nullptr);
     return;
   }
-  if (!is_extended_reporting_) {
+  if (!password_protection_service_->IsExtendedReporting()) {
     Finish(RequestOutcome::NO_EXTENDED_REPORTING, nullptr);
     return;
   }
 
-  CheckWhitelistsOnUIThread();
+  CheckWhitelistOnUIThread();
 }
 
-void PasswordProtectionRequest::CheckWhitelistsOnUIThread() {
+void PasswordProtectionRequest::CheckWhitelistOnUIThread() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  DCHECK(password_protection_service_);
-
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
+  bool* match_whitelist = new bool(false);
+  tracker_.PostTaskAndReply(
+      BrowserThread::GetTaskRunnerForThread(BrowserThread::IO).get(), FROM_HERE,
       base::Bind(&PasswordProtectionService::CheckCsdWhitelistOnIOThread,
-                 password_protection_service_, main_frame_url_,
-                 base::Bind(&PasswordProtectionRequest::OnWhitelistCheckDone,
-                            GetWeakPtr())));
+                 base::Unretained(password_protection_service_.get()),
+                 main_frame_url_, match_whitelist),
+      base::Bind(&PasswordProtectionRequest::OnWhitelistCheckDone, this,
+                 base::Owned(match_whitelist)));
 }
 
-void PasswordProtectionRequest::OnWhitelistCheckDone(bool match_whitelist) {
+void PasswordProtectionRequest::OnWhitelistCheckDone(
+    const bool* match_whitelist) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  if (match_whitelist)
+  if (*match_whitelist)
     Finish(RequestOutcome::MATCHED_WHITELIST, nullptr);
   else
     CheckCachedVerdicts();
@@ -100,9 +100,14 @@ void PasswordProtectionRequest::FillRequestProto() {
   LoginReputationClientRequest::Frame* main_frame =
       request_proto_->add_frames();
   main_frame->set_url(main_frame_url_.spec());
+  main_frame->set_frame_index(0 /* main frame */);
+  main_frame->set_has_password_field(true);
   password_protection_service_->FillReferrerChain(
       main_frame_url_, -1 /* tab id not available */, main_frame);
-  // TODO(jialiul): Add sub-frame information and password form information.
+
+  // TODO(jialiul): Fill more password form related info based on
+  // |password_frame_map_| when Safe Browsing backend is ready to handle these
+  // pieces of information.
 }
 
 void PasswordProtectionRequest::SendRequest() {
@@ -169,17 +174,17 @@ void PasswordProtectionRequest::OnURLFetchComplete(
   fetcher_.reset();  // We don't need it anymore.
   UMA_HISTOGRAM_TIMES("PasswordProtection.RequestNetworkDuration",
                       base::TimeTicks::Now() - request_start_time_);
-  if (response->ParseFromString(response_body)) {
+  if (response->ParseFromString(response_body))
     Finish(RequestOutcome::SUCCEEDED, std::move(response));
-  } else {
+  else
     Finish(RequestOutcome::RESPONSE_MALFORMED, nullptr);
-  }
 }
 
 void PasswordProtectionRequest::Finish(
     RequestOutcome outcome,
     std::unique_ptr<LoginReputationClientResponse> response) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  tracker_.TryCancelAll();

[Nathan Parker, 2017/04/18 23:07:26]

Will this automatically get called in the destructor, in ->RequestFinished
below?

[Jialiu Lin, 2017/04/19 00:21:14]

Yes. 
The destruction of PasswordProtectionRequest can be triggered in 2 cases only:
(1) request finished, or canceled. --> Finish(..) is explicitly called.
(2) password_protection_service_ is deleted. Then the calling sequence is:
   PasswordProtectionService::~PasswordProtectionService()
     PasswordProtectionService::CancelPendingRequests()
       PasswordProtectionRequest::Cancel()
         PasswordProtectionRequset::Finish()
           PasswordProtectionService::RequestFinished()
             PasswordProtectionRequest::~PasswordProtectionRequest()
 

 
   UMA_HISTOGRAM_ENUMERATION("PasswordProtection.RequestOutcome", outcome,
                             RequestOutcome::MAX_OUTCOME);