I'm not sure why this is better. I'd prefer to pass the clock in once at construction rather than passing clock->Now() at every Score(), AddPoints() and MaxPointsPerDayAdded().

To avoid the nastiness of passing a nullptr Time, we can just call clock_->Now() for that one case since it makes sense to say the caller of Reset must specify the new last access time.

The primary reason is that SiteEngagementScore is an implementation detail that's almost always used like this:
  // Create score (passes in clock)
  // Call a single method on score
  // Return
Only in testing are multiple methods called on the same score repeatedly, so the change basically moves passing in the clock on construction to passing in clock->Now on the single method that's called. Given that, it seems a bit strange to pass in the clock; the site engagement service is in charge of times, and the score just does what it's told.

The subtlety with Reset() is that it needs to *not* reset the last shortcut launch time in most cases, except in the case when history is being cleared. That means calling it with clock->Now() is incorrect; you don't want to set the last shortcut launch time to Now(). We could either duplicate a bunch of the reset code for the history clearing case, or pass in the null time.

How do you feel about this: https://codereview.chromium.org/1919383005/ ?

It centralizes the clock passing, removes all the update duplication and moves the shortcut time setting logic into the SiteEngagementService (it was weird for the SiteEngagementScore to decide that the score shouldn't get set in certain cases).

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/090145cb43d2542eaac21d6cfd5c0580b77a1dbf

commit 090145cb43d2542eaac21d6cfd5c0580b77a1dbf
Author: calamity <calamity@chromium.org>
Date: Tue May 31 05:47:55 2016

Small cleanup of SiteEngagementService.

This CL moves pref management into SiteEngagementScore so that
SiteEngagementService has a cleaner flow for operating on
SiteEngagementScores.

BUG= 604305 

Review-Url: https://codereview.chromium.org/1919383005
Cr-Commit-Position: refs/heads/master@{#396789}

[modify] https://crrev.com/090145cb43d2542eaac21d6cfd5c0580b77a1dbf/chrome/browser/engagement/site_engagement_score.cc
[modify] https://crrev.com/090145cb43d2542eaac21d6cfd5c0580b77a1dbf/chrome/browser/engagement/site_engagement_score.h
[modify] https://crrev.com/090145cb43d2542eaac21d6cfd5c0580b77a1dbf/chrome/browser/engagement/site_engagement_score_unittest.cc
[modify] https://crrev.com/090145cb43d2542eaac21d6cfd5c0580b77a1dbf/chrome/browser/engagement/site_engagement_service.cc
[modify] https://crrev.com/090145cb43d2542eaac21d6cfd5c0580b77a1dbf/chrome/browser/engagement/site_engagement_service.h

Moving this nonessential bug to the next milestone.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

This issue has been moved once and is lower than Pri-1. Removing the milestone.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Security>UX component is deprecated in favor of the Team-Security-UX label