Precache should cancel when there is user traffic

Precache should cancel when there is user traffic

When any user initiated traffic is seen during the precache fetches,
precache should be cancelled.

BUG=619221
Committed: https://crrev.com/1e1eff17348e9dca71bf17081b0e4cc6c2212d3b
Cr-Commit-Position: refs/heads/master@{#404474}

[Raj, 2016-06-14 21:28:30]

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

[Raj, 2016-06-14 21:28:31]

mmenke: ptal chrome_network_delegate.cc
bengr: all files :)

[Raj, 2016-06-14 21:35:17]

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

https://codereview.chromium.org/2061303003/diff/20001/components/precache/cor...
components/precache/core/precache_fetcher.cc:299: if
(remaining_resource_urls_to_fetch > 0 && manifests_completed > 0)
This additional check fixes a possible DCHECK failure below at line#302

[mmenke, 2016-06-15 15:25:45]

On 2016/06/14 21:35:17, Raj wrote:
>
https://codereview.chromium.org/2061303003/diff/20001/components/precache/cor...
> File components/precache/core/precache_fetcher.cc (right):
> 
>
https://codereview.chromium.org/2061303003/diff/20001/components/precache/cor...
> components/precache/core/precache_fetcher.cc:299: if
> (remaining_resource_urls_to_fetch > 0 && manifests_completed > 0)
> This additional check fixes a possible DCHECK failure below at line#302

May take me a little while to respond to this - the layering here seems weird,
so either need to suggest something better, or resign myself ot it.

[tbansal1, 2016-06-15 22:48:16]

On 2016/06/15 15:25:45, mmenke wrote:
> On 2016/06/14 21:35:17, Raj wrote:
> >
>
https://codereview.chromium.org/2061303003/diff/20001/components/precache/cor...
> > File components/precache/core/precache_fetcher.cc (right):
> > 
> >
>
https://codereview.chromium.org/2061303003/diff/20001/components/precache/cor...
> > components/precache/core/precache_fetcher.cc:299: if
> > (remaining_resource_urls_to_fetch > 0 && manifests_completed > 0)
> > This additional check fixes a possible DCHECK failure below at line#302
> 
> May take me a little while to respond to this - the layering here seems weird,
> so either need to suggest something better, or resign myself ot it.

The right way (probably TODO for longer-term) might be to create a profile
specific precache object, and have that object register as a network delegate
somewhere in profile_io_data or profile_impl.

[tbansal1, 2016-06-15 22:48:42]

tbansal@chromium.org changed reviewers:
+ tbansal@chromium.org

[tbansal1, 2016-06-15 22:48:43]

drive-by-comment

https://codereview.chromium.org/2061303003/diff/40001/components/data_use_mea...
File components/data_use_measurement/content/data_use_measurement.cc (right):

