Enable site engagement in incognito.

Enable site engagement in incognito.

This CL makes the SiteEngagementService available in incognito. When in
incognito mode, the service will initialize values from the original
profile and then subsequently maintain its own independent copy.

Incognito profiles will not record periodic metrics.

BUG=640863
TBR=dbeam@chromium.org,rdevlin.cronin@chromium.org,dtrainor@chromium.org

Committed: https://crrev.com/0c83f6df5af844d8934d62231bbfc4b94c4b0898
Cr-Commit-Position: refs/heads/master@{#415177}

[calamity, 2016-08-25 05:21:59]

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

[calamity, 2016-08-25 05:22:02]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-08-25 05:22:34]

Dry run: 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, 2016-08-25 06:32:25]

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

[commit-bot: I haz the power, 2016-08-25 06:32:26]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[calamity, 2016-08-26 03:10:03]

calamity@chromium.org changed reviewers:
+ dominickn@chromium.org

[dominickn, 2016-08-26 05:42:03]

Extremely elegant, nice!

https://codereview.chromium.org/2274973003/diff/20001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_score.h (right):

https://codereview.chromium.org/2274973003/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_score.h:119: HostContentSettingsMap*
settings_map,
This may be cleaner if you just pass the profile pointer and do the
HostContentSettingsMap fetch, incognito check and fallback inside site
engagement score to set the initial score. Thoughts?

[dominickn, 2016-08-26 05:48:40]

Also: you should update chrome/browser/android/metrics/launch_metrics.cc to not
have the if (service) check as well.

[calamity, 2016-08-26 06:51:10]

https://codereview.chromium.org/2274973003/diff/20001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_score.h (right):

https://codereview.chromium.org/2274973003/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_score.h:119: HostContentSettingsMap*
settings_map,
On 2016/08/26 05:42:03, dominickn wrote:
> This may be cleaner if you just pass the profile pointer and do the
> HostContentSettingsMap fetch, incognito check and fallback inside site
> engagement score to set the initial score. Thoughts?

Done.

[calamity, 2016-08-26 06:51:27]

Description was changed from

==========
Enable site engagement in incognito.

This CL makes the SiteEngagementService available in incognito. When in
incognito mode, the service will initialize values from the original
profile and then subsequently maintain its own independent copy.

BUG=640863
==========

to

==========
Enable site engagement in incognito.

This CL makes the SiteEngagementService available in incognito. When in
incognito mode, the service will initialize values from the original
profile and then subsequently maintain its own independent copy.

Incognito profiles will not record periodic metrics.

BUG=640863
==========

[dominickn, 2016-08-26 07:06:28]

lgtm % nit

Nice catch on the metrics. Application launch will never be from an incognito
profile so they should be okay.

https://codereview.chromium.org/2274973003/diff/40001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_score.cc (right):

