chrome/browser/safe_browsing/safe_browsing_service.cc - Issue 2855643002: Very basic change to add a TriggerManager class owned by SafeBrowsingService.

Unified Diff: chrome/browser/safe_browsing/safe_browsing_service.cc

Issue 2855643002: Very basic change to add a TriggerManager class owned by SafeBrowsingService. (Closed)

Patch Set: Created 3 years, 8 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

« no previous file with comments | « chrome/browser/safe_browsing/safe_browsing_service.h ('k') | components/safe_browsing/triggers/BUILD.gn » ('j') | components/safe_browsing/triggers/trigger_manager.cc » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: chrome/browser/safe_browsing/safe_browsing_service.cc

diff --git a/chrome/browser/safe_browsing/safe_browsing_service.cc b/chrome/browser/safe_browsing/safe_browsing_service.cc

index 51461b696c212a923c259fa24c4264ee354e793d..80da8b71940daac8af524fe1325e60b7a36f3879 100644

--- a/chrome/browser/safe_browsing/safe_browsing_service.cc

+++ b/chrome/browser/safe_browsing/safe_browsing_service.cc

@@ -39,6 +39,7 @@

#include "components/prefs/pref_service.h"

#include "components/safe_browsing/common/safebrowsing_constants.h"

#include "components/safe_browsing/common/safebrowsing_switches.h"

+#include "components/safe_browsing/triggers/trigger_manager.h"

#include "components/safe_browsing_db/database_manager.h"

#include "components/safe_browsing_db/v4_feature_list.h"

#include "components/safe_browsing_db/v4_get_hash_protocol_manager.h"

@@ -332,6 +333,8 @@ void SafeBrowsingService::Initialize() {

services_delegate_->Initialize(v4_enabled_);

services_delegate_->InitializeCsdService(url_request_context_getter_.get());

+ CreateTriggerManager();

// Track profile creation and destruction.

profiles_registrar_.Add(this, chrome::NOTIFICATION_PROFILE_CREATED,

content::NotificationService::AllSources());

@@ -779,4 +782,8 @@ void SafeBrowsingService::RemovePasswordProtectionService(Profile* profile) {

password_protection_service_map_.erase(it);

}

+void SafeBrowsingService::CreateTriggerManager() {

Jialiu Lin 2017/05/04 15:53:38 How does TriggerManager tie to profile? If Trigg

lpz 2017/05/04 17:51:53 Good question. I've been thinking of TriggerManage

Jialiu Lin 2017/05/04 18:29:47 (1) If you're only interested in creation and dest

(1) If you're only interested in creation and destruction of WebContents, it is possible to use 1 TriggerManager to do that. But if you want to implement WebContentsObserver for other webcontents behavior (e.g. navigation), then 1 WebContentsObserver can only observe 1 WebContents. So eventually the mapping will be 1 TriggerManager <--> n WebContentsObserver <---> n WebContents (2) Yes, you can get PrefService and incognito state from WebContents, but calling code went through Profile. Typically, through Profile::FromBrowserContext(web_contents->GetBrowserContext()). And then you can do profile->GetPrefs() or profile->IsOffTheRecord(). Unfortunately, TriggerManager is in components/safe_browsing, which cannot directly call Profile class. So the pref related decision need to be made somewhere in chrome/browser/safe_browsing/ and passed into TriggerManager somehow. And if you're using a single TriggerManger to work with multiple profile, you might need to keep track of these pref related decisions for each profile inside TriggerManager. Maybe initially, you can put TriggerManager inside chrome/browser/safe_browsing/ instead of putting it in the components?

lpz 2017/05/04 18:49:47 As discussed offline, will keep it under component

On 2017/05/04 18:29:47, Jialiu Lin wrote: > On 2017/05/04 at 17:51:53, lpz wrote: > > On 2017/05/04 15:53:38, Jialiu Lin wrote: > > > How does TriggerManager tie to profile? If TriggerManager reacts > differently > > > based on Prefs/Profile, do you want to consider making it a per-profile > class. > > > Since components/safe_browsing/ has no access to Profile class. > > > > Good question. I've been thinking of TriggerManager operating on WebContents > objects for now (eg listening for WebContents being created/destroyed, keeping a > list of active WebContents). So we have 1 TriggerManager mapping to many > WebContents. We can get the profile for each WebContents, right? So we could > make a decision about what triggers to run for each WebContents independently? > > > > I suppose we only really need the prefservice (which is a component) and the > incognito state (from the browsercontext) for each webcontents, so we may not > need the profile itself. > > > > WDYT? > > (1) If you're only interested in creation and destruction of WebContents, it is > possible to use 1 TriggerManager to do that. But if you want to implement > WebContentsObserver for other webcontents behavior (e.g. navigation), then 1 > WebContentsObserver can only observe 1 WebContents. So eventually the mapping > will be > 1 TriggerManager <--> n WebContentsObserver <---> n WebContents > > (2) Yes, you can get PrefService and incognito state from WebContents, but > calling code went through Profile. Typically, through > Profile::FromBrowserContext(web_contents->GetBrowserContext()). And then you can > do profile->GetPrefs() or profile->IsOffTheRecord(). > Unfortunately, TriggerManager is in components/safe_browsing, which cannot > directly call Profile class. So the pref related decision need to be made > somewhere in chrome/browser/safe_browsing/ and passed into TriggerManager > somehow. > And if you're using a single TriggerManger to work with multiple profile, you > might need to keep track of these pref related decisions for each profile inside > TriggerManager. > > Maybe initially, you can put TriggerManager inside chrome/browser/safe_browsing/ > instead of putting it in the components?

As discussed offline, will keep it under components for now. Code that calls into the TriggerManager (ie: tab_helpers) can do the job of accessing the prefs/incoginto bits from the profile and pass them in a params. If we get stuck needing other browser-level stuff later, there are ways to work around that at that point.

+ DCHECK_CURRENTLY_ON(BrowserThread::UI);

+ trigger_manager_ = base::MakeUnique<TriggerManager>();

} // namespace safe_browsing