Re-add resource speculative prefetching code

Re-add resource speculative prefetching code

This CL fixes the memory leak bug in CL 462423004.
See https://codereview.chromium.org/462423004/ for previous code reviews.

-------------------------
 Original CL description
-------------------------

Revert CL 117933003. Re-add speculative resource prefetching code.
https://codereview.chromium.org/117933003/

The speculative resource prefetching code was experimental code developed by
shishir@. He found that it has little improvement on desktop Chrome (win). We
though this should be beneficial to mobile browsers. After discussing with
tburkard@ and kenjibaheux@, we decided to bring the code back and do more
analysis on mobile devices. Reverting the patchset to re-add the code is the
first step.

The following design doc has discussed all related approaches and action items.
https://docs.google.com/a/google.com/document/d/1ie3hu-zNNXvmTXm3aJAtKUGOh6nZfbNjA0aZE1bzzIg/edit?usp=sharing

BUG=408399, 405690
Committed: https://crrev.com/9f71822da5a865c09675be7a7ac37f5089317fec
Cr-Commit-Position: refs/heads/master@{#295626}

[Zhen Wang, 2014-09-18 14:37:01]

zhenw@chromium.org changed reviewers:
+ thestig@chromium.org

[Zhen Wang, 2014-09-18 14:37:01]

Hi Lei,

This CL fixes the memory leak bug in CL 462423004. 
https://codereview.chromium.org/462423004/

We actually discussed the pointer deletion before, but I made a wrong
conclusion. See patch set 2 for the fix. 

In the real code, the requests will be passed to
ResourcePrefetcherManager::ResourcePrefetcherFinished() and eventually be passed
to and managed by ResourcePrefetchPredictor::Result::Result.

In the test, it will call to the mock method
TestResourcePrefetcherDelegate::ResourcePrefetcherFinished(), in which case, the
pointer is not properly managed. So we need to delete it in the test.

[Lei Zhang, 2014-09-18 17:06:40]

On 2014/09/18 14:37:01, Zhen Wang wrote:
> Hi Lei,
> 
> This CL fixes the memory leak bug in CL 462423004. 
> https://codereview.chromium.org/462423004/
> 
> We actually discussed the pointer deletion before, but I made a wrong
> conclusion. See patch set 2 for the fix. 
> 
> In the real code, the requests will be passed to
> ResourcePrefetcherManager::ResourcePrefetcherFinished() and eventually be
passed
> to and managed by ResourcePrefetchPredictor::Result::Result.
> 
> In the test, it will call to the mock method
> TestResourcePrefetcherDelegate::ResourcePrefetcherFinished(), in which case,
the
> pointer is not properly managed. So we need to delete it in the test.

Can you try changing ResourcePrefetcherFinished() to take a
scoped_ptr<ResourcePrefetcher::RequestVector> instead of a raw pointer and see
if that fixes the memory management? If we are using a scoped_ptr, we shouldn't
need to delete its pointer unless scoped_ptr::release() got called.

[Zhen Wang, 2014-09-18 18:54:09]

> Can you try changing ResourcePrefetcherFinished() to take a
> scoped_ptr<ResourcePrefetcher::RequestVector> instead of a raw pointer and see
> if that fixes the memory management? If we are using a scoped_ptr, we
shouldn't
> need to delete its pointer unless scoped_ptr::release() got called.

Can we declare mock method with smart pointer argument? I tried to use the smart

pointer as the argument and then declare the mock method with smart pointer
argument, too:

MOCK_METHOD2(ResourcePrefetcherFinished,
               void(ResourcePrefetcher* prefetcher,
                    scoped_ptr<ResourcePrefetcher::RequestVector> requests));

I get the following error:

../../chrome/browser/predictors/resource_prefetcher_unittest.cc:61:3: error:
calling a private constructor of class
'scoped_ptr<ScopedVector<predictors::ResourcePrefetcher::Request>,
base::DefaultDeleter<ScopedVector<predictors::ResourcePrefetcher::Request> > >'
  MOCK_METHOD2(ResourcePrefetcherFinished,

This looks like a smart pointer issue. I checked gmock doc. The matcher can
handler
smart pointer. But I didn't see any description about using MOCK_METHODn() with
smart
pointers. Do you have any pointers to any doc?

[Lei Zhang, 2014-09-18 19:36:05]

On 2014/09/18 18:54:09, Zhen Wang wrote:
> > Can you try changing ResourcePrefetcherFinished() to take a
> > scoped_ptr<ResourcePrefetcher::RequestVector> instead of a raw pointer and
see
> > if that fixes the memory management? If we are using a scoped_ptr, we
> shouldn't
> > need to delete its pointer unless scoped_ptr::release() got called.
> 
> Can we declare mock method with smart pointer argument? I tried to use the
smart
> 
> pointer as the argument and then declare the mock method with smart pointer
> argument, too:
> 
> MOCK_METHOD2(ResourcePrefetcherFinished,
>                void(ResourcePrefetcher* prefetcher,
>                     scoped_ptr<ResourcePrefetcher::RequestVector> requests));
> 
> I get the following error:
> 
> ../../chrome/browser/predictors/resource_prefetcher_unittest.cc:61:3: error:
> calling a private constructor of class
> 'scoped_ptr<ScopedVector<predictors::ResourcePrefetcher::Request>,
> base::DefaultDeleter<ScopedVector<predictors::ResourcePrefetcher::Request> >
>'
>   MOCK_METHOD2(ResourcePrefetcherFinished,
> 
> This looks like a smart pointer issue. I checked gmock doc. The matcher can
> handler
> smart pointer. But I didn't see any description about using MOCK_METHODn()
with
> smart
> pointers. Do you have any pointers to any doc?

Wild stab: scoped_ptr is a template class. Try MOCK_METHODn_T? If that doesn't
work and we have to use patch set 2, then please add a comment to explain what's
happening. Otherwise it just looks wrong and future readers will get confused.

[Zhen Wang, 2014-09-18 23:41:22]

> Wild stab: scoped_ptr is a template class. Try MOCK_METHODn_T? If that doesn't
> work and we have to use patch set 2, then please add a comment to explain
what's
> happening. Otherwise it just looks wrong and future readers will get confused.

I tried MOCK_METHODn_T. Still the same error. So I add the comments. ptal.

[Lei Zhang, 2014-09-18 23:44:09]

lgtm

[Zhen Wang, 2014-09-19 00:09:29]

The CQ bit was checked by zhenw@chromium.org

[commit-bot: I haz the power, 2014-09-19 00:11:14]

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

[commit-bot: I haz the power, 2014-09-19 00:41:18]

Committed patchset #4 (id:60001) as c78b9ec100ab0ef932a3b8c6403f2bb4c335285e

[commit-bot: I haz the power, 2014-09-19 00:41:55]

Patchset 4 (id:??) landed as
https://crrev.com/9f71822da5a865c09675be7a7ac37f5089317fec
Cr-Commit-Position: refs/heads/master@{#295626}