Improve PasswordProtectionService and PasswordProtectionRequest

components/safe_browsing/password_protection/password_protection_service.cc

Index: components/safe_browsing/password_protection/password_protection_service.cc
diff --git a/components/safe_browsing/password_protection/password_protection_service.cc b/components/safe_browsing/password_protection/password_protection_service.cc
index 3aee7ac11413ccb50a6e0993c05af43acbcb0445..403378ed3c1e76b06f01c4efc8abd31cdc1111ee 100644
--- a/components/safe_browsing/password_protection/password_protection_service.cc
+++ b/components/safe_browsing/password_protection/password_protection_service.cc
@@ -63,6 +63,12 @@ GURL GetHostNameWithHTTPScheme(const GURL& url) {
 
 }  // namespace
 
+PasswordProtectionFrame::PasswordProtectionFrame() {

[Nathan Parker, 2017/04/17 20:28:02]

I wonder if you can delete this default ctor.
Maybe "PasswordProtectionFrame() = delete;" in the .h file.

[Jialiu Lin, 2017/04/18 00:58:26]

Done.

+  NOTIMPLEMENTED();
+}
+
+PasswordProtectionFrame::~PasswordProtectionFrame() = default;
+
 PasswordProtectionService::PasswordProtectionService(
     const scoped_refptr<SafeBrowsingDatabaseManager>& database_manager,
     scoped_refptr<net::URLRequestContextGetter> request_context_getter,
@@ -80,6 +86,7 @@ PasswordProtectionService::PasswordProtectionService(
 }
 
 PasswordProtectionService::~PasswordProtectionService() {
+  tracker_.TryCancelAll();
   CancelPendingRequests();
   history_service_observer_.RemoveAll();
   weak_factory_.InvalidateWeakPtrs();
@@ -88,27 +95,21 @@ PasswordProtectionService::~PasswordProtectionService() {
 void PasswordProtectionService::RecordPasswordReuse(const GURL& url) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(database_manager_);
-  if (!url.is_valid())
-    return;
-
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      base::Bind(
-          &PasswordProtectionService::CheckCsdWhitelistOnIOThread, GetWeakPtr(),
-          url,
-          base::Bind(&PasswordProtectionService::OnMatchCsdWhiteListResult,
-                     base::Unretained(this))));
+  bool* match_whitelist = new bool(false);

[Nathan Parker, 2017/04/17 20:28:02]

bare pointer alert.  :-)

Could this be a unique_ptr?

[Jialiu Lin, 2017/04/18 00:58:26]

Similar to my reply in PasswordProtectionRequest class, a bare pointer works
better in PostTaskAndReply(). And base::Owned() will take care of the ownership.

+  tracker_.PostTaskAndReply(
+      BrowserThread::GetTaskRunnerForThread(BrowserThread::IO).get(), FROM_HERE,
+      base::Bind(&PasswordProtectionService::CheckCsdWhitelistOnIOThread,
+                 base::Unretained(this), url, match_whitelist),
+      base::Bind(&PasswordProtectionService::OnMatchCsdWhiteListResult,
+                 base::Unretained(this), base::Owned(match_whitelist)));
 }
 
 void PasswordProtectionService::CheckCsdWhitelistOnIOThread(
     const GURL& url,
-    const CheckCsdWhitelistCallback& callback) {
+    bool* check_result) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  DCHECK(database_manager_);
-  bool check_result = database_manager_->MatchCsdWhitelistUrl(url);
-  DCHECK(callback);
-  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-                          base::Bind(callback, check_result));
+  if (url.is_valid())

[Nathan Parker, 2017/04/17 20:28:02]

So no-standard URLs will default to "whitelisted=false," ya?

Actually, maybe we want the default to be "true."  That's what
MatchCsdWhitelistUrl() will default to (for pver4), if the store isn't
available.  BTW this whitelist isn't available on Chromium (non-chrome branded)
builds currently, so it'll default to true.

[Jialiu Lin, 2017/04/18 00:58:27]

Thanks for pointing this out. Changed default to true. 

+    *check_result = database_manager_->MatchCsdWhitelistUrl(url);
 }
 
 LoginReputationClientResponse::VerdictType
