Improve PasswordProtectionService and PasswordProtectionRequest

components/safe_browsing/password_protection/password_protection_service.h

 // 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.
 
 #ifndef COMPONENTS_SAFE_BROWSING_PASSWORD_PROTECTION_PASSWORD_PROTECTION_SERVICE_H_
 #define COMPONENTS_SAFE_BROWSING_PASSWORD_PROTECTION_PASSWORD_PROTECTION_SERVICE_H_
 
-#include <unordered_set>
+#include <set>
 
 #include "base/callback.h"
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "base/scoped_observer.h"
+#include "base/task/cancelable_task_tracker.h"
 #include "base/values.h"
 #include "components/history/core/browser/history_service_observer.h"
 #include "components/safe_browsing/csd.pb.h"
 #include "net/url_request/url_request_context_getter.h"
+#include "third_party/protobuf/src/google/protobuf/repeated_field.h"
 
 namespace history {
 class HistoryService;
 }
 
 class GURL;
 class HostContentSettingsMap;
 
 namespace safe_browsing {
 
 class SafeBrowsingDatabaseManager;
 class PasswordProtectionRequest;
+struct PasswordProtectionFrame;

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

nit: You can remove this like if you move the "using PasswordProtectionFrames"
below the struct definition.

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

Done.

+
+using PasswordFormList = google::protobuf::RepeatedPtrField<
+    LoginReputationClientRequest::Frame::Form>;
+
+using PasswordProtectionFrames =
+    std::vector<std::unique_ptr<PasswordProtectionFrame>>;
+
+// The PasswordProtectionFrame struct encapsulates information about a render
+// frame that has password form(s).
+struct PasswordProtectionFrame {
+  int render_frame_routing_id;
+  int parent_frame_routing_id;
+  GURL last_committed_url;
+  std::unique_ptr<PasswordFormList> password_forms;
+
+  PasswordProtectionFrame();
+
+  ~PasswordProtectionFrame();
+};
 
 // Manage password protection pings and verdicts. There is one instance of this
 // class per profile. Therefore, every PasswordProtectionService instance is
 // associated with a unique HistoryService instance and a unique
 // HostContentSettingsMap instance.
-class PasswordProtectionService : history::HistoryServiceObserver {
+class PasswordProtectionService : public history::HistoryServiceObserver {
  public:
-  using CheckCsdWhitelistCallback = base::Callback<void(bool)>;
-
   PasswordProtectionService(
       const scoped_refptr<SafeBrowsingDatabaseManager>& database_manager,
       scoped_refptr<net::URLRequestContextGetter> request_context_getter,
       history::HistoryService* history_service,
       HostContentSettingsMap* host_content_settings_map);
 
   ~PasswordProtectionService() override;
 
   base::WeakPtr<PasswordProtectionService> GetWeakPtr() {
     return weak_factory_.GetWeakPtr();
@@ skipping 12 matching lines @@
 
   // Stores |verdict| in |settings| based on |url|, |verdict| and
   // |receive_time|.
   void CacheVerdict(const GURL& url,
                     LoginReputationClientResponse* verdict,
                     const base::Time& receive_time);
 
   // Creates an instance of PasswordProtectionRequest and call Start() on that
   // instance. This function also insert this request object in |requests_| for
   // record keeping.
-  void StartRequest(const GURL& main_frame_url,
-                    LoginReputationClientRequest::TriggerType type);
+  void StartRequest(
+      const GURL& main_frame_url,
+      LoginReputationClientRequest::TriggerType type,
+      std::unique_ptr<PasswordProtectionFrames> pending_password_frames);
+
+  void MaybeStartLowReputationRequest(
+      const GURL& main_frame_url,
+      std::unique_ptr<PasswordProtectionFrames> pending_password_frames);
+
+  scoped_refptr<SafeBrowsingDatabaseManager> database_manager();

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

nit: const;

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

Done.

 
  protected:
   friend class PasswordProtectionRequest;
 
   // Called by a PasswordProtectionRequest instance when it finishes to remove
   // itself from |requests_|.
   virtual void RequestFinished(
       PasswordProtectionRequest* request,
       std::unique_ptr<LoginReputationClientResponse> response);
 
@@ skipping 21 matching lines @@
       const GURL& event_url,
       int event_tab_id,  // -1 if tab id is not available.
       LoginReputationClientRequest::Frame* frame) = 0;
 
   virtual bool IsExtendedReporting() = 0;
   virtual bool IsIncognito() = 0;
 
   // If we can send ping to Safe Browsing backend.
   virtual bool IsPingingEnabled() = 0;
 
-  void CheckCsdWhitelistOnIOThread(const GURL& url,
-                                   const CheckCsdWhitelistCallback& callback);
+  void CheckCsdWhitelistOnIOThread(const GURL& url, bool* check_result);
 
   // Increases "PasswordManager.PasswordReuse.MainFrameMatchCsdWhitelist" UMA
   // metric based on input.
-  void OnMatchCsdWhiteListResult(bool match_whitelist);
+  void OnMatchCsdWhiteListResult(const bool* match_whitelist);
 
  private:
   friend class PasswordProtectionServiceTest;
+  friend class TestPasswordProtectionService;
   FRIEND_TEST_ALL_PREFIXES(PasswordProtectionServiceTest,
                            TestParseInvalidVerdictEntry);
   FRIEND_TEST_ALL_PREFIXES(PasswordProtectionServiceTest,
                            TestParseValidVerdictEntry);
   FRIEND_TEST_ALL_PREFIXES(PasswordProtectionServiceTest,
                            TestPathVariantsMatchCacheExpression);
   FRIEND_TEST_ALL_PREFIXES(PasswordProtectionServiceTest,
                            TestCleanUpCachedVerdicts);
 
   // Overridden from history::HistoryServiceObserver.
@@ skipping 35 matching lines @@
   int stored_verdict_count_;
 
   scoped_refptr<SafeBrowsingDatabaseManager> database_manager_;
 
   // The context we use to issue network requests. This request_context_getter
   // is obtained from SafeBrowsingService so that we can use the Safe Browsing
   // cookie store.
   scoped_refptr<net::URLRequestContextGetter> request_context_getter_;
 
   // Set of pending PasswordProtectionRequests.
-  std::unordered_set<std::unique_ptr<PasswordProtectionRequest>> requests_;
+  std::set<scoped_refptr<PasswordProtectionRequest>> requests_;
 
   ScopedObserver<history::HistoryService, history::HistoryServiceObserver>
       history_service_observer_;
 
   // Content settings map associated with this instance.
   HostContentSettingsMap* content_settings_;
 
+  // Weakptr can only cancel task if it is posted to the same thread. Therefore,
+  // we need CancelableTaskTracker to cancel tasks posted to IO thread.
+  base::CancelableTaskTracker tracker_;
+
   base::WeakPtrFactory<PasswordProtectionService> weak_factory_;
   DISALLOW_COPY_AND_ASSIGN(PasswordProtectionService);
 };
 
 }  // namespace safe_browsing
 
 #endif  // COMPONENTS_SAFE_BROWSING_PASSWORD_PROTECTION_PASSWORD_PROTECTION_SERVICE_H_