https://codereview.chromium.org/2061303003/diff/40001/components/data_use_mea...
components/data_use_measurement/content/data_use_measurement.cc:108: bool
DataUseMeasurement::IsUserTraffic(const net::URLRequest* request) {
nit: This is a static function. So, you need to add static as a comment. See
https://cs.chromium.org/search/?q=f:cc+f:net+%22//+static%22&sq=package:chrom...

[bengr, 2016-06-20 20:21:14]

I agree with mmenke that the layering is a little weird, but don't see an
obviously better approach.

https://codereview.chromium.org/2061303003/diff/40001/chrome/browser/precache...
File chrome/browser/precache/precache_util.cc (right):

https://codereview.chromium.org/2061303003/diff/40001/chrome/browser/precache...
chrome/browser/precache/precache_util.cc:17: void
PrecacheUtil::UpdatePrecacheMetricsAndState(const net::URLRequest* request,
There should be unit tests for these methods.

https://codereview.chromium.org/2061303003/diff/40001/chrome/browser/precache...
chrome/browser/precache/precache_util.cc:37: void
PrecacheUtil::UpdatePrecacheMetricsAndStateOnUIThread(
Can this method be in an anonymous namespace in this file?

https://codereview.chromium.org/2061303003/diff/40001/chrome/browser/precache...
File chrome/browser/precache/precache_util.h (right):

https://codereview.chromium.org/2061303003/diff/40001/chrome/browser/precache...
chrome/browser/precache/precache_util.h:14: class PrecacheUtil {
Why are these in a class?

https://codereview.chromium.org/2061303003/diff/40001/components/precache/con...
File components/precache/content/precache_manager.h (right):

https://codereview.chromium.org/2061303003/diff/40001/components/precache/con...
components/precache/content/precache_manager.h:108: base::TimeDelta latency,
Forward declare. Also shouldn't this be a const base::TimeDelta&?

[mmenke, 2016-06-21 15:54:35]

chrome/browser/net LGTM

https://codereview.chromium.org/2061303003/diff/40001/components/data_use_mea...
File components/data_use_measurement/content/data_use_measurement.cc (right):

https://codereview.chromium.org/2061303003/diff/40001/components/data_use_mea...
components/data_use_measurement/content/data_use_measurement.cc:108: bool
DataUseMeasurement::IsUserTraffic(const net::URLRequest* request) {
On 2016/06/15 22:48:43, tbansal1 wrote:
> nit: This is a static function. So, you need to add static as a comment. See
>
https://cs.chromium.org/search/?q=f:cc+f:net+%22//+static%22&sq=package:chrom...

The Google style guide no longer mentions doing this, so I no longer call out
this, either way, unless we're inconsistent within a file.  Not saying one way
is better than the other, just mentioning it.

[Raj, 2016-06-30 06:03:08]

bengr: ptal

https://codereview.chromium.org/2061303003/diff/40001/chrome/browser/precache...
File chrome/browser/precache/precache_util.cc (right):

https://codereview.chromium.org/2061303003/diff/40001/chrome/browser/precache...
chrome/browser/precache/precache_util.cc:17: void
PrecacheUtil::UpdatePrecacheMetricsAndState(const net::URLRequest* request,
On 2016/06/20 20:21:14, bengr wrote:
> There should be unit tests for these methods.

I am trying to add tests. But looks little complicated to create a test profile
and test this.
BrowserTests have a way of testing profiles. But I believe, browsertests are not
supported in android. And testing this precache code in non-android does not
make sense.
I could add unittest for this separately from this CL. WDYT.

The functions used by this api have unittests, and this function is just a
wrapper.
 * RecordStatsForFetch() in precache_manager_unittest.cc
 * IsUserTraffic() in data_use_measurement_unittest.cc

https://codereview.chromium.org/2061303003/diff/40001/chrome/browser/precache...
chrome/browser/precache/precache_util.cc:37: void
PrecacheUtil::UpdatePrecacheMetricsAndStateOnUIThread(
On 2016/06/20 20:21:14, bengr wrote:
> Can this method be in an anonymous namespace in this file?

Done.

https://codereview.chromium.org/2061303003/diff/40001/chrome/browser/precache...
File chrome/browser/precache/precache_util.h (right):

https://codereview.chromium.org/2061303003/diff/40001/chrome/browser/precache...
chrome/browser/precache/precache_util.h:14: class PrecacheUtil {
On 2016/06/20 20:21:14, bengr wrote:
> Why are these in a class?

Removed the class.

https://codereview.chromium.org/2061303003/diff/40001/components/data_use_mea...
File components/data_use_measurement/content/data_use_measurement.cc (right):

https://codereview.chromium.org/2061303003/diff/40001/components/data_use_mea...
components/data_use_measurement/content/data_use_measurement.cc:108: bool
DataUseMeasurement::IsUserTraffic(const net::URLRequest* request) {
On 2016/06/21 15:54:35, mmenke wrote:
> On 2016/06/15 22:48:43, tbansal1 wrote:
> > nit: This is a static function. So, you need to add static as a comment. See
> >
>
https://cs.chromium.org/search/?q=f:cc+f:net+%22//+static%22&sq=package:chrom...
> 
> The Google style guide no longer mentions doing this, so I no longer call out
> this, either way, unless we're inconsistent within a file.  Not saying one way
> is better than the other, just mentioning it.

Done.

https://codereview.chromium.org/2061303003/diff/40001/components/precache/con...
File components/precache/content/precache_manager.h (right):

https://codereview.chromium.org/2061303003/diff/40001/components/precache/con...
components/precache/content/precache_manager.h:108: base::TimeDelta latency,
On 2016/06/20 20:21:14, bengr wrote:
> Forward declare. Also shouldn't this be a const base::TimeDelta&?

Done.

[bengr, 2016-07-06 17:04:28]

https://codereview.chromium.org/2061303003/diff/60001/chrome/browser/precache...
File chrome/browser/precache/precache_util.cc (right):

https://codereview.chromium.org/2061303003/diff/60001/chrome/browser/precache...
chrome/browser/precache/precache_util.cc:46: void
UpdatePrecacheMetricsAndState(const net::URLRequest* request,
#include url_request.h

https://codereview.chromium.org/2061303003/diff/60001/chrome/browser/precache...
chrome/browser/precache/precache_util.cc:47: void* profile_id) {
You should probably DCHECK_CURRENTLY_ON(content::BrowserThread::IO)

https://codereview.chromium.org/2061303003/diff/60001/chrome/browser/precache...
chrome/browser/precache/precache_util.cc:52: base::TimeDelta latency =
base::TimeTicks::Now() - request->creation_time();
#include time.h

https://codereview.chromium.org/2061303003/diff/60001/chrome/browser/precache...
chrome/browser/precache/precache_util.cc:60: GURL(request->referrer()), latency,
base::Time::Now(),
#include gurl.h

https://codereview.chromium.org/2061303003/diff/60001/chrome/browser/precache...
File chrome/browser/precache/precache_util.h (right):

https://codereview.chromium.org/2061303003/diff/60001/chrome/browser/precache...
chrome/browser/precache/precache_util.h:1: // Copyright (c) 2016 The Chromium
Authors. All rights reserved.
Current convention is to omit the "(c)"

https://codereview.chromium.org/2061303003/diff/60001/chrome/browser/precache...
chrome/browser/precache/precache_util.h:8: #include "base/time/time.h"
I don't think you need this here.

https://codereview.chromium.org/2061303003/diff/60001/chrome/browser/precache...
chrome/browser/precache/precache_util.h:9: #include
"net/url_request/url_request.h"
You should be able to forward declare this instead.

https://codereview.chromium.org/2061303003/diff/60001/chrome/browser/precache...
chrome/browser/precache/precache_util.h:10: #include "url/gurl.h"
I don't think you need this here.

https://codereview.chromium.org/2061303003/diff/60001/components/data_use_mea...
File components/data_use_measurement/content/data_use_measurement.cc (right):

https://codereview.chromium.org/2061303003/diff/60001/components/data_use_mea...
components/data_use_measurement/content/data_use_measurement.cc:72: bool
is_user_traffic = IsUserTraffic(request);
is_user_initiated_request, if you change the name of the method.

https://codereview.chromium.org/2061303003/diff/60001/components/data_use_mea...
components/data_use_measurement/content/data_use_measurement.cc:115: // with
upcoming changes in PlzNavigate.
This should be verified. File a bug and reference it in a TODO.

https://codereview.chromium.org/2061303003/diff/60001/components/data_use_mea...
File components/data_use_measurement/content/data_use_measurement.h (right):

https://codereview.chromium.org/2061303003/diff/60001/components/data_use_mea...
components/data_use_measurement/content/data_use_measurement.h:45: static bool
IsUserTraffic(const net::URLRequest* request);
A better name for this is probably IsUserInitiatedRequest

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

https://codereview.chromium.org/2061303003/diff/60001/components/precache/cor...
components/precache/core/precache_fetcher.cc:303: if
(remaining_resource_urls_to_fetch > 0 && manifests_completed > 0)
This seems like it should be a DCHECK. Could you explain why it is ok for this
method to be called when manifests_completed <= 0?

[Raj, 2016-07-06 18:28:23]

ptal

https://codereview.chromium.org/2061303003/diff/60001/chrome/browser/precache...
File chrome/browser/precache/precache_util.cc (right):

https://codereview.chromium.org/2061303003/diff/60001/chrome/browser/precache...
chrome/browser/precache/precache_util.cc:46: void
UpdatePrecacheMetricsAndState(const net::URLRequest* request,
On 2016/07/06 17:04:27, bengr wrote:
> #include url_request.h

Done.

https://codereview.chromium.org/2061303003/diff/60001/chrome/browser/precache...
chrome/browser/precache/precache_util.cc:47: void* profile_id) {
On 2016/07/06 17:04:27, bengr wrote:
> You should probably DCHECK_CURRENTLY_ON(content::BrowserThread::IO)

Done.

https://codereview.chromium.org/2061303003/diff/60001/chrome/browser/precache...
chrome/browser/precache/precache_util.cc:52: base::TimeDelta latency =
base::TimeTicks::Now() - request->creation_time();
On 2016/07/06 17:04:27, bengr wrote:
> #include time.h

Done.

https://codereview.chromium.org/2061303003/diff/60001/chrome/browser/precache...
chrome/browser/precache/precache_util.cc:60: GURL(request->referrer()), latency,
base::Time::Now(),
On 2016/07/06 17:04:27, bengr wrote:
> #include gurl.h

Done.

https://codereview.chromium.org/2061303003/diff/60001/chrome/browser/precache...
File chrome/browser/precache/precache_util.h (right):

https://codereview.chromium.org/2061303003/diff/60001/chrome/browser/precache...
chrome/browser/precache/precache_util.h:1: // Copyright (c) 2016 The Chromium
Authors. All rights reserved.
On 2016/07/06 17:04:27, bengr wrote:
> Current convention is to omit the "(c)"

Done.

https://codereview.chromium.org/2061303003/diff/60001/chrome/browser/precache...
chrome/browser/precache/precache_util.h:8: #include "base/time/time.h"
On 2016/07/06 17:04:27, bengr wrote:
> I don't think you need this here.

Done.

https://codereview.chromium.org/2061303003/diff/60001/chrome/browser/precache...
chrome/browser/precache/precache_util.h:9: #include
"net/url_request/url_request.h"
On 2016/07/06 17:04:27, bengr wrote:
> You should be able to forward declare this instead.

Done.

https://codereview.chromium.org/2061303003/diff/60001/chrome/browser/precache...
chrome/browser/precache/precache_util.h:10: #include "url/gurl.h"
On 2016/07/06 17:04:27, bengr wrote:
> I don't think you need this here.

Done.

https://codereview.chromium.org/2061303003/diff/60001/components/data_use_mea...
File components/data_use_measurement/content/data_use_measurement.cc (right):

https://codereview.chromium.org/2061303003/diff/60001/components/data_use_mea...
components/data_use_measurement/content/data_use_measurement.cc:72: bool
is_user_traffic = IsUserTraffic(request);
On 2016/07/06 17:04:27, bengr wrote:
> is_user_initiated_request, if you change the name of the method.

Done.

https://codereview.chromium.org/2061303003/diff/60001/components/data_use_mea...
components/data_use_measurement/content/data_use_measurement.cc:115: // with
upcoming changes in PlzNavigate.
On 2016/07/06 17:04:27, bengr wrote:
> This should be verified. File a bug and reference it in a TODO.

Filed bug. Pls add any usecases this needs to be verified.
https://bugs.chromium.org/p/chromium/issues/detail?id=626063

https://codereview.chromium.org/2061303003/diff/60001/components/data_use_mea...
File components/data_use_measurement/content/data_use_measurement.h (right):

https://codereview.chromium.org/2061303003/diff/60001/components/data_use_mea...
components/data_use_measurement/content/data_use_measurement.h:45: static bool
IsUserTraffic(const net::URLRequest* request);
On 2016/07/06 17:04:27, bengr wrote:
> A better name for this is probably IsUserInitiatedRequest

Done.

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

https://codereview.chromium.org/2061303003/diff/60001/components/precache/cor...
components/precache/core/precache_fetcher.cc:303: if
(remaining_resource_urls_to_fetch > 0 && manifests_completed > 0)
On 2016/07/06 17:04:27, bengr wrote:
> This seems like it should be a DCHECK. Could you explain why it is ok for this
> method to be called when manifests_completed <= 0?

hmm. I got this failure in a few of my tests.
Now it is less apparent to me why this would be the case.
I removed this check. Will fix it when I hit such issue again.

https://codereview.chromium.org/2061303003/diff/80001/components/precache/con...
File components/precache/content/precache_manager_unittest.cc (right):

https://codereview.chromium.org/2061303003/diff/80001/components/precache/con...
components/precache/content/precache_manager_unittest.cc:150: }  // namespace
Moving PrecacheManagerTest out of anonymous namespace for the
FRIEND_TEST_ALL_PREFIXES declarations in precache_manager.h to take effect.

[bengr, 2016-07-08 16:54:23]

lgtm, but please address these nits.

https://codereview.chromium.org/2061303003/diff/40001/chrome/browser/precache...
File chrome/browser/precache/precache_util.cc (right):

https://codereview.chromium.org/2061303003/diff/40001/chrome/browser/precache...
chrome/browser/precache/precache_util.cc:17: void
PrecacheUtil::UpdatePrecacheMetricsAndState(const net::URLRequest* request,
On 2016/06/30 06:03:08, Raj wrote:
> On 2016/06/20 20:21:14, bengr wrote:
> > There should be unit tests for these methods.
> 
> I am trying to add tests. But looks little complicated to create a test
profile
> and test this.
> BrowserTests have a way of testing profiles. But I believe, browsertests are
not
> supported in android. And testing this precache code in non-android does not
> make sense.
> I could add unittest for this separately from this CL. WDYT.
> 
> The functions used by this api have unittests, and this function is just a
> wrapper.
>  * RecordStatsForFetch() in precache_manager_unittest.cc
>  * IsUserTraffic() in data_use_measurement_unittest.cc

If the functions this uses have unit tests, then just add a TODO to add more
test coverage.

https://codereview.chromium.org/2061303003/diff/80001/components/data_use_mea...
File components/data_use_measurement/content/data_use_measurement.cc (right):

https://codereview.chromium.org/2061303003/diff/80001/components/data_use_mea...
components/data_use_measurement/content/data_use_measurement.cc:117: //
TODO(626063): Verify this condition for different use cases.
// TODO(rajendrant): Verify this condition for different use cases. See
crbug.com/626063.

[Raj, 2016-07-08 20:05:50]

https://codereview.chromium.org/2061303003/diff/40001/chrome/browser/precache...
File chrome/browser/precache/precache_util.cc (right):

https://codereview.chromium.org/2061303003/diff/40001/chrome/browser/precache...
chrome/browser/precache/precache_util.cc:17: void
PrecacheUtil::UpdatePrecacheMetricsAndState(const net::URLRequest* request,
On 2016/07/08 16:54:22, bengr wrote:
> On 2016/06/30 06:03:08, Raj wrote:
> > On 2016/06/20 20:21:14, bengr wrote:
> > > There should be unit tests for these methods.
> > 
> > I am trying to add tests. But looks little complicated to create a test
> profile
> > and test this.
> > BrowserTests have a way of testing profiles. But I believe, browsertests are
> not
> > supported in android. And testing this precache code in non-android does not
> > make sense.
> > I could add unittest for this separately from this CL. WDYT.
> > 
> > The functions used by this api have unittests, and this function is just a
> > wrapper.
> >  * RecordStatsForFetch() in precache_manager_unittest.cc
> >  * IsUserTraffic() in data_use_measurement_unittest.cc
> 
> If the functions this uses have unit tests, then just add a TODO to add more
> test coverage.

Done.

https://codereview.chromium.org/2061303003/diff/80001/components/data_use_mea...
File components/data_use_measurement/content/data_use_measurement.cc (right):

https://codereview.chromium.org/2061303003/diff/80001/components/data_use_mea...
components/data_use_measurement/content/data_use_measurement.cc:117: //
TODO(626063): Verify this condition for different use cases.
On 2016/07/08 16:54:23, bengr wrote:
> // TODO(rajendrant): Verify this condition for different use cases. See
> crbug.com/626063.

Done.

[Raj, 2016-07-08 20:05:55]

The CQ bit was checked by rajendrant@chromium.org

[Raj, 2016-07-08 20:05:55]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org,
bengr@chromium.org
Link to the patchset: https://codereview.chromium.org/2061303003/#ps100001
(title: "Addressed nits")

[commit-bot: I haz the power, 2016-07-08 20:06:34]

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, 2016-07-08 20:11:28]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-07-08 20:11:33]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-08 20:14:01]

Description was changed from

==========
Precache should cancel when there is user traffic

When any user initiated traffic is seen during the precache fetches,
precache should be cancelled.

BUG=619221
==========

to

==========
Precache should cancel when there is user traffic

When any user initiated traffic is seen during the precache fetches,
precache should be cancelled.

BUG=619221

Committed: https://crrev.com/1e1eff17348e9dca71bf17081b0e4cc6c2212d3b
Cr-Commit-Position: refs/heads/master@{#404474}
==========

[commit-bot: I haz the power, 2016-07-08 20:14:02]

Patchset 6 (id:??) landed as
https://crrev.com/1e1eff17348e9dca71bf17081b0e4cc6c2212d3b
Cr-Commit-Position: refs/heads/master@{#404474}