[Offline pages]: Implement background loader to save on last retry, and record last retry success U…

[Offline pages]: Implement background loader to save on last retry, and record last retry success UMA separately

BUG=705090
Review-Url: https://codereview.chromium.org/2818783002
Cr-Commit-Position: refs/heads/master@{#465088}
Committed: https://chromium.googlesource.com/chromium/src/+/3bcd8469054bf76be2008630904f4e2f9ba146e1

[chili, 2017-04-13 18:22:42]

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

[commit-bot: I haz the power, 2017-04-13 18:23:23]

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-13 18:45:16]

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

[commit-bot: I haz the power, 2017-04-13 18:45:16]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[chili, 2017-04-13 18:52:38]

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

[commit-bot: I haz the power, 2017-04-13 18:53:17]

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-13 19:53:14]

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

[commit-bot: I haz the power, 2017-04-13 19:53:15]

Dry run: This issue passed the CQ dry run.

[chili, 2017-04-13 20:39:54]

Patchset #1 (id:1) has been deleted

[chili, 2017-04-13 20:40:07]

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

[commit-bot: I haz the power, 2017-04-13 20:40:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chili, 2017-04-13 20:50:28]

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

[commit-bot: I haz the power, 2017-04-13 20:50:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chili, 2017-04-13 20:51:22]

chili@chromium.org changed reviewers:
+ fgorski@chromium.org, petewil@chromium.org, romax@chromium.org

[commit-bot: I haz the power, 2017-04-13 22:36:26]

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

[commit-bot: I haz the power, 2017-04-13 22:36:27]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Pete Williamson, 2017-04-13 22:57:15]

Looks pretty good, there are a few more test cases I'd like to see.

https://codereview.chromium.org/2818783002/diff/60001/chrome/browser/android/...
File chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc
(right):

