[OfflinePages] Improve visiblity/handling of "Loading not started" case

[OfflinePages] Improve visiblity/handling of "Loading not started" case
The IsPrerenderingPossible() no longer is applicable to the "offline"
origin prerender requests (it is now more tied to NoStatePrefetch
setting) so this stops prematurely checking it (via CanPrerender())
and instead passes request to PrerenderManager to let is decide whether
it accepts the "offline" origin request or not.

Also adds new Offliner RequestStatus code LOADING_NOT_ACCEPTED to
add clarity between the RC not trying to start the request vs. the
Offliner not accepting it.

BUG=677189
Committed: https://crrev.com/9033511eba1ce7c9b06a4372df9030ccd177353b
Cr-Commit-Position: refs/heads/master@{#441573}

[dougarnett, 2016-12-28 20:28:44]

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

[commit-bot: I haz the power, 2016-12-28 20:28:55]

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, 2016-12-28 20:52:38]

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

[commit-bot: I haz the power, 2016-12-28 20:52:39]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[dougarnett, 2016-12-29 16:51:02]

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

[commit-bot: I haz the power, 2016-12-29 16:51:15]

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, 2016-12-29 17:39:44]

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

[commit-bot: I haz the power, 2016-12-29 17:39:45]

Dry run: This issue passed the CQ dry run.

[dougarnett, 2016-12-29 17:42:25]

dougarnett@chromium.org changed reviewers:
+ petewil@chromium.org

[dougarnett, 2016-12-29 17:42:25]

Pete, maybe we can discuss what this is about on Tuesday or so.

[Pete Williamson, 2017-01-03 20:22:53]

https://codereview.chromium.org/2608553002/diff/20001/components/offline_page...
File components/offline_pages/core/background/offliner.h (right):

https://codereview.chromium.org/2608553002/diff/20001/components/offline_page...
components/offline_pages/core/background/offliner.h:43: // The
RequestCoordinator did not start loading the request.
Should we split this out into a new code?  I'm not sure if it will be important
to be able to tell the difference between the new and old meaning of the code
while looking at UMA.  Maybe this isn't important, if we can slice the UMA by
milestone, but I thought I'd check to make sure we had a story.

https://codereview.chromium.org/2608553002/diff/20001/components/offline_page...
File
components/offline_pages/core/background/request_coordinator_event_logger.cc
(right):

https://codereview.chromium.org/2608553002/diff/20001/components/offline_page...
components/offline_pages/core/background/request_coordinator_event_logger.cc:38:
case Offliner::LOADING_NOT_ACCEPTED:
Do we need a new test case?  I noticed that we removed some tests, but I didn't
see any new ones.

[dougarnett, 2017-01-04 22:03:38]

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

[commit-bot: I haz the power, 2017-01-04 22:04:10]

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

[dougarnett, 2017-01-04 22:23:43]

https://codereview.chromium.org/2608553002/diff/20001/components/offline_page...
File components/offline_pages/core/background/offliner.h (right):

https://codereview.chromium.org/2608553002/diff/20001/components/offline_page...
components/offline_pages/core/background/offliner.h:43: // The
RequestCoordinator did not start loading the request.
On 2017/01/03 20:22:53, Pete Williamson wrote:
> Should we split this out into a new code?  I'm not sure if it will be
important
> to be able to tell the difference between the new and old meaning of the code
> while looking at UMA.  Maybe this isn't important, if we can slice the UMA by
> milestone, but I thought I'd check to make sure we had a story.

Done

https://codereview.chromium.org/2608553002/diff/20001/components/offline_page...
File
components/offline_pages/core/background/request_coordinator_event_logger.cc
(right):

https://codereview.chromium.org/2608553002/diff/20001/components/offline_page...
components/offline_pages/core/background/request_coordinator_event_logger.cc:38:
case Offliner::LOADING_NOT_ACCEPTED:
On 2017/01/03 20:22:53, Pete Williamson wrote:
> Do we need a new test case?  I noticed that we removed some tests, but I
didn't
> see any new ones.

I removed CanPrerender() method and so removed tests related to that method for
PrerenderingLoader and PrerenderAdapter. 

The existing case of Offliner returning false has different status now
(LOADING_NOT_STARTED => LOADING_NOT_ACCEPTED). There is a test that covers that
(StartScheduledProcessingWithLoadingDisabled) - it used to trigger on the
CanPrerender() check but now the primary code falls through to the LoadAndSave()
call returning false so I think this is ok. 

