Reporting: Cap number of reports in cache.

Reporting: Cap number of reports in cache.

BUG=704259
Review-Url: https://codereview.chromium.org/2847813002
Cr-Commit-Position: refs/heads/master@{#470353}
Committed: https://chromium.googlesource.com/chromium/src/+/4e18c94a1f6cc8f76720e1bf7b8f7b9cec1ad4ac

[Julia Tuttle, 2017-04-27 16:58:55]

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

[commit-bot: I haz the power, 2017-04-27 16:59:19]

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, 2017-04-27 17:16:27]

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

[commit-bot: I haz the power, 2017-04-27 17:16:28]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Julia Tuttle, 2017-04-28 17:40:31]

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

[commit-bot: I haz the power, 2017-04-28 17:41:00]

Dry run: CQ is trying da patch.

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

[Julia Tuttle, 2017-04-28 17:44:41]

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

[Julia Tuttle, 2017-04-28 17:44:51]

juliatuttle@chromium.org changed reviewers:
+ shivanisha@chromium.org

[Julia Tuttle, 2017-04-28 17:44:52]

PTAL, shivanisha.

[commit-bot: I haz the power, 2017-04-28 17:45:08]

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, 2017-04-28 18:58:27]

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

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

Dry run: This issue passed the CQ dry run.

[shivanisha, 2017-05-08 20:38:13]

On 2017/04/28 at 17:44:52, juliatuttle wrote:
> PTAL, shivanisha.

Will be looking at it today evening/tomorrow morning. Sorry for the delay.

[shivanisha, 2017-05-09 13:58:28]

https://codereview.chromium.org/2847813002/diff/40001/net/reporting/reporting...
File net/reporting/reporting_cache.cc (right):

https://codereview.chromium.org/2847813002/diff/40001/net/reporting/reporting...
net/reporting/reporting_cache.cc:69: // pending, the cache should have a report
to evict.
dcheck that to_evict is not pending?

https://codereview.chromium.org/2847813002/diff/40001/net/reporting/reporting...
net/reporting/reporting_cache.cc:73: DCHECK_EQ(1u, erased);
May be call RemoveReports instead of lines 71 to 73?

https://codereview.chromium.org/2847813002/diff/40001/net/reporting/reporting...
File net/reporting/reporting_cache_unittest.cc (right):

https://codereview.chromium.org/2847813002/diff/40001/net/reporting/reporting...
net/reporting/reporting_cache_unittest.cc:420: tick_clock()->NowTicks(), 0);
Its possible for earliest_queued to be different than tick_clock()->NowTicks()
for the first report added. And that will make the subsequent
EXPECT_NE(earliest_queued, report->queued) invalid. So for i==0 use
earliest_queued instead of tick_clock()->NowTicks()?

[Julia Tuttle, 2017-05-09 14:37:09]

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

[Julia Tuttle, 2017-05-09 14:37:22]

PTAL, shivanisha.

https://codereview.chromium.org/2847813002/diff/40001/net/reporting/reporting...
File net/reporting/reporting_cache.cc (right):

https://codereview.chromium.org/2847813002/diff/40001/net/reporting/reporting...
net/reporting/reporting_cache.cc:69: // pending, the cache should have a report
to evict.
On 2017/05/09 13:58:27, shivanisha wrote:
> dcheck that to_evict is not pending?

I do that, two lines below. I'll move the comment down so it's lined up right.

https://codereview.chromium.org/2847813002/diff/40001/net/reporting/reporting...
net/reporting/reporting_cache.cc:73: DCHECK_EQ(1u, erased);
On 2017/05/09 13:58:27, shivanisha wrote:
> May be call RemoveReports instead of lines 71 to 73?

Don't wanna check pending_reports_ and call OnCacheUpdated twice, and the actual
removal is, like, two lines.

https://codereview.chromium.org/2847813002/diff/40001/net/reporting/reporting...
File net/reporting/reporting_cache_unittest.cc (right):