https://codereview.chromium.org/2818783002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc:491:
TEST_F(BackgroundLoaderOfflinerTest, HandleTimeoutWithLowBarStartedTriesMet) {
Should we have a test where we get a last timeout, but the low bar is not met?

https://codereview.chromium.org/2818783002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc:508:
TEST_F(BackgroundLoaderOfflinerTest, HandleTimeoutWithStartedTriesMet) {
Should we test both started and finished attempt count?

https://codereview.chromium.org/2818783002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc:520:
TEST_F(BackgroundLoaderOfflinerTest, HandleTimeoutWithLowBar) {
Nit - this test name might be better as 
HandleTimeoutWithLowBarButNotAtRetryLimit

or something like that.

https://codereview.chromium.org/2818783002/diff/60001/chrome/browser/android/...
File chrome/browser/android/offline_pages/prerendering_offliner.cc (right):

https://codereview.chromium.org/2818783002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner.cc:161: if
(saved_on_last_retry_)
Should we clear this in the ctor just for completeness?

[romax, 2017-04-13 23:17:14]

mostly looks good and I had similar comments as petewil@.

https://codereview.chromium.org/2818783002/diff/60001/chrome/browser/android/...
File chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc
(right):

https://codereview.chromium.org/2818783002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc:508:
TEST_F(BackgroundLoaderOfflinerTest, HandleTimeoutWithStartedTriesMet) {
On 2017/04/13 22:57:15, Pete Williamson wrote:
> Should we test both started and finished attempt count?

I think the test is missing for the prerendering_offliner part as well (it was
my bad :( , can you please give it a quick fix if possible?

[fgorski, 2017-04-14 05:15:58]

fix event logger, please, on top of other comments.

https://codereview.chromium.org/2818783002/diff/60001/chrome/browser/android/...
File chrome/browser/android/offline_pages/background_loader_offliner.cc (right):

https://codereview.chromium.org/2818783002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner.cc:204:
(request.started_attempt_count() + 1 > policy_->GetMaxStartedTries() ||
I am wondering whether we can make all these checks outside of the offliner and
only pass information whether this is_last_retry_ or not. 

That way you have only 2 tests to write. I think we are spilling policy too much
that way.

I looked at HandleTimeout of PrerenderingOffliner and of course there is a minor
difference there, but the code is duplicated otherwise... telling me it belongs
outside of that call. Also, policy_ is used only in this method in case of the
other offliner...

bool PrerenderingOffliner::HandleTimeout(const SavePageRequest& request) {
  if (pending_request_) {
    DCHECK(request.request_id() == pending_request_->request_id());
    if (GetOrCreateLoader()->IsLowbarMet() &&
        (request.started_attempt_count() + 1 >= policy_->GetMaxStartedTries() ||
         request.completed_attempt_count() + 1 >=
             policy_->GetMaxCompletedTries())) {
      GetOrCreateLoader()->StartSnapshot();
      return true;
    }
  }
  return false;
}

https://codereview.chromium.org/2818783002/diff/60001/chrome/browser/android/...
File chrome/browser/android/offline_pages/background_loader_offliner.h (right):

https://codereview.chromium.org/2818783002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner.h:110: //
Whether the low bar has been met.
be more specific about the low bar, please... low bar of what?

https://codereview.chromium.org/2818783002/diff/60001/chrome/browser/android/...
File chrome/browser/android/offline_pages/prerendering_offliner.cc (right):

https://codereview.chromium.org/2818783002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner.cc:161: if
(saved_on_last_retry_)
On 2017/04/13 22:57:15, Pete Williamson wrote:
> Should we clear this in the ctor just for completeness?

If by clear you mean initialize: +1
... And Cathy knows how much debugging that takes to find.

https://codereview.chromium.org/2818783002/diff/60001/components/offline_page...
File components/offline_pages/core/background/request_coordinator.cc (right):

https://codereview.chromium.org/2818783002/diff/60001/components/offline_page...
components/offline_pages/core/background/request_coordinator.cc:1010: case
Offliner::RequestStatus::LOADING_FAILED_NO_NEXT:
Add line here, and go over all statuses, please,  as this looks like less then
what we have...
To prevent NOTREACHED() below

[chili, 2017-04-15 00:33:53]

Patchset #5 (id:100001) has been deleted

[chili, 2017-04-15 00:34:04]

Patchset #5 (id:120001) has been deleted

[chili, 2017-04-15 00:34:58]

chili@chromium.org changed reviewers:
+ holte@chromium.org

[chili, 2017-04-15 00:34:59]

+holte for histogram update

Thanks for the feedback!

https://codereview.chromium.org/2818783002/diff/60001/chrome/browser/android/...
File chrome/browser/android/offline_pages/background_loader_offliner.cc (right):

https://codereview.chromium.org/2818783002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner.cc:204:
(request.started_attempt_count() + 1 > policy_->GetMaxStartedTries() ||
On 2017/04/14 05:15:57, fgorski wrote:
> I am wondering whether we can make all these checks outside of the offliner
and
> only pass information whether this is_last_retry_ or not. 
> 
> That way you have only 2 tests to write. I think we are spilling policy too
much
> that way.
> 
> I looked at HandleTimeout of PrerenderingOffliner and of course there is a
minor
> difference there, but the code is duplicated otherwise... telling me it
belongs
> outside of that call. Also, policy_ is used only in this method in case of the
> other offliner...
> 
> bool PrerenderingOffliner::HandleTimeout(const SavePageRequest& request) {
>   if (pending_request_) {
>     DCHECK(request.request_id() == pending_request_->request_id());
>     if (GetOrCreateLoader()->IsLowbarMet() &&
>         (request.started_attempt_count() + 1 >= policy_->GetMaxStartedTries()
||
>          request.completed_attempt_count() + 1 >=
>              policy_->GetMaxCompletedTries())) {
>       GetOrCreateLoader()->StartSnapshot();
>       return true;
>     }
>   }
>   return false;
> }

I agree with this, but can we make the change in a separate CL? I filed
crbug/711842 to track.

The move will require updates in a number of unittests, and it might be better
to do that separately.

https://codereview.chromium.org/2818783002/diff/60001/chrome/browser/android/...
File chrome/browser/android/offline_pages/background_loader_offliner.h (right):

https://codereview.chromium.org/2818783002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner.h:110: //
Whether the low bar has been met.
On 2017/04/14 05:15:57, fgorski wrote:
> be more specific about the low bar, please... low bar of what?

Done.

https://codereview.chromium.org/2818783002/diff/60001/chrome/browser/android/...
File chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc
(right):

https://codereview.chromium.org/2818783002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc:491:
TEST_F(BackgroundLoaderOfflinerTest, HandleTimeoutWithLowBarStartedTriesMet) {
On 2017/04/13 22:57:15, Pete Williamson wrote:
> Should we have a test where we get a last timeout, but the low bar is not met?

That is the situation in HandleTimeoutWithStartedTriesMet

https://codereview.chromium.org/2818783002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc:508:
TEST_F(BackgroundLoaderOfflinerTest, HandleTimeoutWithStartedTriesMet) {
On 2017/04/13 22:57:15, Pete Williamson wrote:
> Should we test both started and finished attempt count?

Done.

https://codereview.chromium.org/2818783002/diff/60001/chrome/browser/android/...
File chrome/browser/android/offline_pages/prerendering_offliner.cc (right):

https://codereview.chromium.org/2818783002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner.cc:161: if
(saved_on_last_retry_)
On 2017/04/14 05:15:57, fgorski wrote:
> On 2017/04/13 22:57:15, Pete Williamson wrote:
> > Should we clear this in the ctor just for completeness?
> 
> If by clear you mean initialize: +1
> ... And Cathy knows how much debugging that takes to find.

Done.
This actually caused a test failure T___T

https://codereview.chromium.org/2818783002/diff/60001/components/offline_page...
File components/offline_pages/core/background/request_coordinator.cc (right):

https://codereview.chromium.org/2818783002/diff/60001/components/offline_page...
components/offline_pages/core/background/request_coordinator.cc:1010: case
Offliner::RequestStatus::LOADING_FAILED_NO_NEXT:
On 2017/04/14 05:15:57, fgorski wrote:
> Add line here, and go over all statuses, please,  as this looks like less then
> what we have...
> To prevent NOTREACHED() below

Done.

[chili, 2017-04-15 00:40:49]

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

[commit-bot: I haz the power, 2017-04-15 00:41:11]

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-15 01:53:23]

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

[commit-bot: I haz the power, 2017-04-15 01:53:23]

Dry run: This issue passed the CQ dry run.

[Pete Williamson, 2017-04-17 16:40:03]

https://codereview.chromium.org/2818783002/diff/80001/chrome/browser/android/...
File chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc
(right):

https://codereview.chromium.org/2818783002/diff/80001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc:538:
HandleTimeoutWithNoLowBarCompletedTriesMet) {
Looking at this test and the next test, I think that one has the low bar set and
one not set, but I don't see any code that treats these differently - did I miss
something?

https://codereview.chromium.org/2818783002/diff/80001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc:550:
TEST_F(BackgroundLoaderOfflinerTest, HandleTimeoutWithLowBar) {
It would be great if the name of this test indicated that the retry limit was
not hit.

[fgorski, 2017-04-17 17:48:59]

https://codereview.chromium.org/2818783002/diff/60001/chrome/browser/android/...
File chrome/browser/android/offline_pages/background_loader_offliner.cc (right):

https://codereview.chromium.org/2818783002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner.cc:204:
(request.started_attempt_count() + 1 > policy_->GetMaxStartedTries() ||
On 2017/04/15 00:34:58, chili wrote:
> On 2017/04/14 05:15:57, fgorski wrote:
> > I am wondering whether we can make all these checks outside of the offliner
> and
> > only pass information whether this is_last_retry_ or not. 
> > 
> > That way you have only 2 tests to write. I think we are spilling policy too
> much
> > that way.
> > 
> > I looked at HandleTimeout of PrerenderingOffliner and of course there is a
> minor
> > difference there, but the code is duplicated otherwise... telling me it
> belongs
> > outside of that call. Also, policy_ is used only in this method in case of
the
> > other offliner...
> > 
> > bool PrerenderingOffliner::HandleTimeout(const SavePageRequest& request) {
> >   if (pending_request_) {
> >     DCHECK(request.request_id() == pending_request_->request_id());
> >     if (GetOrCreateLoader()->IsLowbarMet() &&
> >         (request.started_attempt_count() + 1 >=
policy_->GetMaxStartedTries()
> ||
> >          request.completed_attempt_count() + 1 >=
> >              policy_->GetMaxCompletedTries())) {
> >       GetOrCreateLoader()->StartSnapshot();
> >       return true;
> >     }
> >   }
> >   return false;
> > }
> 
> I agree with this, but can we make the change in a separate CL? I filed
> crbug/711842 to track.
> 
> The move will require updates in a number of unittests, and it might be better
> to do that separately.

Acknowledged.

https://codereview.chromium.org/2818783002/diff/140001/chrome/browser/android...
File chrome/browser/android/offline_pages/prerendering_offliner.cc (right):

https://codereview.chromium.org/2818783002/diff/140001/chrome/browser/android...
chrome/browser/android/offline_pages/prerendering_offliner.cc:171:
saved_on_last_retry_ = false;
Would a single prerendering offliner be used for multiple snapshots? If so is
this covering all cases when this field has to be reset to false?

Otherwise, in case it does not take multiple snapshots, why bother resetting the
value?

https://codereview.chromium.org/2818783002/diff/140001/components/offline_pag...
File components/offline_pages/core/background/request_coordinator.cc (right):

https://codereview.chromium.org/2818783002/diff/140001/components/offline_pag...
components/offline_pages/core/background/request_coordinator.cc:1007: case
Offliner::RequestStatus::SAVED_ON_LAST_RETRY:
Let's bind this to a check for immediate vs. scheduled processing, to not retry
if we are timed out on a background processing window.

Additionally this status should go there: REQUEST_COORDINATOR_TIMED_OUT

[chili, 2017-04-17 19:37:52]

https://codereview.chromium.org/2818783002/diff/80001/chrome/browser/android/...
File chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc
(right):

https://codereview.chromium.org/2818783002/diff/80001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc:538:
HandleTimeoutWithNoLowBarCompletedTriesMet) {
On 2017/04/17 16:40:03, Pete Williamson wrote:
> Looking at this test and the next test, I think that one has the low bar set
and
> one not set, but I don't see any code that treats these differently - did I
miss
> something?

Good catch. Forgot to set the lowbar

https://codereview.chromium.org/2818783002/diff/80001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc:550:
TEST_F(BackgroundLoaderOfflinerTest, HandleTimeoutWithLowBar) {
On 2017/04/17 16:40:03, Pete Williamson wrote:
> It would be great if the name of this test indicated that the retry limit was
> not hit.

The name is updated in newest patch. You are looking at a slightly older one?

https://codereview.chromium.org/2818783002/diff/140001/chrome/browser/android...
File chrome/browser/android/offline_pages/prerendering_offliner.cc (right):

https://codereview.chromium.org/2818783002/diff/140001/chrome/browser/android...
chrome/browser/android/offline_pages/prerendering_offliner.cc:171:
saved_on_last_retry_ = false;
On 2017/04/17 17:48:59, fgorski wrote:
> Would a single prerendering offliner be used for multiple snapshots? If so is
> this covering all cases when this field has to be reset to false?
> 
> Otherwise, in case it does not take multiple snapshots, why bother resetting
the
> value?

Good point. Added an additional clearing when we first LoadAndSave a page. The
PrerenderingOffliner is used across multiple snapshots.

https://codereview.chromium.org/2818783002/diff/140001/components/offline_pag...
File components/offline_pages/core/background/request_coordinator.cc (right):

https://codereview.chromium.org/2818783002/diff/140001/components/offline_pag...
components/offline_pages/core/background/request_coordinator.cc:1007: case
Offliner::RequestStatus::SAVED_ON_LAST_RETRY:
On 2017/04/17 17:48:59, fgorski wrote:
> Let's bind this to a check for immediate vs. scheduled processing, to not
retry
> if we are timed out on a background processing window.
> 
> Additionally this status should go there: REQUEST_COORDINATOR_TIMED_OUT

done.

I remember we talked about some additional check if the processing window was
IMMEDIATE, but can't remember what it was....

[chili, 2017-04-17 19:38:12]

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

[commit-bot: I haz the power, 2017-04-17 19:38:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chili, 2017-04-17 19:51:15]

Patchset #6 (id:160001) has been deleted

[fgorski, 2017-04-17 19:51:36]

lgtm

As for immediate window checks I don't think there was anything you missed here.

[chili, 2017-04-17 19:51:37]

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

[commit-bot: I haz the power, 2017-04-17 19:52:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Pete Williamson, 2017-04-17 20:27:44]

lgtm

[commit-bot: I haz the power, 2017-04-17 21:03:25]

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

[commit-bot: I haz the power, 2017-04-17 21:03:26]

Dry run: This issue passed the CQ dry run.

[Steven Holte, 2017-04-17 22:15:03]

lgtm

[chili, 2017-04-18 00:57:10]

The CQ bit was checked by chili@chromium.org

[commit-bot: I haz the power, 2017-04-18 00:57:36]

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-18 01:04:40]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1492477030360880,
"parent_rev": "60c4a2e6afafac1ddacf83ca4c4fd75d6ee1bc15", "commit_rev":
"3bcd8469054bf76be2008630904f4e2f9ba146e1"}

[commit-bot: I haz the power, 2017-04-18 01:05:16]

Description was changed from

==========
[Offline pages]: Implement background loader to save on last retry, and record
last retry success UMA separately

BUG=705090
==========

to

==========
[Offline pages]: Implement background loader to save on last retry, and record
last retry success UMA separately

BUG=705090

Review-Url: https://codereview.chromium.org/2818783002
Cr-Commit-Position: refs/heads/master@{#465088}
Committed:
https://chromium.googlesource.com/chromium/src/+/3bcd8469054bf76be2008630904f...
==========

[commit-bot: I haz the power, 2017-04-18 01:05:18]

Committed patchset #6 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/3bcd8469054bf76be2008630904f...