Precache manifest should not be added to fetcher pool which is full

Precache manifest should not be added to fetcher pool which is full

Fetcher pool accepts limited number of requests which are fetched in
parallel. If the pool is full, precache manifest resources should not be
added to it.

BUG=606231
Committed: https://crrev.com/b2754f713e984d267353ce00d8a9e5ff6a6fafae
Cr-Commit-Position: refs/heads/master@{#399387}

[Raj, 2016-06-03 00:15:25]

rajendrant@chromium.org changed reviewers:
+ bengr@chromium.org

[bengr, 2016-06-06 18:55:15]

https://codereview.chromium.org/2037903002/diff/1/components/precache/core/pr...
File components/precache/core/precache_fetcher.cc (right):

https://codereview.chromium.org/2037903002/diff/1/components/precache/core/pr...
components/precache/core/precache_fetcher.cc:402: if
(manifest_urls_to_fetch_.empty() || !pool_.IsAvailable())
Could you add a test that tries to precache more resources and manifests than
could fit at once in the pool?

[bengr, 2016-06-07 21:14:40]

https://codereview.chromium.org/2037903002/diff/20001/components/precache/cor...
File components/precache/core/precache_fetcher_unittest.cc (right):

https://codereview.chromium.org/2037903002/diff/20001/components/precache/cor...
components/precache/core/precache_fetcher_unittest.cc:991: const size_t
kNumResources = 15;
This test should check the precondition that this exceeds the capacity of the
pool.

[Raj, 2016-06-08 03:10:42]

https://codereview.chromium.org/2037903002/diff/1/components/precache/core/pr...
File components/precache/core/precache_fetcher.cc (right):

https://codereview.chromium.org/2037903002/diff/1/components/precache/core/pr...
components/precache/core/precache_fetcher.cc:402: if
(manifest_urls_to_fetch_.empty() || !pool_.IsAvailable())
On 2016/06/06 18:55:15, bengr wrote:
> Could you add a test that tries to precache more resources and manifests than
> could fit at once in the pool?

Done.

https://codereview.chromium.org/2037903002/diff/20001/components/precache/cor...
File components/precache/core/precache_fetcher_unittest.cc (right):

https://codereview.chromium.org/2037903002/diff/20001/components/precache/cor...
components/precache/core/precache_fetcher_unittest.cc:991: const size_t
kNumResources = 15;
On 2016/06/07 21:14:40, bengr wrote:
> This test should check the precondition that this exceeds the capacity of the
> pool.

Done.
This test fails if the DCHECK was not removed.

https://cs.chromium.org/chromium/src/components/precache/core/precache_fetche...

[bengr, 2016-06-08 23:49:56]

https://codereview.chromium.org/2037903002/diff/20001/components/precache/cor...
File components/precache/core/precache_fetcher_unittest.cc (right):

https://codereview.chromium.org/2037903002/diff/20001/components/precache/cor...
components/precache/core/precache_fetcher_unittest.cc:991: const size_t
kNumResources = 15;
On 2016/06/08 03:10:42, Raj wrote:
> On 2016/06/07 21:14:40, bengr wrote:
> > This test should check the precondition that this exceeds the capacity of
the
> > pool.
> 
> Done.
> This test fails if the DCHECK was not removed.
> 
>
https://cs.chromium.org/chromium/src/components/precache/core/precache_fetche...

That's not what I mean. If someone changes the pool capacity to be greater than
20, this test would pass even if the DCHECK were added back in, so you should
verify that the number of resources/hosts you are using in the test is greater
than the pool size.

[Raj, 2016-06-09 00:32:46]

https://codereview.chromium.org/2037903002/diff/20001/components/precache/cor...
File components/precache/core/precache_fetcher_unittest.cc (right):

https://codereview.chromium.org/2037903002/diff/20001/components/precache/cor...
components/precache/core/precache_fetcher_unittest.cc:991: const size_t
kNumResources = 15;
On 2016/06/08 23:49:56, bengr wrote:
> On 2016/06/08 03:10:42, Raj wrote:
> > On 2016/06/07 21:14:40, bengr wrote:
> > > This test should check the precondition that this exceeds the capacity of
> the
> > > pool.
> > 
> > Done.
> > This test fails if the DCHECK was not removed.
> > 
> >
>
https://cs.chromium.org/chromium/src/components/precache/core/precache_fetche...
> 
> That's not what I mean. If someone changes the pool capacity to be greater
than
> 20, this test would pass even if the DCHECK were added back in, so you should
> verify that the number of resources/hosts you are using in the test is greater
> than the pool size.

Actually, this test will fail if pool capacity is increased to 20.
Since it checks for IsAvailable() to be false at some point during the fetches.

Though, I have added code to get the max pool size and using it for
kNumResources here.

[bengr, 2016-06-10 19:49:51]

https://codereview.chromium.org/2037903002/diff/60001/components/precache/cor...
File components/precache/core/precache_fetcher_unittest.cc (right):

https://codereview.chromium.org/2037903002/diff/60001/components/precache/cor...
components/precache/core/precache_fetcher_unittest.cc:1057:
precache_fetcher.Start();
I would change 1018 back to '= 15' and would make the pool capacity a member
method of PrecacheFetcher. Then here you'd do something like:
EXPECT_GT(kNumResources, precache_fetcher.GetMaximumParallelFetches());

size_t PrecacheFetcher::GetMaximumParallelFetches() const {
  return pool_.max_size();
}

[Raj, 2016-06-10 21:12:03]

Thanks for looking into.

https://codereview.chromium.org/2037903002/diff/60001/components/precache/cor...
File components/precache/core/precache_fetcher_unittest.cc (right):

https://codereview.chromium.org/2037903002/diff/60001/components/precache/cor...
components/precache/core/precache_fetcher_unittest.cc:1057:
precache_fetcher.Start();
On 2016/06/10 19:49:51, bengr wrote:
> I would change 1018 back to '= 15' and would make the pool capacity a member
> method of PrecacheFetcher. Then here you'd do something like:
> EXPECT_GT(kNumResources, precache_fetcher.GetMaximumParallelFetches());
> 
> size_t PrecacheFetcher::GetMaximumParallelFetches() const {
>   return pool_.max_size();
> }

Done.
Just added this test as a friend and accessing max_size_ directly.
Otherwise need to add two functions.
  GetMaximumParallelFetches() to PrecacheFetcher
  max_size() to FetcherPool

[bengr, 2016-06-10 22:10:59]

https://codereview.chromium.org/2037903002/diff/80001/components/precache/cor...
File components/precache/core/precache_fetcher_unittest.cc (right):

https://codereview.chromium.org/2037903002/diff/80001/components/precache/cor...
components/precache/core/precache_fetcher_unittest.cc:1059:
EXPECT_GT(kNumResources, precache_fetcher.pool_.max_size_);
Add a getter to the pool.

[Raj, 2016-06-10 22:23:03]

https://codereview.chromium.org/2037903002/diff/80001/components/precache/cor...
File components/precache/core/precache_fetcher_unittest.cc (right):

https://codereview.chromium.org/2037903002/diff/80001/components/precache/cor...
components/precache/core/precache_fetcher_unittest.cc:1059:
EXPECT_GT(kNumResources, precache_fetcher.pool_.max_size_);
On 2016/06/10 22:10:59, bengr wrote:
> Add a getter to the pool.

Done.

[bengr, 2016-06-12 00:42:21]

LGTM. Please fix the variable name.

https://codereview.chromium.org/2037903002/diff/120001/components/precache/co...
File components/precache/core/precache_fetcher_unittest.cc (right):

https://codereview.chromium.org/2037903002/diff/120001/components/precache/co...
components/precache/core/precache_fetcher_unittest.cc:71: }  // namespace
Why can't everything be in this anonymous namespace?

