Add GetTotalEngagementPoints to SiteEngagementService.

Add GetTotalEngagementPoints to SiteEngagementService.

This CL adds a method to get the sum of all engagement points for all sites.

BUG=464234
Committed: https://crrev.com/f50cd9900ec9d317e05b6aa0a066df53543e3a33
Cr-Commit-Position: refs/heads/master@{#337558}

[calamity, 2015-07-01 03:33:47]

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

[raymes, 2015-07-01 05:33:09]

Cool :)

https://codereview.chromium.org/1207383006/diff/1/chrome/browser/engagement/s...
File chrome/browser/engagement/site_engagement_service.cc (right):

https://codereview.chromium.org/1207383006/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_service.cc:35:
scoped_ptr<base::DictionaryValue> GetOriginDict(
How about we rename this GetScoreDict or GetScoreDictForOrigin? I think it would
make things a bit clearer

https://codereview.chromium.org/1207383006/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_service.cc:201: for (auto it :
engagement_settings) {
perhaps 
for (auto site : engagement_settings)

Also I think you can use "const auto& site" here to just grab a reference.

https://codereview.chromium.org/1207383006/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_service.cc:203:
GetOriginDict(settings_map, GURL(it.primary_pattern.ToString()));
I guess we should technically check that the GURL is valid first (since
ContentSettingsPattern isn't necessarily a url (it may be a pattern). In our
case it /should/ be a URL but there isn't anything that enforces that (and it's
being read from a pref on disk) so we should check.

https://codereview.chromium.org/1207383006/diff/1/chrome/browser/engagement/s...
File chrome/browser/engagement/site_engagement_service_unittest.cc (right):

https://codereview.chromium.org/1207383006/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_service_unittest.cc:340: }
Could we add a new test where we browse to a bunch of sites and check that the
individual scores (service->GetScore) add up to the total score?

[calamity, 2015-07-06 01:51:07]

https://codereview.chromium.org/1207383006/diff/1/chrome/browser/engagement/s...
File chrome/browser/engagement/site_engagement_service.cc (right):

https://codereview.chromium.org/1207383006/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_service.cc:35:
scoped_ptr<base::DictionaryValue> GetOriginDict(
On 2015/07/01 05:33:09, raymes wrote:
> How about we rename this GetScoreDict or GetScoreDictForOrigin? I think it
would
> make things a bit clearer

Done.

https://codereview.chromium.org/1207383006/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_service.cc:201: for (auto it :
engagement_settings) {
On 2015/07/01 05:33:08, raymes wrote:
> perhaps 
> for (auto site : engagement_settings)
> 
> Also I think you can use "const auto& site" here to just grab a reference.

Done.

https://codereview.chromium.org/1207383006/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_service.cc:203:
GetOriginDict(settings_map, GURL(it.primary_pattern.ToString()));
On 2015/07/01 05:33:09, raymes wrote:
> I guess we should technically check that the GURL is valid first (since
> ContentSettingsPattern isn't necessarily a url (it may be a pattern). In our
> case it /should/ be a URL but there isn't anything that enforces that (and
it's
> being read from a pref on disk) so we should check.

Done.

https://codereview.chromium.org/1207383006/diff/1/chrome/browser/engagement/s...
File chrome/browser/engagement/site_engagement_service_unittest.cc (right):

https://codereview.chromium.org/1207383006/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_service_unittest.cc:340: }
On 2015/07/01 05:33:09, raymes wrote:
> Could we add a new test where we browse to a bunch of sites and check that the
> individual scores (service->GetScore) add up to the total score?

Yep, I just needed to check with benwells why this didn't use real numbers and
stuff.

[raymes, 2015-07-06 02:50:51]

lgtm with the following comments

https://codereview.chromium.org/1207383006/diff/20001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_service.cc (right):

https://codereview.chromium.org/1207383006/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service.cc:171:
scoped_ptr<base::DictionaryValue> settings =
nit here and below: score_dict =

https://codereview.chromium.org/1207383006/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service.cc:205:
DCHECK(origin.is_valid());
hmm I think because this is coming from disk we really should check it and skip
the origin if it's invalid?

https://codereview.chromium.org/1207383006/diff/20001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_service_unittest.cc (right):

https://codereview.chromium.org/1207383006/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service_unittest.cc:338: // Expect
that site engagement
nit: this comment looks unfinished

https://codereview.chromium.org/1207383006/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service_unittest.cc:341:
switches::kEnableSiteEngagementService);
nit: we should probably just move this into SetUp()

[calamity, 2015-07-07 02:47:25]

https://codereview.chromium.org/1207383006/diff/20001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_service.cc (right):

https://codereview.chromium.org/1207383006/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service.cc:171:
scoped_ptr<base::DictionaryValue> settings =
On 2015/07/06 02:50:51, raymes wrote:
> nit here and below: score_dict = 

Done.

https://codereview.chromium.org/1207383006/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service.cc:205:
DCHECK(origin.is_valid());
On 2015/07/06 02:50:51, raymes wrote:
> hmm I think because this is coming from disk we really should check it and
skip
> the origin if it's invalid?

Done.

https://codereview.chromium.org/1207383006/diff/20001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_service_unittest.cc (right):

https://codereview.chromium.org/1207383006/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service_unittest.cc:338: // Expect
that site engagement
On 2015/07/06 02:50:51, raymes wrote:
> nit: this comment looks unfinished

Oops.

https://codereview.chromium.org/1207383006/diff/20001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service_unittest.cc:341:
switches::kEnableSiteEngagementService);
On 2015/07/06 02:50:51, raymes wrote:
> nit: we should probably just move this into SetUp()

Done.

[calamity, 2015-07-07 02:55:07]

The CQ bit was checked by calamity@chromium.org

[calamity, 2015-07-07 02:55:07]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/1207383006/#ps40001
(title: "address comments")

[raymes, 2015-07-07 02:55:18]

lgtm~

[commit-bot: I haz the power, 2015-07-07 02:55:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1207383006/40001

[commit-bot: I haz the power, 2015-07-07 03:58:30]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-07-07 03:59:21]

Patchset 3 (id:??) landed as
https://crrev.com/f50cd9900ec9d317e05b6aa0a066df53543e3a33
Cr-Commit-Position: refs/heads/master@{#337558}