https://codereview.chromium.org/2274973003/diff/40001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_score.cc:198: :
SiteEngagementScore(clock, GetScoreDictForOrigin(profile, origin)) {
Nit: origin_(origin), profile_(profile) {}

[dominickn, 2016-08-27 04:24:07]

https://codereview.chromium.org/2274973003/diff/40001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_service_factory.h (right):

https://codereview.chromium.org/2274973003/diff/40001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service_factory.h:20: // * there
should be no site engagement tracking in incognito
Additional nit: fix this comment.

[calamity, 2016-08-29 06:43:48]

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

[commit-bot: I haz the power, 2016-08-29 06:44:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[calamity, 2016-08-29 06:45:38]

Description was changed from

==========
Enable site engagement in incognito.

This CL makes the SiteEngagementService available in incognito. When in
incognito mode, the service will initialize values from the original
profile and then subsequently maintain its own independent copy.

Incognito profiles will not record periodic metrics.

BUG=640863
==========

to

==========
Enable site engagement in incognito.

This CL makes the SiteEngagementService available in incognito. When in
incognito mode, the service will initialize values from the original
profile and then subsequently maintain its own independent copy.

Incognito profiles will not record periodic metrics.

BUG=640863
TBR=dbeam@chromium.org,rdevlin.cronin@chromium.org,dtrainor@chromium.org
==========

[calamity, 2016-08-29 06:45:39]

calamity@chromium.org changed reviewers:
+ dbeam@chromium.org, dtrainor@chromium.org, rdevlin.cronin@chromium.org

[calamity, 2016-08-29 06:46:56]

TBRs

dbeam@: site_engagement_ui.cc
dtrainor@: launch_metrics.cc
rdevlincronin@: application_launch.cc

PTAL, thanks!

https://codereview.chromium.org/2274973003/diff/40001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_score.cc (right):

https://codereview.chromium.org/2274973003/diff/40001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_score.cc:198: :
SiteEngagementScore(clock, GetScoreDictForOrigin(profile, origin)) {
On 2016/08/26 07:06:28, dominickn wrote:
> Nit: origin_(origin), profile_(profile) {}

You can't actually do this apparently >_> I tried this as well when I saw this.
Delegated constructors are _only_ allowed to call another constructor in their
initialiser list.

https://codereview.chromium.org/2274973003/diff/40001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_service_factory.h (right):

https://codereview.chromium.org/2274973003/diff/40001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service_factory.h:20: // * there
should be no site engagement tracking in incognito
On 2016/08/27 04:24:07, dominickn wrote:
> Additional nit: fix this comment.

Done.

[dominickn, 2016-08-29 06:59:55]

still lgtm

https://codereview.chromium.org/2274973003/diff/40001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_score.cc (right):

https://codereview.chromium.org/2274973003/diff/40001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_score.cc:198: :
SiteEngagementScore(clock, GetScoreDictForOrigin(profile, origin)) {
On 2016/08/29 06:46:56, calamity wrote:
> On 2016/08/26 07:06:28, dominickn wrote:
> > Nit: origin_(origin), profile_(profile) {}
> 
> You can't actually do this apparently >_> I tried this as well when I saw
this.
> Delegated constructors are _only_ allowed to call another constructor in their
> initialiser list.

Acknowledged.

[commit-bot: I haz the power, 2016-08-29 08:17:23]

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

[commit-bot: I haz the power, 2016-08-29 08:17:24]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-08-29 14:28:22]

chrome/browser/ui/extensions/application_launch.cc lgtm

[David Trainor- moved to gerrit, 2016-08-29 15:47:52]

launch_metrics.cc lgtm!

[Dan Beam, 2016-08-30 00:16:59]

lgtm but shouldn't there be more

  if (incognito) return;

in non-const SiteEngagementService methods?

[calamity, 2016-08-30 01:38:08]

The CQ bit was checked by calamity@chromium.org

[calamity, 2016-08-30 01:41:45]

On 2016/08/30 00:16:59, Dan Beam wrote:
> lgtm but shouldn't there be more
> 
>   if (incognito) return;
> 
> in non-const SiteEngagementService methods?

A lot of the non-const stuff should actually Just Work since everything will end
up writing to the incognito profile's prefs. I'm a bit more concerned about
other things that may need to be turned off for incognito but I'll do a
follow-up as I figure out what exactly won't work here.

[commit-bot: I haz the power, 2016-08-30 02:45:05]

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, 2016-08-30 05:46:42]

Description was changed from

==========
Enable site engagement in incognito.

This CL makes the SiteEngagementService available in incognito. When in
incognito mode, the service will initialize values from the original
profile and then subsequently maintain its own independent copy.

Incognito profiles will not record periodic metrics.

BUG=640863
TBR=dbeam@chromium.org,rdevlin.cronin@chromium.org,dtrainor@chromium.org
==========

to

==========
Enable site engagement in incognito.

This CL makes the SiteEngagementService available in incognito. When in
incognito mode, the service will initialize values from the original
profile and then subsequently maintain its own independent copy.

Incognito profiles will not record periodic metrics.

BUG=640863
TBR=dbeam@chromium.org,rdevlin.cronin@chromium.org,dtrainor@chromium.org
==========

[commit-bot: I haz the power, 2016-08-30 05:46:43]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-08-30 05:49:49]

Description was changed from

==========
Enable site engagement in incognito.

This CL makes the SiteEngagementService available in incognito. When in
incognito mode, the service will initialize values from the original
profile and then subsequently maintain its own independent copy.

Incognito profiles will not record periodic metrics.

BUG=640863
TBR=dbeam@chromium.org,rdevlin.cronin@chromium.org,dtrainor@chromium.org
==========

to

==========
Enable site engagement in incognito.

This CL makes the SiteEngagementService available in incognito. When in
incognito mode, the service will initialize values from the original
profile and then subsequently maintain its own independent copy.

Incognito profiles will not record periodic metrics.

BUG=640863
TBR=dbeam@chromium.org,rdevlin.cronin@chromium.org,dtrainor@chromium.org

Committed: https://crrev.com/0c83f6df5af844d8934d62231bbfc4b94c4b0898
Cr-Commit-Position: refs/heads/master@{#415177}
==========

[commit-bot: I haz the power, 2016-08-30 05:49:51]

Patchset 4 (id:??) landed as
https://crrev.com/0c83f6df5af844d8934d62231bbfc4b94c4b0898
Cr-Commit-Position: refs/heads/master@{#415177}