Improve PasswordProtectionService and PasswordProtectionRequest

components/safe_browsing/password_protection/password_protection_service.cc

 // Copyright 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/safe_browsing/password_protection/password_protection_service.h"
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
@@ skipping 45 matching lines @@
 // e.g., "https://www.foo.com:80/bar/test.cgi" -> "http://www.foo.com".
 GURL GetHostNameWithHTTPScheme(const GURL& url) {
   DCHECK(url.SchemeIsHTTPOrHTTPS());
   std::string result(url::kHttpScheme);
   result.append(url::kStandardSchemeSeparator).append(url.HostNoBrackets());
   return GURL(result);
 }
 
 }  // 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,
     HistoryService* history_service,
     HostContentSettingsMap* host_content_settings_map)
     : stored_verdict_count_(-1),
       database_manager_(database_manager),
       request_context_getter_(request_context_getter),
       history_service_observer_(this),
       content_settings_(host_content_settings_map),
       weak_factory_(this) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (history_service)
     history_service_observer_.Add(history_service);
 }
 
 PasswordProtectionService::~PasswordProtectionService() {
+  tracker_.TryCancelAll();
   CancelPendingRequests();
   history_service_observer_.RemoveAll();
   weak_factory_.InvalidateWeakPtrs();
 }
 
 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
 PasswordProtectionService::GetCachedVerdict(
     const GURL& url,
     LoginReputationClientResponse* out_response) {
   if (!url.is_valid())
     return LoginReputationClientResponse::VERDICT_TYPE_UNSPECIFIED;
 
   DCHECK(content_settings_);
@@ skipping 72 matching lines @@
   // override it.
   verdict_dictionary->SetWithoutPathExpansion(verdict->cache_expression(),
                                               std::move(verdict_entry));
   content_settings_->SetWebsiteSettingDefaultScope(
       hostname, GURL(), CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION,
       std::string(), std::move(verdict_dictionary));
 }
 
 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) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   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.
   for (auto it = requests_.begin(); it != requests_.end(); it++) {
     if (it->get() == request) {
       requests_.erase(it);
       break;
     }
   }
 }
 
 void PasswordProtectionService::CancelPendingRequests() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   for (auto it = requests_.begin(); it != requests_.end();) {
     // We need to advance the iterator before we cancel because canceling
     // the request will invalidate it when RequestFinished is called.
     PasswordProtectionRequest* request = it->get();
     it++;
     request->Cancel(false);
   }
   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();
   DCHECK(!api_key.empty());
   return url.Resolve("?key=" + net::EscapeQueryParamValue(api_key, true));
 }
 
 int PasswordProtectionService::GetStoredVerdictCount() {
   DCHECK(content_settings_);
   // If we have already computed this, return its value.
@@ skipping 18 matching lines @@
       stored_verdict_count_ += static_cast<int>(verdict_dictionary->size());
   }
   return stored_verdict_count_;
 }
 
 int PasswordProtectionService::GetRequestTimeoutInMS() {
   return kRequestTimeoutMs;
 }
 
 void PasswordProtectionService::OnMatchCsdWhiteListResult(
-    bool match_whitelist) {
+    const bool* match_whitelist) {
   UMA_HISTOGRAM_BOOLEAN(
       "PasswordManager.PasswordReuse.MainFrameMatchCsdWhitelist",
-      match_whitelist);
+      *match_whitelist);
 }
 
 void PasswordProtectionService::OnURLsDeleted(
     history::HistoryService* history_service,
     bool all_history,
     bool expired,
     const history::URLRows& deleted_rows,
     const std::set<GURL>& favicon_urls) {
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
@@ skipping 92 matching lines @@
   V4ProtocolManagerUtil::GeneratePathVariantsToCheck(canonical_path,
                                                      std::string(), paths);
 }
 
 // Return the path of the cache expression. e.g.:
 // "www.google.com"     -> "/"
 // "www.google.com/abc" -> "/abc/"
 // "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] != '/')
     out_put.append("/");
 
   size_t first_slash_pos = out_put.find_first_of("/");
   DCHECK_NE(std::string::npos, first_slash_pos);
   return out_put.substr(first_slash_pos);
 }
 
@@ skipping 12 matching lines @@
   const std::vector<char> verdict_blob(serialized_proto.begin(),
                                        serialized_proto.end());
   std::unique_ptr<base::Value> binary_value =
       base::MakeUnique<base::Value>(verdict_blob);
   DCHECK_EQ(base::Value::Type::BINARY, binary_value->type());
   result->Set(kVerdictProto, std::move(binary_value));
   return result;
 }
 
 }  // namespace safe_browsing