Very basic change to add a TriggerManager class owned by SafeBrowsingService.

Very basic change to add a TriggerManager class owned by SafeBrowsingService.
TriggerManager is just a skeleton here, to be expanded in future CLs.

Review-Url: https://codereview.chromium.org/2855643002
Cr-Commit-Position: refs/heads/master@{#469413}
Committed: https://chromium.googlesource.com/chromium/src/+/475352fde5d62050fa44d51e4cb2c740c9b6bc69

[lpz, 2017-05-01 16:34:27]

The CQ bit was checked by lpz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 16:34:55]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lpz, 2017-05-01 17:26:14]

Description was changed from

==========
Very basic change to add a TriggerManager class owned by SafeBrowsingService.
TriggerManager is just a skeleton here, to be expanded in future CLs.
==========

to

==========
Very basic change to add a TriggerManager class owned by SafeBrowsingService.
TriggerManager is just a skeleton here, to be expanded in future CLs.
==========

[lpz, 2017-05-01 17:26:14]

lpz@chromium.org changed reviewers:
+ jialiul@chromium.org

[commit-bot: I haz the power, 2017-05-01 18:11:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-01 18:11:46]

Dry run: This issue passed the CQ dry run.

[Jialiu Lin, 2017-05-04 15:53:38]

really sorry. I forgot to click "publish".

https://codereview.chromium.org/2855643002/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/2855643002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service.cc:785: void
SafeBrowsingService::CreateTriggerManager() {
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.

https://codereview.chromium.org/2855643002/diff/1/components/safe_browsing/tr...
File components/safe_browsing/triggers/trigger_manager.cc (right):

https://codereview.chromium.org/2855643002/diff/1/components/safe_browsing/tr...
components/safe_browsing/triggers/trigger_manager.cc:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
2012->2017

[lpz, 2017-05-04 17:49:18]

The CQ bit was checked by lpz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 17:49:55]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lpz, 2017-05-04 17:51:53]

https://codereview.chromium.org/2855643002/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/2855643002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service.cc:785: void
SafeBrowsingService::CreateTriggerManager() {
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?

https://codereview.chromium.org/2855643002/diff/1/components/safe_browsing/tr...
File components/safe_browsing/triggers/trigger_manager.cc (right):

https://codereview.chromium.org/2855643002/diff/1/components/safe_browsing/tr...
components/safe_browsing/triggers/trigger_manager.cc:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2017/05/04 15:53:38, Jialiu Lin wrote:
> 2012->2017

Done.

[Jialiu Lin, 2017-05-04 18:29:47]

https://codereview.chromium.org/2855643002/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/2855643002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service.cc:785: void
SafeBrowsingService::CreateTriggerManager() {
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?

[lpz, 2017-05-04 18:49:47]

Thanks Jialiu!

https://codereview.chromium.org/2855643002/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/2855643002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service.cc:785: void
SafeBrowsingService::CreateTriggerManager() {
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.

[Jialiu Lin, 2017-05-04 18:50:44]

lgtm

[commit-bot: I haz the power, 2017-05-04 18:58:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-04 18:58:36]

Dry run: This issue passed the CQ dry run.

[lpz, 2017-05-04 18:59:32]

The CQ bit was checked by lpz@chromium.org

[commit-bot: I haz the power, 2017-05-04 19:00:16]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-04 19:06:59]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493924372952610,
"parent_rev": "15c6e6959e859602b939bec765b9caa277fde43b", "commit_rev":
"475352fde5d62050fa44d51e4cb2c740c9b6bc69"}

[commit-bot: I haz the power, 2017-05-04 19:07:07]

Description was changed from

==========
Very basic change to add a TriggerManager class owned by SafeBrowsingService.
TriggerManager is just a skeleton here, to be expanded in future CLs.
==========

to

==========
Very basic change to add a TriggerManager class owned by SafeBrowsingService.
TriggerManager is just a skeleton here, to be expanded in future CLs.

Review-Url: https://codereview.chromium.org/2855643002
Cr-Commit-Position: refs/heads/master@{#469413}
Committed:
https://chromium.googlesource.com/chromium/src/+/475352fde5d62050fa44d51e4cb2...
==========

[commit-bot: I haz the power, 2017-05-04 19:07:08]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/475352fde5d62050fa44d51e4cb2...