https://codereview.chromium.org/2037903002/diff/120001/components/precache/co...
components/precache/core/precache_fetcher_unittest.cc:380: bool
parallel_fetches_beyond_capacity;
nit: The name should end with a trailing "_"

[Raj, 2016-06-12 03:55:13]

https://codereview.chromium.org/2037903002/diff/120001/components/precache/co...
File components/precache/core/precache_fetcher_unittest.cc (right):

https://codereview.chromium.org/2037903002/diff/120001/components/precache/co...
components/precache/core/precache_fetcher_unittest.cc:71: }  // namespace
On 2016/06/12 00:42:20, bengr wrote:
> Why can't everything be in this anonymous namespace?

PrecacheFetcherTest class needs to be in nonanonymous namespace to be friend of
PrecacheFetcher.

https://codereview.chromium.org/2037903002/diff/120001/components/precache/co...
components/precache/core/precache_fetcher_unittest.cc:380: bool
parallel_fetches_beyond_capacity;
On 2016/06/12 00:42:20, bengr wrote:
> nit: The name should end with a trailing "_"

Done.

[Raj, 2016-06-12 03:55:22]

The CQ bit was checked by rajendrant@chromium.org

[Raj, 2016-06-12 03:55:22]

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

[commit-bot: I haz the power, 2016-06-12 03:55:29]

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

[commit-bot: I haz the power, 2016-06-12 04:45:01]

Committed patchset #8 (id:130001)

[commit-bot: I haz the power, 2016-06-12 04:45:04]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-12 04:47:37]

Description was changed from

==========
Precache manifest should not be added to fetcher pool which is full

Fetcher pool accepts limited number of requests which are fetched in
parallel. If the pool is full, precache manifest resources should not be
added to it.

BUG=606231
==========

to

==========
Precache manifest should not be added to fetcher pool which is full

Fetcher pool accepts limited number of requests which are fetched in
parallel. If the pool is full, precache manifest resources should not be
added to it.

BUG=606231

Committed: https://crrev.com/b2754f713e984d267353ce00d8a9e5ff6a6fafae
Cr-Commit-Position: refs/heads/master@{#399387}
==========

[commit-bot: I haz the power, 2016-06-12 04:47:38]

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