But there is a remaining case of a database failure for
RequestQueue::MarkAttemptStarted() that could result in LOADING_NOT_STAR
TED or now QUEUE_UPDATE_FAILED. It would be good to have a test for that case
but is a bit off target for this CL and may need some RequestQueue Stub support.
Should I add in a follow up CL?

[Pete Williamson, 2017-01-04 22:31:41]

https://codereview.chromium.org/2608553002/diff/20001/components/offline_page...
File
components/offline_pages/core/background/request_coordinator_event_logger.cc
(right):

https://codereview.chromium.org/2608553002/diff/20001/components/offline_page...
components/offline_pages/core/background/request_coordinator_event_logger.cc:38:
case Offliner::LOADING_NOT_ACCEPTED:
On 2017/01/04 22:23:43, dougarnett wrote:
> On 2017/01/03 20:22:53, Pete Williamson wrote:
> > Do we need a new test case?  I noticed that we removed some tests, but I
> didn't
> > see any new ones.
> 
> I removed CanPrerender() method and so removed tests related to that method
for
> PrerenderingLoader and PrerenderAdapter. 
> 
> The existing case of Offliner returning false has different status now
> (LOADING_NOT_STARTED => LOADING_NOT_ACCEPTED). There is a test that covers
that
> (StartScheduledProcessingWithLoadingDisabled) - it used to trigger on the
> CanPrerender() check but now the primary code falls through to the
LoadAndSave()
> call returning false so I think this is ok. 
> 
> But there is a remaining case of a database failure for
> RequestQueue::MarkAttemptStarted() that could result in LOADING_NOT_STAR
> TED or now QUEUE_UPDATE_FAILED. It would be good to have a test for that case
> but is a bit off target for this CL and may need some RequestQueue Stub
support.
> Should I add in a follow up CL?

I'm happy to see the test case in a follow up CL.  Please add either a TODO here
or open a bug.

https://codereview.chromium.org/2608553002/diff/40001/components/offline_page...
File components/offline_pages/core/background/request_coordinator_unittest.cc
(right):

https://codereview.chromium.org/2608553002/diff/40001/components/offline_page...
components/offline_pages/core/background/request_coordinator_unittest.cc:512:
SetIsLowEndDeviceForTest(false);
This should probably be moved into SetUp(), and only the functions that need to
use a low end device should call SetIsLowEndDeviceForTest(true);

[dougarnett, 2017-01-04 22:52:46]

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

[commit-bot: I haz the power, 2017-01-04 22:53:14]

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

[dougarnett, 2017-01-04 23:04:59]

https://codereview.chromium.org/2608553002/diff/20001/components/offline_page...
File
components/offline_pages/core/background/request_coordinator_event_logger.cc
(right):

https://codereview.chromium.org/2608553002/diff/20001/components/offline_page...
components/offline_pages/core/background/request_coordinator_event_logger.cc:38:
case Offliner::LOADING_NOT_ACCEPTED:
On 2017/01/04 22:31:41, Pete Williamson wrote:
> On 2017/01/04 22:23:43, dougarnett wrote:
> > On 2017/01/03 20:22:53, Pete Williamson wrote:
> > > Do we need a new test case?  I noticed that we removed some tests, but I
> > didn't
> > > see any new ones.
> > 
> > I removed CanPrerender() method and so removed tests related to that method
> for
> > PrerenderingLoader and PrerenderAdapter. 
> > 
> > The existing case of Offliner returning false has different status now
> > (LOADING_NOT_STARTED => LOADING_NOT_ACCEPTED). There is a test that covers
> that
> > (StartScheduledProcessingWithLoadingDisabled) - it used to trigger on the
> > CanPrerender() check but now the primary code falls through to the
> LoadAndSave()
> > call returning false so I think this is ok. 
> > 
> > But there is a remaining case of a database failure for
> > RequestQueue::MarkAttemptStarted() that could result in LOADING_NOT_STAR
> > TED or now QUEUE_UPDATE_FAILED. It would be good to have a test for that
case
> > but is a bit off target for this CL and may need some RequestQueue Stub
> support.
> > Should I add in a follow up CL?
> 
> I'm happy to see the test case in a follow up CL.  Please add either a TODO
here
> or open a bug.

TODO-ed

