Safe Browsing: Add Omnibox Interaction Incident Reporter

chrome/browser/safe_browsing/safe_browsing_service.h

 // Copyright (c) 2012 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.
 //
 // The Safe Browsing service is responsible for downloading anti-phishing and
 // anti-malware tables and checking urls against them.
 
 #ifndef CHROME_BROWSER_SAFE_BROWSING_SAFE_BROWSING_SERVICE_H_
 #define CHROME_BROWSER_SAFE_BROWSING_SAFE_BROWSING_SERVICE_H_
 
@@ skipping 40 matching lines @@
 class URLRequestContext;
 class URLRequestContextGetter;
 }
 
 namespace safe_browsing {
 class ClientSideDetectionService;
 class DownloadProtectionService;
 
 #if defined(FULL_SAFE_BROWSING)
 class IncidentReportingService;
+class OmniboxWatcher;
 class OffDomainInclusionDetector;
 #endif
 }
 
 // Construction needs to happen on the main thread.
 // The SafeBrowsingService owns both the UI and Database managers which do
 // the heavylifting of safebrowsing service. Both of these managers stay
 // alive until SafeBrowsingService is destroyed, however, they are disabled
 // permanently when Shutdown method is called.
 class SafeBrowsingService
@@ skipping 128 matching lines @@
   void Observe(int type,
                const content::NotificationSource& source,
                const content::NotificationDetails& details) override;
 
   // Starts following the safe browsing preference on |pref_service|.
   void AddPrefService(PrefService* pref_service);
 
   // Stop following the safe browsing preference on |pref_service|.
   void RemovePrefService(PrefService* pref_service);
 
+  // Creates a OmniboxWatcher for the newly-instantiated |profile|.
+  void AddOmniboxWatcher(Profile* profile);
+
+  // Deletes the OmniboxWatcher associated with |profile|.
+  void RemoveOmniboxWatcher(Profile* profile);
+
   // Checks if any profile is currently using the safe browsing service, and
   // starts or stops the service accordingly.
   void RefreshState();
 
   // The factory used to instanciate a SafeBrowsingService object.
   // Useful for tests, so they can provide their own implementation of
   // SafeBrowsingService.
   static SafeBrowsingServiceFactory* factory_;
 
   // The SafeBrowsingURLRequestContextGetter used to access
@@ skipping 13 matching lines @@
   // Whether the service is running. 'enabled_' is used by SafeBrowsingService
   // on the IO thread during normal operations.
   bool enabled_;
 
   // Tracks existing PrefServices, and the safe browsing preference on each.
   // This is used to determine if any profile is currently using the safe
   // browsing service, and to start it up or shut it down accordingly.
   // Accessed on UI thread.
   std::map<PrefService*, PrefChangeRegistrar*> prefs_map_;
 
+  // Tracks existing OmniboxWatchers. This is used to keep one per profile.
+  std::map<Profile*, safe_browsing::OmniboxWatcher*> omnibox_watcher_map_;

[grt (UTC plus 2), 2014/12/05 18:40:56]

how about putting a scoped_ptr<OmniboxWatcher> member on
IncidentReportingService::ProfileContext instead of having a new collection
here? i have some reservations about mixing incident handlers in there, but it
seems like such a no-brainer to have IncidentReportingService::OnProfileAdded
create the watcher and let it die when the ProfileContext is deleted in
IncidentReportingService::OnProfileDestroyed.

[Mark P, 2014/12/09 19:23:30]

Done.

It is okay that one listens for NOTIFICATION_PROFILE_ADDED and one listens for
NOTIFICATION_PROFILE_CREATED?  I don't think these are equivalent.

[Mark P, 2014/12/09 19:24:48]

Done.

I love how I can offhandedly mention any issue that I'm having when I leave work
on day, and, before I spend much time thinking about it, I have the solution in
my inbox in the morning. :-)

It is okay that one listens for NOTIFICATION_PROFILE_ADDED and one listens for
NOTIFICATION_PROFILE_CREATED?  I don't think these are equivalent.

[grt (UTC plus 2), 2014/12/09 20:15:52]

I think it's okay. PROFILE_ADDED happens when the Profile is added to the
ProfileManager. It likely happens a little later than _CREATED, but still before
the omnibox can be interacted with.

noms: Are there any cases where NOTIFICATION_PROFILE_CREATED can be sent for a
profile, but not NOTIFICATION_PROFILE_ADDED? More to the point, is it possible
for a user to interact with the omnibox before PROFILE_ADDED is called?

[noms (inactive), 2014/12/09 20:35:52]

Hmmm, so I think that this can happen for sure in tests (you manually create a
Profile, but you don't add it to the ProfileManager). In a regular Chrome run, I
think it would only happen if something's gone horribly wrong: Chrome crashes
after the Profile was created, the ProfileManager gets itself corrupted...all
situations when all bets are off anyway.

I didn't check who listens to PROFILE_CREATED, but keep in mind at that point
all PKSes and stuff like that aren't initialized for the Profile yet -- those
get done after PROFILE_CREATED and before PROFILE_ADDED. 

[grt (UTC plus 2), 2014/12/10 15:13:16]

Thanks for the info. I think this is okay, then.

+
   // Used to track creation and destruction of profiles on the UI thread.
   content::NotificationRegistrar prefs_registrar_;
 
   // The ClientSideDetectionService is managed by the SafeBrowsingService,
   // since its running state and lifecycle depends on SafeBrowsingService's.
   // Accessed on UI thread.
   scoped_ptr<safe_browsing::ClientSideDetectionService> csd_service_;
 
   // The DownloadProtectionService is managed by the SafeBrowsingService,
   // since its running state and lifecycle depends on SafeBrowsingService's.
@@ skipping 24 matching lines @@
 class SafeBrowsingServiceFactory {
  public:
   SafeBrowsingServiceFactory() { }
   virtual ~SafeBrowsingServiceFactory() { }
   virtual SafeBrowsingService* CreateSafeBrowsingService() = 0;
  private:
   DISALLOW_COPY_AND_ASSIGN(SafeBrowsingServiceFactory);
 };
 
 #endif  // CHROME_BROWSER_SAFE_BROWSING_SAFE_BROWSING_SERVICE_H_