@@ -201,19 +202,35 @@ void PasswordProtectionService::CacheVerdict(
 
 void PasswordProtectionService::StartRequest(
     const GURL& main_frame_url,
-    LoginReputationClientRequest::TriggerType type) {
+    LoginReputationClientRequest::TriggerType type,
+    std::unique_ptr<PasswordProtectionFrames> pending_password_frames) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  if (!IsPingingEnabled())
-    return;
-  std::unique_ptr<PasswordProtectionRequest> request =
-      base::MakeUnique<PasswordProtectionRequest>(
-          main_frame_url, type, IsExtendedReporting(), IsIncognito(),
-          GetWeakPtr(), GetRequestTimeoutInMS());
+  scoped_refptr<PasswordProtectionRequest> request(
+      new PasswordProtectionRequest(
+          main_frame_url, type, std::move(pending_password_frames),
+          GetWeakPtr(), database_manager_, GetRequestTimeoutInMS()));
   DCHECK(request);
   request->Start();
   requests_.insert(std::move(request));
 }
 
+void PasswordProtectionService::MaybeStartLowReputationRequest(
+    const GURL& main_frame_url,
+    std::unique_ptr<PasswordProtectionFrames> pending_password_frames) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (!IsPingingEnabled())
+    return;
+
+  // Don't start request if |main_frame_url| is invalid or is NOT http or https.

[Nathan Parker, 2017/04/17 20:28:02]

nit: this comment isn't necessary, since the code says the same thing.

Could say: Skip URLs that we can't get a reliable reputation for

[Jialiu Lin, 2017/04/18 00:58:27]

Done.

+  if (!main_frame_url.is_valid() || !main_frame_url.SchemeIsHTTPOrHTTPS()) {
+    return;
+  }
+
+  StartRequest(main_frame_url,
+               LoginReputationClientRequest::UNFAMILIAR_LOGIN_PAGE,
+               std::move(pending_password_frames));
+}
+
 void PasswordProtectionService::RequestFinished(
     PasswordProtectionRequest* request,
     std::unique_ptr<LoginReputationClientResponse> response) {
@@ -222,7 +239,7 @@ void PasswordProtectionService::RequestFinished(
   DCHECK(request);
   // TODO(jialiul): We don't cache verdict for incognito mode for now.
   // Later we may consider temporarily caching verdict.
-  if (!request->is_incognito() && response)
+  if (response && !IsIncognito())
     CacheVerdict(request->main_frame_url(), response.get(), base::Time::Now());
 
   // Finished processing this request. Remove it from pending list.
@@ -246,6 +263,11 @@ void PasswordProtectionService::CancelPendingRequests() {
   DCHECK(requests_.empty());
 }
 
+scoped_refptr<SafeBrowsingDatabaseManager>
+PasswordProtectionService::database_manager() {
+  return database_manager_;
+}
+
 GURL PasswordProtectionService::GetPasswordProtectionRequestUrl() {
   GURL url(kPasswordProtectionRequestUrl);
   std::string api_key = google_apis::GetAPIKey();
@@ -284,10 +306,10 @@ int PasswordProtectionService::GetRequestTimeoutInMS() {
 }
 
 void PasswordProtectionService::OnMatchCsdWhiteListResult(
-    bool match_whitelist) {
+    const bool* match_whitelist) {
   UMA_HISTOGRAM_BOOLEAN(
       "PasswordManager.PasswordReuse.MainFrameMatchCsdWhitelist",
-      match_whitelist);
+      *match_whitelist);
 }
 
 void PasswordProtectionService::OnURLsDeleted(
@@ -400,7 +422,10 @@ void PasswordProtectionService::GeneratePathVariantsWithoutQuery(
 // "foo.com/foo/bar/"  -> "/foo/bar/"
 std::string PasswordProtectionService::GetCacheExpressionPath(
     const std::string& cache_expression) {
-  DCHECK(!cache_expression.empty());
+  // TODO(jialiul): Change this to a DCHECk when SB server is ready.
+  if (cache_expression.empty())
+    return std::string("/");
+
   std::string out_put(cache_expression);
   // Append a trailing slash if needed.
   if (out_put[out_put.length() - 1] != '/')