https://codereview.chromium.org/2608553002/diff/40001/components/offline_page...
File components/offline_pages/core/background/request_coordinator_unittest.cc
(right):

https://codereview.chromium.org/2608553002/diff/40001/components/offline_page...
components/offline_pages/core/background/request_coordinator_unittest.cc:512:
SetIsLowEndDeviceForTest(false);
On 2017/01/04 22:31:41, Pete Williamson wrote:
> This should probably be moved into SetUp(), and only the functions that need
to
> use a low end device should call SetIsLowEndDeviceForTest(true);

Done.

[dougarnett, 2017-01-04 23:07:16]

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

[dougarnett, 2017-01-04 23:07:21]

Steven, can you review histogram enum change here? 
Deprecated one enum value and added two new values to replace its cases.

[Steven Holte, 2017-01-04 23:32:20]

histograms lgtm

[commit-bot: I haz the power, 2017-01-05 00:34:59]

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

[commit-bot: I haz the power, 2017-01-05 00:34:59]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Pete Williamson, 2017-01-05 01:23:47]

lgtm

(Sorry, I didn't think the low end device change would be that much work, but it
is a good change to have)

[dougarnett, 2017-01-05 02:55:17]

The CQ bit was checked by dougarnett@chromium.org

[commit-bot: I haz the power, 2017-01-05 02:55:51]

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-01-05 03:46:26]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1483584917980050,
"parent_rev": "59491d29f0923cef7539de15b8bfc284cfbddd9f", "commit_rev":
"ba3cabd3353bc51e42cb1bee14c7901dd3176b10"}

[commit-bot: I haz the power, 2017-01-05 03:46:58]

Description was changed from

==========
[OfflinePages] Improve visiblity/handling of "Loading not started" case
The IsPrerenderingPossible() no longer is applicable to the "offline"
origin prerender requests (it is now more tied to NoStatePrefetch
setting) so this stops prematurely checking it (via CanPrerender())
and instead passes request to PrerenderManager to let is decide whether
it accepts the "offline" origin request or not.

Also adds new Offliner RequestStatus code LOADING_NOT_ACCEPTED to
add clarity between the RC not trying to start the request vs. the
Offliner not accepting it.

BUG=677189
==========

to

==========
[OfflinePages] Improve visiblity/handling of "Loading not started" case
The IsPrerenderingPossible() no longer is applicable to the "offline"
origin prerender requests (it is now more tied to NoStatePrefetch
setting) so this stops prematurely checking it (via CanPrerender())
and instead passes request to PrerenderManager to let is decide whether
it accepts the "offline" origin request or not.

Also adds new Offliner RequestStatus code LOADING_NOT_ACCEPTED to
add clarity between the RC not trying to start the request vs. the
Offliner not accepting it.

BUG=677189

Review-Url: https://codereview.chromium.org/2608553002
==========

[commit-bot: I haz the power, 2017-01-05 03:47:00]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2017-01-05 03:50:36]

Description was changed from

==========
[OfflinePages] Improve visiblity/handling of "Loading not started" case
The IsPrerenderingPossible() no longer is applicable to the "offline"
origin prerender requests (it is now more tied to NoStatePrefetch
setting) so this stops prematurely checking it (via CanPrerender())
and instead passes request to PrerenderManager to let is decide whether
it accepts the "offline" origin request or not.

Also adds new Offliner RequestStatus code LOADING_NOT_ACCEPTED to
add clarity between the RC not trying to start the request vs. the
Offliner not accepting it.

BUG=677189

Review-Url: https://codereview.chromium.org/2608553002
==========

to

==========
[OfflinePages] Improve visiblity/handling of "Loading not started" case
The IsPrerenderingPossible() no longer is applicable to the "offline"
origin prerender requests (it is now more tied to NoStatePrefetch
setting) so this stops prematurely checking it (via CanPrerender())
and instead passes request to PrerenderManager to let is decide whether
it accepts the "offline" origin request or not.

Also adds new Offliner RequestStatus code LOADING_NOT_ACCEPTED to
add clarity between the RC not trying to start the request vs. the
Offliner not accepting it.

BUG=677189

Committed: https://crrev.com/9033511eba1ce7c9b06a4372df9030ccd177353b
Cr-Commit-Position: refs/heads/master@{#441573}
==========

[commit-bot: I haz the power, 2017-01-05 03:50:38]

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