Integrate SiteEngagementEvictionPolicy with QuotaManager.

Integrate SiteEngagementEvictionPolicy with QuotaManager.

This CL hooks the SiteEngagementEvictionPolicy to get its usage map and
global quota values from the QuotaManager and then sending them to the
UI thread for calculation.

This CL also adds a test that verifies that this plumbing completes as
expected.

Actually instantiating the SiteEngagementEvictionPolicy in the business
codepath will be implemented at a later date.

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

[calamity, 2015-09-16 07:22:27]

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

[raymes, 2015-09-17 04:14:31]

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

https://codereview.chromium.org/1343273003/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_eviction_policy.cc:98:
CR_DEFINE_STATIC_LOCAL(
I think this should only be called from within a function. You could have a
Get...() but I'm wondering if there is something better we can do to pass the
ScoreProvider though?

https://codereview.chromium.org/1343273003/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_eviction_policy.cc:100:
score_provider_callback_,
I believe the trailing underscore style is only used for member variables.

https://codereview.chromium.org/1343273003/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_eviction_policy.cc:118: const
storage::GetOriginCallback& callback) {
Sorry I can't recall which threads this stuff gets called on. I assume this gets
called on the IO thread?

https://codereview.chromium.org/1343273003/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_eviction_policy.cc:159:
base::Bind(score_provider_callback_, browser_context_);
nit: {} for multiline if

https://codereview.chromium.org/1343273003/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_eviction_policy.cc:170: void
SiteEngagementEvictionPolicy::SetSiteEngagementScoreProviderCallback(
I can't see where this is called from, besides in tests? I might be missing
something.

https://codereview.chromium.org/1343273003/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_eviction_policy.cc:177: GURL
SiteEngagementEvictionPolicy::CalculateEvictionOrigin(
We should probably actually name this CalculateEvictionOriginForTest

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

https://codereview.chromium.org/1343273003/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_eviction_policy_unittest.cc:141: //
Ensure the SiteEngagementEvictionPolicy completes completes successfully
nit: completes completes

https://codereview.chromium.org/1343273003/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_eviction_policy_unittest.cc:147:
EXPECT_FALSE(eviction_origin().is_empty());
Can we actually verify what the expected origin should be?

[calamity, 2015-09-17 06:06:36]

Patchset #2 (id:20001) has been deleted

[calamity, 2015-09-17 06:06:46]

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

https://codereview.chromium.org/1343273003/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_eviction_policy.cc:98:
CR_DEFINE_STATIC_LOCAL(
On 2015/09/17 04:14:30, raymes wrote:
> I think this should only be called from within a function. You could have a
> Get...() but I'm wondering if there is something better we can do to pass the
> ScoreProvider though?

Cleaned this up as discussed.

https://codereview.chromium.org/1343273003/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_eviction_policy.cc:100:
score_provider_callback_,
On 2015/09/17 04:14:31, raymes wrote:
> I believe the trailing underscore style is only used for member variables.

Fixed.

https://codereview.chromium.org/1343273003/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_eviction_policy.cc:118: const
storage::GetOriginCallback& callback) {
On 2015/09/17 04:14:30, raymes wrote:
> Sorry I can't recall which threads this stuff gets called on. I assume this
gets
> called on the IO thread?

Yep, added DCHECKs to document.

Tests now use ThreadBundle because that's what the cool kids are into.

https://codereview.chromium.org/1343273003/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_eviction_policy.cc:159:
base::Bind(score_provider_callback_, browser_context_);
On 2015/09/17 04:14:30, raymes wrote:
> nit: {} for multiline if

Removed.

https://codereview.chromium.org/1343273003/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_eviction_policy.cc:170: void
SiteEngagementEvictionPolicy::SetSiteEngagementScoreProviderCallback(
On 2015/09/17 04:14:31, raymes wrote:
> I can't see where this is called from, besides in tests? I might be missing
> something.

Yeah, this is only for tests. Updated the name to reflect that.

https://codereview.chromium.org/1343273003/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_eviction_policy.cc:177: GURL
SiteEngagementEvictionPolicy::CalculateEvictionOrigin(
On 2015/09/17 04:14:31, raymes wrote:
> We should probably actually name this CalculateEvictionOriginForTest

Done.

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

https://codereview.chromium.org/1343273003/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_eviction_policy_unittest.cc:141: //
Ensure the SiteEngagementEvictionPolicy completes completes successfully
On 2015/09/17 04:14:31, raymes wrote:
> nit: completes completes

Done.

https://codereview.chromium.org/1343273003/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_eviction_policy_unittest.cc:147:
EXPECT_FALSE(eviction_origin().is_empty());
On 2015/09/17 04:14:31, raymes wrote:
> Can we actually verify what the expected origin should be?

I think that's the job of the unit tests. This test's primary purpose is to kick
the pipes and make sure data goes through correctly.

I can get rid of all but 1 origin from the test data and make sure that comes
through?

[raymes, 2015-09-17 06:58:21]

lgtm

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

https://codereview.chromium.org/1343273003/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_eviction_policy_unittest.cc:147:
EXPECT_FALSE(eviction_origin().is_empty());
I think it would be nice to verify that one of the 3 inputs came through for
sanity :)

[calamity, 2015-09-21 03:27:50]

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

[calamity, 2015-09-21 03:27:51]

+michaeln for storage/ OWNERS.

FYI, followup CLs exist at https://codereview.chromium.org/1355793002/ and
https://codereview.chromium.org/1354543002/ for context. I'll send them out to
raymes@ first, but they're there if you want to look at them.

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

https://codereview.chromium.org/1343273003/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_eviction_policy_unittest.cc:147:
EXPECT_FALSE(eviction_origin().is_empty());
On 2015/09/17 06:58:21, raymes wrote:
> I think it would be nice to verify that one of the 3 inputs came through for
> sanity :) 

Done.

[raymes, 2015-09-22 06:08:49]

I reviewed the storage stuff a bit and added some more comments.

https://codereview.chromium.org/1343273003/diff/80001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_eviction_policy.cc (right):

https://codereview.chromium.org/1343273003/diff/80001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_eviction_policy.cc:92: :
GetSiteEngagementService(browser_context);
nit: how about we fold this logic into GetSiteEngagementService() so we can just
have have:
SiteEngagementScoreProvider* score_provider = GetScoreProvider();

https://codereview.chromium.org/1343273003/diff/80001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_eviction_policy.cc:108:
manager_(manager),
nit: perhaps call this quota_manager_ for clarity?

https://codereview.chromium.org/1343273003/diff/80001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_eviction_policy.cc:114:
SiteEngagementEvictionPolicy::~SiteEngagementEvictionPolicy() {}
Should we fire eviction_origin_callback_ if we're destroyed prior to hearing
back from the quota_manager

https://codereview.chromium.org/1343273003/diff/80001/storage/browser/quota/c...
File storage/browser/quota/client_usage_tracker.cc (right):

https://codereview.chromium.org/1343273003/diff/80001/storage/browser/quota/c...
storage/browser/quota/client_usage_tracker.cc:176: void
ClientUsageTracker::GetUsageForCachedOrigins(
What's the difference between this and the above function?

https://codereview.chromium.org/1343273003/diff/80001/storage/browser/quota/q...
File storage/browser/quota/quota_manager.cc (right):

https://codereview.chromium.org/1343273003/diff/80001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:1452: is_getting_eviction_origin_ = true;
Should we dcheck this is false (as with lru_origin_callback_ in the old case)

https://codereview.chromium.org/1343273003/diff/80001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:1461: did_get_origin_callback);
Should this return here?

https://codereview.chromium.org/1343273003/diff/80001/storage/browser/quota/q...
File storage/browser/quota/quota_manager.h (right):

https://codereview.chromium.org/1343273003/diff/80001/storage/browser/quota/q...
storage/browser/quota/quota_manager.h:255: UsageTracker*
GetUsageTracker(StorageType type) const;
The storage people would be able to comment on whether it's better to expose the
UsageTracker directly or just to expose the usage map. What were your thoughts?

https://codereview.chromium.org/1343273003/diff/80001/storage/browser/quota/q...
storage/browser/quota/quota_manager.h:297: friend class
::SiteEngagementEvictionPolicyWithQuotaManagerTest;
nit: is the :: necessary, I think the forward declaration above should be
enough?

[calamity, 2015-09-23 01:36:38]

Patchset #5 (id:100001) has been deleted

[calamity, 2015-09-23 01:46:25]

https://codereview.chromium.org/1343273003/diff/80001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_eviction_policy.cc (right):

https://codereview.chromium.org/1343273003/diff/80001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_eviction_policy.cc:92: :
GetSiteEngagementService(browser_context);
On 2015/09/22 06:08:48, raymes wrote:
> nit: how about we fold this logic into GetSiteEngagementService() so we can
just
> have have:
> SiteEngagementScoreProvider* score_provider = GetScoreProvider();

I don't think that function was providing much value since I changed the way the
test callback works. Just inlined it here.

https://codereview.chromium.org/1343273003/diff/80001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_eviction_policy.cc:108:
manager_(manager),
On 2015/09/22 06:08:49, raymes wrote:
> nit: perhaps call this quota_manager_ for clarity?

Done.

https://codereview.chromium.org/1343273003/diff/80001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_eviction_policy.cc:114:
SiteEngagementEvictionPolicy::~SiteEngagementEvictionPolicy() {}
On 2015/09/22 06:08:49, raymes wrote:
> Should we fire eviction_origin_callback_ if we're destroyed prior to hearing
> back from the quota_manager

Hmm. Well, the quota manager is the one receiving as well as the owner of this
policy. I don't think anything bad will happen if we don't.

https://codereview.chromium.org/1343273003/diff/80001/storage/browser/quota/c...
File storage/browser/quota/client_usage_tracker.cc (right):

https://codereview.chromium.org/1343273003/diff/80001/storage/browser/quota/c...
storage/browser/quota/client_usage_tracker.cc:176: void
ClientUsageTracker::GetUsageForCachedOrigins(
On 2015/09/22 06:08:49, raymes wrote:
> What's the difference between this and the above function?

This one considers ports and scheme etc. The one above is purely based on the
host.

https://codereview.chromium.org/1343273003/diff/80001/storage/browser/quota/q...
File storage/browser/quota/quota_manager.cc (right):

https://codereview.chromium.org/1343273003/diff/80001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:1452: is_getting_eviction_origin_ = true;
On 2015/09/22 06:08:49, raymes wrote:
> Should we dcheck this is false (as with lru_origin_callback_ in the old case)

Done.

https://codereview.chromium.org/1343273003/diff/80001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:1461: did_get_origin_callback);
On 2015/09/22 06:08:49, raymes wrote:
> Should this return here?

Done. Good catch >_>

https://codereview.chromium.org/1343273003/diff/80001/storage/browser/quota/q...
File storage/browser/quota/quota_manager.h (right):

https://codereview.chromium.org/1343273003/diff/80001/storage/browser/quota/q...
storage/browser/quota/quota_manager.h:255: UsageTracker*
GetUsageTracker(StorageType type) const;
On 2015/09/22 06:08:49, raymes wrote:
> The storage people would be able to comment on whether it's better to expose
the
> UsageTracker directly or just to expose the usage map. What were your
thoughts?

I thought it would pollute the QuotaManager API less to just expose the usage
tracker.

https://codereview.chromium.org/1343273003/diff/80001/storage/browser/quota/q...
storage/browser/quota/quota_manager.h:297: friend class
::SiteEngagementEvictionPolicyWithQuotaManagerTest;
On 2015/09/22 06:08:49, raymes wrote:
> nit: is the :: necessary, I think the forward declaration above should be
> enough?

Didn't seem to be.

C++, how does it work.

[raymes, 2015-09-23 01:56:14]

lgtm

https://codereview.chromium.org/1343273003/diff/80001/storage/browser/quota/c...
File storage/browser/quota/client_usage_tracker.cc (right):

https://codereview.chromium.org/1343273003/diff/80001/storage/browser/quota/c...
storage/browser/quota/client_usage_tracker.cc:176: void
ClientUsageTracker::GetUsageForCachedOrigins(
On 2015/09/23 01:46:24, calamity wrote:
> On 2015/09/22 06:08:49, raymes wrote:
> > What's the difference between this and the above function?
> 
> This one considers ports and scheme etc. The one above is purely based on the
> host.

Should we use the same naming convention? GetCachedOriginsUsage?

https://codereview.chromium.org/1343273003/diff/80001/storage/browser/quota/q...
File storage/browser/quota/quota_manager.h (right):

https://codereview.chromium.org/1343273003/diff/80001/storage/browser/quota/q...
storage/browser/quota/quota_manager.h:297: friend class
::SiteEngagementEvictionPolicyWithQuotaManagerTest;
On 2015/09/23 01:46:24, calamity wrote:
> On 2015/09/22 06:08:49, raymes wrote:
> > nit: is the :: necessary, I think the forward declaration above should be
> > enough?
> 
> Didn't seem to be.
> 
> C++, how does it work.

Agreed!

https://codereview.chromium.org/1343273003/diff/120001/storage/browser/quota/...
File storage/browser/quota/client_usage_tracker.cc (right):

https://codereview.chromium.org/1343273003/diff/120001/storage/browser/quota/...
storage/browser/quota/client_usage_tracker.cc:172: LOG(ERROR) << host;
nit: remove this

[calamity, 2015-09-23 02:15:57]

https://codereview.chromium.org/1343273003/diff/80001/storage/browser/quota/c...
File storage/browser/quota/client_usage_tracker.cc (right):

https://codereview.chromium.org/1343273003/diff/80001/storage/browser/quota/c...
storage/browser/quota/client_usage_tracker.cc:176: void
ClientUsageTracker::GetUsageForCachedOrigins(
On 2015/09/23 01:56:14, raymes wrote:
> On 2015/09/23 01:46:24, calamity wrote:
> > On 2015/09/22 06:08:49, raymes wrote:
> > > What's the difference between this and the above function?
> > 
> > This one considers ports and scheme etc. The one above is purely based on
the
> > host.
> 
> Should we use the same naming convention? GetCachedOriginsUsage?
> 

There was a method that was already named that in the header but was
unimplemented. Deleted it and changed this.

https://codereview.chromium.org/1343273003/diff/120001/storage/browser/quota/...
File storage/browser/quota/client_usage_tracker.cc (right):

https://codereview.chromium.org/1343273003/diff/120001/storage/browser/quota/...
storage/browser/quota/client_usage_tracker.cc:172: LOG(ERROR) << host;
On 2015/09/23 01:56:14, raymes wrote:
> nit: remove this

Done.

[michaeln, 2015-09-23 20:33:50]

https://codereview.chromium.org/1343273003/diff/140001/storage/browser/quota/...
File storage/browser/quota/quota_manager.h (right):

https://codereview.chromium.org/1343273003/diff/140001/storage/browser/quota/...
storage/browser/quota/quota_manager.h:255: UsageTracker*
GetUsageTracker(StorageType type) const;
I don't think we should expose the UsageTracker in this way. We are planning to
revamp the quota system and exposing this internal class would prohibit
significant changes. (In the revamped system, that class may not be there at
all?)

I'd like to codify the contract in the eviction policy class and pass into it
data that the quota system has that may be relevant to eviction.

[calamity, 2015-09-24 04:14:26]

> File storage/browser/quota/quota_manager.h (right):
> 
>
https://codereview.chromium.org/1343273003/diff/140001/storage/browser/quota/...
> storage/browser/quota/quota_manager.h:255: UsageTracker*
> GetUsageTracker(StorageType type) const;
> I don't think we should expose the UsageTracker in this way. We are planning
to
> revamp the quota system and exposing this internal class would prohibit
> significant changes. (In the revamped system, that class may not be there at
> all?)
> 
> I'd like to codify the contract in the eviction policy class and pass into it
> data that the quota system has that may be relevant to eviction.

Hmm alright. In that case, I guess my proposed API would be

class QuotaEvictionPolicy {

  struct EvictionPolicyInfo {
    GURL lru_origin;
    std::map<GURL, int64> origin_usage_map;
    int64 global_quota;
  };

  virtual void GetEvictionOrigin(
      scoped_ptr<EvictionPolicyInfo> info,
      const scoped_refptr<SpecialStoragePolicy>& special_storage_policy,
      const GetOriginCallback& callback) = 0;

  virtual bool NeedsLRUOrigin() { return false; }
  virtual bool NeedsOriginUsageMap() { return false; }
  virtual bool NeedsGlobalQuota() { return false; }
}

So policies will request only what they need and then a new QuotaTask in the
QuotaManager will go fetch the right things and then call
QuotaEvictionPolicy::GetEvictionOrigin with the right data.

Also I'll need to implement a QuotaTask that fetches the origin usage that I
want.

Does that design sound about right?

Is there any info about the quota system redesign you can direct me to?

We're hoping to get this in for 47 (2nd Oct branch date) and there are some
extra CLs behind this one.

[calamity, 2015-09-29 07:09:16]

https://codereview.chromium.org/1343273003/diff/140001/storage/browser/quota/...
File storage/browser/quota/quota_manager.h (right):

https://codereview.chromium.org/1343273003/diff/140001/storage/browser/quota/...
storage/browser/quota/quota_manager.h:255: UsageTracker*
GetUsageTracker(StorageType type) const;
On 2015/09/23 20:33:50, michaeln wrote:
> I don't think we should expose the UsageTracker in this way. We are planning
to
> revamp the quota system and exposing this internal class would prohibit
> significant changes. (In the revamped system, that class may not be there at
> all?)
> 
> I'd like to codify the contract in the eviction policy class and pass into it
> data that the quota system has that may be relevant to eviction.

Done.

[calamity, 2015-09-30 03:29:46]

Patchset #7 (id:160001) has been deleted

[michaeln, 2015-10-06 00:35:09]

https://codereview.chromium.org/1343273003/diff/220001/chrome/browser/engagem...
File chrome/browser/engagement/site_engagement_eviction_policy.cc (right):

https://codereview.chromium.org/1343273003/diff/220001/chrome/browser/engagem...
chrome/browser/engagement/site_engagement_eviction_policy.cc:116:
info->origin_usage_map, info->global_quota),
I thought you might have created the |info| struct to avoid copying the map, but
i see it being copied at this point anyway? If you're not going to use the info
struct in that fashion, maybe leave the GetEvictionOrigin() method signature as
is since the struct has very few data members.

https://codereview.chromium.org/1343273003/diff/220001/storage/browser/quota/...
File storage/browser/quota/client_usage_tracker.h (left):

https://codereview.chromium.org/1343273003/diff/220001/storage/browser/quota/...
storage/browser/quota/client_usage_tracker.h:55: int64
GetCachedOriginsUsage(const std::set<GURL>& origins,
thnx for cleaning this up

https://codereview.chromium.org/1343273003/diff/220001/storage/browser/quota/...
File storage/browser/quota/quota_manager.cc (right):

https://codereview.chromium.org/1343273003/diff/220001/storage/browser/quota/...
storage/browser/quota/quota_manager.cc:542: class
QuotaManager::GetEvictionOriginTask : public QuotaTask {
This task may not bee needed?

At the start of an eviction round, the QuotaTemporaryStorageEvictor class
gathers data needed for the round. All of the data this tasks gathers is readily
available when GetUsageAndQuotaForEviction completes w/o any additional async
steps. I think you can populate an origin map at that time prior to calling
GetEvictionOrigin.

https://codereview.chromium.org/1343273003/diff/220001/storage/browser/quota/...
storage/browser/quota/quota_manager.cc:563:
->GetGlobalUsage(base::Bind(&GetEvictionOriginTask::DidGetGlobalUsage,
The base::BarrierClosure (barrier_closure.h) class exists to handle this pattern
where a final done closure in invoked after a number of invocations of the
barrier.

I think this method can complete immediately and may call DidGetGlobalUsage
prior to return. So remaining_tasks_ could be set to -1 then get incremented
back to 0 below which is probably not what you had in mind.
GetTemporaryGlobalQuota method can complete immediately too.

https://codereview.chromium.org/1343273003/diff/220001/storage/browser/quota/...
File storage/browser/quota/quota_manager.h (right):

https://codereview.chromium.org/1343273003/diff/220001/storage/browser/quota/...
storage/browser/quota/quota_manager.h:98: virtual bool NeedsGlobalQuota();
It'd be simpler to always include this info and I think it comes nearly for free
as the data is readily available at the time GetEvictionOrigin is called.

https://codereview.chromium.org/1343273003/diff/220001/storage/browser/quota/...
storage/browser/quota/quota_manager.h:213: void
SetQuotaEvictionPolicy(StorageType type,
We can probably constrain this to just "temp" storage type so I think we can
remove the |type| param from this method. It'll be easy enough to expand if we
ever need to.

https://codereview.chromium.org/1343273003/diff/220001/storage/browser/quota/...
storage/browser/quota/quota_manager.h:267: UsageTracker*
GetUsageTracker(StorageType type) const;
pleas keep this method private

https://codereview.chromium.org/1343273003/diff/220001/storage/browser/quota/...
storage/browser/quota/quota_manager.h:488: eviction_policy_map_;
lets put these new data members next to the other evcition related data members

[calamity, 2015-10-07 02:14:11]

Patchset #11 (id:260001) has been deleted

[calamity, 2015-10-07 02:41:53]

Patchset #11 (id:280001) has been deleted

[calamity, 2015-10-07 02:42:46]

https://codereview.chromium.org/1343273003/diff/220001/chrome/browser/engagem...
File chrome/browser/engagement/site_engagement_eviction_policy.cc (right):

https://codereview.chromium.org/1343273003/diff/220001/chrome/browser/engagem...
chrome/browser/engagement/site_engagement_eviction_policy.cc:116:
info->origin_usage_map, info->global_quota),
On 2015/10/06 00:35:09, michaeln wrote:
> I thought you might have created the |info| struct to avoid copying the map,
but
> i see it being copied at this point anyway? If you're not going to use the
info
> struct in that fashion, maybe leave the GetEvictionOrigin() method signature
as
> is since the struct has very few data members.

Done.

https://codereview.chromium.org/1343273003/diff/220001/storage/browser/quota/...
File storage/browser/quota/quota_manager.cc (right):

https://codereview.chromium.org/1343273003/diff/220001/storage/browser/quota/...
storage/browser/quota/quota_manager.cc:542: class
QuotaManager::GetEvictionOriginTask : public QuotaTask {
On 2015/10/06 00:35:09, michaeln wrote:
> This task may not bee needed?
> 
> At the start of an eviction round, the QuotaTemporaryStorageEvictor class
> gathers data needed for the round. All of the data this tasks gathers is
readily
> available when GetUsageAndQuotaForEviction completes w/o any additional async
> steps. I think you can populate an origin map at that time prior to calling
> GetEvictionOrigin.

Ah ok, cool. Deleted this.

https://codereview.chromium.org/1343273003/diff/220001/storage/browser/quota/...
storage/browser/quota/quota_manager.cc:563:
->GetGlobalUsage(base::Bind(&GetEvictionOriginTask::DidGetGlobalUsage,
On 2015/10/06 00:35:09, michaeln wrote:
> The base::BarrierClosure (barrier_closure.h) class exists to handle this
pattern
> where a final done closure in invoked after a number of invocations of the
> barrier.
> 
> I think this method can complete immediately and may call DidGetGlobalUsage
> prior to return. So remaining_tasks_ could be set to -1 then get incremented
> back to 0 below which is probably not what you had in mind.
> GetTemporaryGlobalQuota method can complete immediately too.

Acknowledged.

https://codereview.chromium.org/1343273003/diff/220001/storage/browser/quota/...
File storage/browser/quota/quota_manager.h (right):

https://codereview.chromium.org/1343273003/diff/220001/storage/browser/quota/...
storage/browser/quota/quota_manager.h:98: virtual bool NeedsGlobalQuota();
On 2015/10/06 00:35:09, michaeln wrote:
> It'd be simpler to always include this info and I think it comes nearly for
free
> as the data is readily available at the time GetEvictionOrigin is called.

Done.

https://codereview.chromium.org/1343273003/diff/220001/storage/browser/quota/...
storage/browser/quota/quota_manager.h:213: void
SetQuotaEvictionPolicy(StorageType type,
On 2015/10/06 00:35:09, michaeln wrote:
> We can probably constrain this to just "temp" storage type so I think we can
> remove the |type| param from this method. It'll be easy enough to expand if we
> ever need to.

Done.

https://codereview.chromium.org/1343273003/diff/220001/storage/browser/quota/...
storage/browser/quota/quota_manager.h:267: UsageTracker*
GetUsageTracker(StorageType type) const;
On 2015/10/06 00:35:09, michaeln wrote:
> pleas keep this method private

Done.

https://codereview.chromium.org/1343273003/diff/220001/storage/browser/quota/...
storage/browser/quota/quota_manager.h:488: eviction_policy_map_;
On 2015/10/06 00:35:09, michaeln wrote:
> lets put these new data members next to the other evcition related data
members

Done.

[michaeln, 2015-10-12 23:36:26]

https://codereview.chromium.org/1343273003/diff/300001/chrome/browser/engagem...
File chrome/browser/engagement/site_engagement_eviction_policy.h (right):

https://codereview.chromium.org/1343273003/diff/300001/chrome/browser/engagem...
chrome/browser/engagement/site_engagement_eviction_policy.h:40:
base::Callback<SiteEngagementScoreProvider*(content::BrowserContext*)>*
usually we pass these by const ref

https://codereview.chromium.org/1343273003/diff/300001/chrome/browser/engagem...
File chrome/browser/engagement/site_engagement_eviction_policy_unittest.cc
(right):

https://codereview.chromium.org/1343273003/diff/300001/chrome/browser/engagem...
chrome/browser/engagement/site_engagement_eviction_policy_unittest.cc:66: class
SiteEngagementEvictionPolicyWithQuotaManagerTest : public testing::Test {
The production code looks pretty straight forward but the test code seems overly
complicated somehow? The ratio of test apparatus to production code is off
balance. Some thoughts on that.

I think there are two things to test...

1) Ensure the QuotaManager invokes a caller provided eviction policy. This test
doesn't need to use the SiteEngagementEvictionPolicy at all, would that keeps
track of whether its been called would be sufficient. A test like that probably
belongs in quota_manager_unittest.cc (or maybe in
quota_temporary_storage_evictor_unittest.cc).

2) Ensure the logic in the SiteEngagementEvictionPolicy does what you want. It's
a class with one public method. The most direct way of testing that is to simply
allocate the object on the stack and call its GetEvictionOrigin() method and see
that it does what you want.  I don't think the CalculateEvictionOriginForTests
function is needed really. Also, what if the policy class was constructed with a
score provider, I think that might make tests easier to compose w/o special
cases, wdyt?

class SiteEngagementEvictionPolicy {
 public:
  explicit SiteEngagementEvictionPolicy(scoped_ptr<SiteEngagementScoreProvider>
score_provider);
 private:
  score_provider_;
};

https://codereview.chromium.org/1343273003/diff/300001/chrome/browser/engagem...
chrome/browser/engagement/site_engagement_eviction_policy_unittest.cc:75: false,
data_dir_.path(), base::ThreadTaskRunnerHandle::Get().get(),
The trailing .get()s are not needed (or wanted really). The QuotaManager
constructor accepts const refs to scoped_refptr<T>s.

(You probably copy/pasted this from another test. In the distant past, i think
the ctor took rawptr types as inputs.)

[calamity, 2015-10-13 03:48:04]

Patchset #12 (id:320001) has been deleted

[calamity, 2015-10-13 08:34:40]

https://codereview.chromium.org/1343273003/diff/300001/chrome/browser/engagem...
File chrome/browser/engagement/site_engagement_eviction_policy.h (right):

https://codereview.chromium.org/1343273003/diff/300001/chrome/browser/engagem...
chrome/browser/engagement/site_engagement_eviction_policy.h:40:
base::Callback<SiteEngagementScoreProvider*(content::BrowserContext*)>*
On 2015/10/12 23:36:26, michaeln wrote:
> usually we pass these by const ref

Removed since the integration test w/ QuotaManager was removed.

https://codereview.chromium.org/1343273003/diff/300001/chrome/browser/engagem...
File chrome/browser/engagement/site_engagement_eviction_policy_unittest.cc
(right):

https://codereview.chromium.org/1343273003/diff/300001/chrome/browser/engagem...
chrome/browser/engagement/site_engagement_eviction_policy_unittest.cc:66: class
SiteEngagementEvictionPolicyWithQuotaManagerTest : public testing::Test {
On 2015/10/12 23:36:26, michaeln wrote:
> The production code looks pretty straight forward but the test code seems
overly
> complicated somehow? The ratio of test apparatus to production code is off
> balance. Some thoughts on that.
> 

Ah yeah, this was an integration test with the QuotaManager that was good to
have when the policy was asking the QuotaManager for all its data. It's no
longer necessary so removed.

> I think there are two things to test...
> 
> 1) Ensure the QuotaManager invokes a caller provided eviction policy. This
test
> doesn't need to use the SiteEngagementEvictionPolicy at all, would that keeps
> track of whether its been called would be sufficient. A test like that
probably
> belongs in quota_manager_unittest.cc (or maybe in
> quota_temporary_storage_evictor_unittest.cc).
> 

Agreed. Done.

> 2) Ensure the logic in the SiteEngagementEvictionPolicy does what you want.
It's
> a class with one public method. The most direct way of testing that is to
simply
> allocate the object on the stack and call its GetEvictionOrigin() method and
see
> that it does what you want.  I don't think the CalculateEvictionOriginForTests
> function is needed really. Also, what if the policy class was constructed with
a
> score provider, I think that might make tests easier to compose w/o special
> cases, wdyt?
> 
> class SiteEngagementEvictionPolicy {
>  public:
>   explicit
SiteEngagementEvictionPolicy(scoped_ptr<SiteEngagementScoreProvider>
> score_provider);
>  private:
>   score_provider_;
> };
> 

The browser context is used to ensure lifetime is not an issue. This accounts
for the case where the SiteEngagementService is already destroyed when the UI
thread task is run.

If there are no objections to keeping the BrowserContext as the ScoreProvider
key, I think CalculateEvictionOriginForTests is a good testing API because it
directly tests the business logic of the class and is a nice place to inject a
ScoreProvider for testing.

https://codereview.chromium.org/1343273003/diff/300001/chrome/browser/engagem...
chrome/browser/engagement/site_engagement_eviction_policy_unittest.cc:75: false,
data_dir_.path(), base::ThreadTaskRunnerHandle::Get().get(),
On 2015/10/12 23:36:26, michaeln wrote:
> The trailing .get()s are not needed (or wanted really). The QuotaManager
> constructor accepts const refs to scoped_refptr<T>s.
> 
> (You probably copy/pasted this from another test. In the distant past, i think
> the ctor took rawptr types as inputs.)

N/A.

[michaeln, 2015-10-13 20:13:39]

lgtm, thnx for cleaning up the tests

https://codereview.chromium.org/1343273003/diff/300001/chrome/browser/engagem...
File chrome/browser/engagement/site_engagement_eviction_policy_unittest.cc
(right):

https://codereview.chromium.org/1343273003/diff/300001/chrome/browser/engagem...
chrome/browser/engagement/site_engagement_eviction_policy_unittest.cc:66: class
SiteEngagementEvictionPolicyWithQuotaManagerTest : public testing::Test {
On 2015/10/13 08:34:40, calamity wrote:
> On 2015/10/12 23:36:26, michaeln wrote:
> > The production code looks pretty straight forward but the test code seems
> overly
> > complicated somehow? The ratio of test apparatus to production code is off
> > balance. Some thoughts on that.
> > 
> 
> Ah yeah, this was an integration test with the QuotaManager that was good to
> have when the policy was asking the QuotaManager for all its data. It's no
> longer necessary so removed.
> 
> > I think there are two things to test...
> > 
> > 1) Ensure the QuotaManager invokes a caller provided eviction policy. This
> test
> > doesn't need to use the SiteEngagementEvictionPolicy at all, would that
keeps
> > track of whether its been called would be sufficient. A test like that
> probably
> > belongs in quota_manager_unittest.cc (or maybe in
> > quota_temporary_storage_evictor_unittest.cc).
> > 
> 
> Agreed. Done.
> 
> > 2) Ensure the logic in the SiteEngagementEvictionPolicy does what you want.
> It's
> > a class with one public method. The most direct way of testing that is to
> simply
> > allocate the object on the stack and call its GetEvictionOrigin() method and
> see
> > that it does what you want.  I don't think the
CalculateEvictionOriginForTests
> > function is needed really. Also, what if the policy class was constructed
with
> a
> > score provider, I think that might make tests easier to compose w/o special
> > cases, wdyt?
> > 
> > class SiteEngagementEvictionPolicy {
> >  public:
> >   explicit
> SiteEngagementEvictionPolicy(scoped_ptr<SiteEngagementScoreProvider>
> > score_provider);
> >  private:
> >   score_provider_;
> > };
> > 
> 
> The browser context is used to ensure lifetime is not an issue. This accounts
> for the case where the SiteEngagementService is already destroyed when the UI
> thread task is run.
> 
> If there are no objections to keeping the BrowserContext as the ScoreProvider
> key, I think CalculateEvictionOriginForTests is a good testing API because it
> directly tests the business logic of the class and is a nice place to inject a
> ScoreProvider for testing.

Ok, thnx for getting rid of the global ptr in the production code and the test
function will get stripped out by the linker.

https://codereview.chromium.org/1343273003/diff/340001/content/browser/quota/...
File content/browser/quota/quota_manager_unittest.cc (right):

https://codereview.chromium.org/1343273003/diff/340001/content/browser/quota/...
content/browser/quota/quota_manager_unittest.cc:77:
callback.Run(GURL("http://bar.com/"));
maybe make the test check for an explicit value returned here so its unambiguous
about where the result came from: ala GURL("http://test.eviction.policy/result")

https://codereview.chromium.org/1343273003/diff/340001/content/browser/quota/...
content/browser/quota/quota_manager_unittest.cc:1285:
EXPECT_TRUE(eviction_origin().is_empty());
maybe no need to test this case if the unambiguous result is EXPECTED_EQ below

https://codereview.chromium.org/1343273003/diff/340001/storage/browser/quota/...
File storage/browser/quota/quota_manager.cc (right):

https://codereview.chromium.org/1343273003/diff/340001/storage/browser/quota/...
storage/browser/quota/quota_manager.cc:1462: // This is populated by the prior
call to GetUsageAndQuotaForEviction().
nit: "The cached origins are" ...

https://codereview.chromium.org/1343273003/diff/340001/storage/browser/quota/...
File storage/browser/quota/quota_temporary_storage_evictor.cc (right):

https://codereview.chromium.org/1343273003/diff/340001/storage/browser/quota/...
storage/browser/quota/quota_temporary_storage_evictor.cc:199:
kStorageTypeTemporary, qau.quota,
funny variable name :)

[calamity, 2015-10-14 04:49:16]

https://codereview.chromium.org/1343273003/diff/340001/content/browser/quota/...
File content/browser/quota/quota_manager_unittest.cc (right):

https://codereview.chromium.org/1343273003/diff/340001/content/browser/quota/...
content/browser/quota/quota_manager_unittest.cc:77:
callback.Run(GURL("http://bar.com/"));
On 2015/10/13 20:13:39, michaeln wrote:
> maybe make the test check for an explicit value returned here so its
unambiguous
> about where the result came from: ala
GURL("http://test.eviction.policy/result")

Done.

https://codereview.chromium.org/1343273003/diff/340001/content/browser/quota/...
content/browser/quota/quota_manager_unittest.cc:1285:
EXPECT_TRUE(eviction_origin().is_empty());
On 2015/10/13 20:13:39, michaeln wrote:
> maybe no need to test this case if the unambiguous result is EXPECTED_EQ below

Done.

https://codereview.chromium.org/1343273003/diff/340001/storage/browser/quota/...
File storage/browser/quota/quota_manager.cc (right):

https://codereview.chromium.org/1343273003/diff/340001/storage/browser/quota/...
storage/browser/quota/quota_manager.cc:1462: // This is populated by the prior
call to GetUsageAndQuotaForEviction().
On 2015/10/13 20:13:39, michaeln wrote:
> nit: "The cached origins are" ...

Done.

https://codereview.chromium.org/1343273003/diff/340001/storage/browser/quota/...
File storage/browser/quota/quota_temporary_storage_evictor.cc (right):

https://codereview.chromium.org/1343273003/diff/340001/storage/browser/quota/...
storage/browser/quota/quota_temporary_storage_evictor.cc:199:
kStorageTypeTemporary, qau.quota,
On 2015/10/13 20:13:39, michaeln wrote:
> funny variable name :)

Yeah, should at least be uaq, right?

[calamity, 2015-10-14 04:49:27]

The CQ bit was checked by calamity@chromium.org

[calamity, 2015-10-14 04:49:27]

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

[commit-bot: I haz the power, 2015-10-14 04:50:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1343273003/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1343273003/360001

[commit-bot: I haz the power, 2015-10-14 05:00:42]

Committed patchset #13 (id:360001)

[commit-bot: I haz the power, 2015-10-14 05:01:41]

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

[calamity, 2015-10-19 04:50:51]

Patchset #14 (id:380001) has been deleted