https://codereview.chromium.org/2847813002/diff/40001/net/reporting/reporting...
net/reporting/reporting_cache_unittest.cc:420: tick_clock()->NowTicks(), 0);
On 2017/05/09 13:58:27, shivanisha wrote:
> Its possible for earliest_queued to be different than tick_clock()->NowTicks()
> for the first report added. And that will make the subsequent
> EXPECT_NE(earliest_queued, report->queued) invalid. So for i==0 use
> earliest_queued instead of tick_clock()->NowTicks()?

tick_clock() refers to the SimpleTestTickClock created in TestReportingContext,
so it won't advance except when I ask it to.

[commit-bot: I haz the power, 2017-05-09 14:37:33]

Dry run: CQ is trying da patch.

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

[shivanisha, 2017-05-09 14:43:29]

On 2017/05/09 at 14:37:22, juliatuttle wrote:
> PTAL, shivanisha.
> 
>
https://codereview.chromium.org/2847813002/diff/40001/net/reporting/reporting...
> File net/reporting/reporting_cache.cc (right):
> 
>
https://codereview.chromium.org/2847813002/diff/40001/net/reporting/reporting...
> net/reporting/reporting_cache.cc:69: // pending, the cache should have a
report to evict.
> On 2017/05/09 13:58:27, shivanisha wrote:
> > dcheck that to_evict is not pending?
> 
> I do that, two lines below. I'll move the comment down so it's lined up right.
> 
>
https://codereview.chromium.org/2847813002/diff/40001/net/reporting/reporting...
> net/reporting/reporting_cache.cc:73: DCHECK_EQ(1u, erased);
> On 2017/05/09 13:58:27, shivanisha wrote:
> > May be call RemoveReports instead of lines 71 to 73?
> 
> Don't wanna check pending_reports_ and call OnCacheUpdated twice, and the
actual removal is, like, two lines.
> 
>
https://codereview.chromium.org/2847813002/diff/40001/net/reporting/reporting...
> File net/reporting/reporting_cache_unittest.cc (right):
> 
>
https://codereview.chromium.org/2847813002/diff/40001/net/reporting/reporting...
> net/reporting/reporting_cache_unittest.cc:420: tick_clock()->NowTicks(), 0);
> On 2017/05/09 13:58:27, shivanisha wrote:
> > Its possible for earliest_queued to be different than
tick_clock()->NowTicks()
> > for the first report added. And that will make the subsequent
> > EXPECT_NE(earliest_queued, report->queued) invalid. So for i==0 use
> > earliest_queued instead of tick_clock()->NowTicks()?
> 
> tick_clock() refers to the SimpleTestTickClock created in
TestReportingContext, so it won't advance except when I ask it to.

lgtm

[Julia Tuttle, 2017-05-09 14:50:05]

The CQ bit was unchecked by juliatuttle@chromium.org

[Julia Tuttle, 2017-05-09 14:50:10]

The CQ bit was checked by juliatuttle@chromium.org

[commit-bot: I haz the power, 2017-05-09 14:50:39]

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-09 16:45:56]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494341410080110,
"parent_rev": "3bb853ef1d06f0ce7338b98f39388dbe60a291a6", "commit_rev":
"4e18c94a1f6cc8f76720e1bf7b8f7b9cec1ad4ac"}

[commit-bot: I haz the power, 2017-05-09 16:46:10]

Description was changed from

==========
Reporting: Cap number of reports in cache.

BUG=704259
==========

to

==========
Reporting: Cap number of reports in cache.

BUG=704259

Review-Url: https://codereview.chromium.org/2847813002
Cr-Commit-Position: refs/heads/master@{#470353}
Committed:
https://chromium.googlesource.com/chromium/src/+/4e18c94a1f6cc8f76720e1bf7b8f...
==========

[commit-bot: I haz the power, 2017-05-09 16:46:12]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/4e18c94a1f6cc8f76720e1bf7b8f...