[Offline Pages] Defines longer processing budget for immediate bg loads.

[Offline Pages] Defines longer processing budget for immediate bg loads.

BUG=655341
Committed: https://crrev.com/48475f48458467f0f196847a6d007dc444294be7
Cr-Commit-Position: refs/heads/master@{#427098}

[dougarnett, 2016-10-19 17:10:55]

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

[commit-bot: I haz the power, 2016-10-19 17:11:29]

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-10-19 17:21:59]

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

[commit-bot: I haz the power, 2016-10-19 17:22:00]

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-10-19 21:04:22]

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

[commit-bot: I haz the power, 2016-10-19 21:04:59]

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

[dougarnett, 2016-10-19 21:11:34]

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

[Dmitry Titov, 2016-10-19 21:41:25]

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

[Dmitry Titov, 2016-10-19 21:41:26]

Drive-by:

https://chromiumcodereview.appspot.com/2431193003/diff/20001/components/offli...
File components/offline_pages/background/offliner_policy.h (right):

https://chromiumcodereview.appspot.com/2431193003/diff/20001/components/offli...
components/offline_pages/background/offliner_policy.h:11: const int
kDefaultBackgroundProcessingTimeBudgetSeconds = 60 * 3 - 10;
Would be good to have some comments reflecting why these specific values are
selected. Are they arbitrary? Or not? "-10" looks like it tries to be 10 seconds
less than some other constant. Which one?

[Dmitry Titov, 2016-10-19 21:41:28]

Drive-by:

[commit-bot: I haz the power, 2016-10-19 22:21:17]

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

[commit-bot: I haz the power, 2016-10-19 22:21:18]

Dry run: This issue passed the CQ dry run.

[Pete Williamson, 2016-10-19 23:18:18]

https://chromiumcodereview.appspot.com/2431193003/diff/20001/components/offli...
File components/offline_pages/background/offliner_policy.h (right):

https://chromiumcodereview.appspot.com/2431193003/diff/20001/components/offli...
components/offline_pages/background/offliner_policy.h:11: const int
kDefaultBackgroundProcessingTimeBudgetSeconds = 60 * 3 - 10;
On 2016/10/19 21:41:26, Dmitry Titov wrote:
> Would be good to have some comments reflecting why these specific values are
> selected. Are they arbitrary? Or not? "-10" looks like it tries to be 10
seconds
> less than some other constant. Which one?

This is based on the Marshmallow Doze mode 3 minute limit.  We want to avoid
being killed by doze mode by finishing up before it is over.  In the future, we
might be smarter about what OS we are running on, and use longer timeouts on
pre-marshmallow OSs.

https://chromiumcodereview.appspot.com/2431193003/diff/20001/components/offli...
components/offline_pages/background/offliner_policy.h:13: const int
kSinglePageTimeLimitWhenBackgroundScheduledSeconds = 60 * 2;
Based on our recent Gin2G-poor testing, we might also put this at 170 seconds.

https://chromiumcodereview.appspot.com/2431193003/diff/20001/components/offli...
components/offline_pages/background/offliner_policy.h:92: int
GetProcessingTimeBudgetWhenBackgroundScheduledInSeconds() const {
Maybe better to have one method that takes an enum instead of two.
SCHEDULED_WINDOW or IMMEDIATE_WINDOW might work here.

https://chromiumcodereview.appspot.com/2431193003/diff/20001/components/offli...
File components/offline_pages/background/request_coordinator_unittest.cc
(right):

https://chromiumcodereview.appspot.com/2431193003/diff/20001/components/offli...
components/offline_pages/background/request_coordinator_unittest.cc:267:
SetIsLowEndDeviceForTest(false);
I think I'd prefer to see this set-up in the test, these methods are supposed to
be simple pass-throughs to the same named method.

I'd even be happy with putting these values into the SetUp routine (and
overriding them as needed).

https://chromiumcodereview.appspot.com/2431193003/diff/20001/components/offli...
components/offline_pages/background/request_coordinator_unittest.cc:571:
StartProcessingImmediately();
Why add this?  The more of the chain that we bring into the test, the more
complicated to debug the test becomes. I'm trying to isolate the method under
test as much as possible.  If we need it, that's fine, but I wouldn't want to
add it just to make the tests look similar.

https://chromiumcodereview.appspot.com/2431193003/diff/20001/components/offli...
components/offline_pages/background/request_coordinator_unittest.cc:625:
StartProcessingImmediately();
Same question here, and the other instances below.

[dougarnett, 2016-10-19 23:29:12]

https://chromiumcodereview.appspot.com/2431193003/diff/20001/components/offli...
File components/offline_pages/background/request_coordinator_unittest.cc
(right):

https://chromiumcodereview.appspot.com/2431193003/diff/20001/components/offli...
components/offline_pages/background/request_coordinator_unittest.cc:267:
SetIsLowEndDeviceForTest(false);
On 2016/10/19 23:18:18, Pete Williamson wrote:
> I think I'd prefer to see this set-up in the test, these methods are supposed
to
> be simple pass-throughs to the same named method.
> 
> I'd even be happy with putting these values into the SetUp routine (and
> overriding them as needed).

Had this all repeated in many tests. I can go back to that. Since they are
related to making the immediate trigger work, I was trying to collect together.

https://chromiumcodereview.appspot.com/2431193003/diff/20001/components/offli...
components/offline_pages/background/request_coordinator_unittest.cc:571:
StartProcessingImmediately();
On 2016/10/19 23:18:18, Pete Williamson wrote:
> Why add this?  The more of the chain that we bring into the test, the more
> complicated to debug the test becomes. I'm trying to isolate the method under
> test as much as possible.  If we need it, that's fine, but I wouldn't want to
> add it just to make the tests look similar.

The motivation was that we now have a processing_state that was not being set up
by this test approach where it is never actually started. This was hitting a
DCHECK that I would like to keep. An alternative might be to provide test access
to directly set/mock the processing_state but seemed fragile path.

[dougarnett, 2016-10-21 18:28:54]

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

[commit-bot: I haz the power, 2016-10-21 18:29:37]

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-10-21 18:40:56]

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

[commit-bot: I haz the power, 2016-10-21 18:40:57]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dougarnett, 2016-10-21 18:47:00]

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

[commit-bot: I haz the power, 2016-10-21 18:47:36]

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-10-21 18:57:34]

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

[commit-bot: I haz the power, 2016-10-21 18:57:35]

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

[dougarnett, 2016-10-21 19:54:59]

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

[commit-bot: I haz the power, 2016-10-21 19:55:27]

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

[dougarnett, 2016-10-21 20:01:20]

https://codereview.chromium.org/2431193003/diff/20001/components/offline_page...
File components/offline_pages/background/offliner_policy.h (right):

https://codereview.chromium.org/2431193003/diff/20001/components/offline_page...
components/offline_pages/background/offliner_policy.h:11: const int
kDefaultBackgroundProcessingTimeBudgetSeconds = 60 * 3 - 10;
On 2016/10/19 23:18:18, Pete Williamson wrote:
> On 2016/10/19 21:41:26, Dmitry Titov wrote:
> > Would be good to have some comments reflecting why these specific values are
> > selected. Are they arbitrary? Or not? "-10" looks like it tries to be 10
> seconds
> > less than some other constant. Which one?
> 
> This is based on the Marshmallow Doze mode 3 minute limit.  We want to avoid
> being killed by doze mode by finishing up before it is over.  In the future,
we
> might be smarter about what OS we are running on, and use longer timeouts on
> pre-marshmallow OSs.

Did some doc and naming here to try to communicate a bit about these
values/guesses

https://codereview.chromium.org/2431193003/diff/20001/components/offline_page...
components/offline_pages/background/offliner_policy.h:13: const int
kSinglePageTimeLimitWhenBackgroundScheduledSeconds = 60 * 2;
On 2016/10/19 23:18:18, Pete Williamson wrote:
> Based on our recent Gin2G-poor testing, we might also put this at 170 seconds.

Done.

https://codereview.chromium.org/2431193003/diff/20001/components/offline_page...
components/offline_pages/background/offliner_policy.h:92: int
GetProcessingTimeBudgetWhenBackgroundScheduledInSeconds() const {
On 2016/10/19 23:18:18, Pete Williamson wrote:
> Maybe better to have one method that takes an enum instead of two.
> SCHEDULED_WINDOW or IMMEDIATE_WINDOW might work here.

Adding TODO for now. Currently this enum is private so let's talk more about
exposing and possible other policy considerations between these modes - eg, I'm
wondering about different TryNextRequest behaviour on failures/timeouts with
immediate mode (but will need design discussion).

https://codereview.chromium.org/2431193003/diff/20001/components/offline_page...
File components/offline_pages/background/request_coordinator_unittest.cc
(right):

https://codereview.chromium.org/2431193003/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator_unittest.cc:267:
SetIsLowEndDeviceForTest(false);
On 2016/10/19 23:29:12, dougarnett wrote:
> On 2016/10/19 23:18:18, Pete Williamson wrote:
> > I think I'd prefer to see this set-up in the test, these methods are
supposed
> to
> > be simple pass-throughs to the same named method.
> > 
> > I'd even be happy with putting these values into the SetUp routine (and
> > overriding them as needed).
> 
> Had this all repeated in many tests. I can go back to that. Since they are
> related to making the immediate trigger work, I was trying to collect
together.

Dropped the immediate start and reworked common setup for OfflinerDoneCallback
tests into helper method that includes setting some necessary processing state.

https://codereview.chromium.org/2431193003/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator_unittest.cc:625:
StartProcessingImmediately();
On 2016/10/19 23:18:18, Pete Williamson wrote:
> Same question here, and the other instances below.

Done.

[commit-bot: I haz the power, 2016-10-21 21:21:29]

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

[commit-bot: I haz the power, 2016-10-21 21:21:30]

Dry run: This issue passed the CQ dry run.

[Pete Williamson, 2016-10-22 01:10:34]

lgtm

[dougarnett, 2016-10-24 14:46:54]

The CQ bit was checked by dougarnett@chromium.org

[commit-bot: I haz the power, 2016-10-24 14:47:12]

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-10-24 15:50:54]

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

[commit-bot: I haz the power, 2016-10-24 15:50:58]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dougarnett, 2016-10-24 17:10:25]

The CQ bit was checked by dougarnett@chromium.org

[commit-bot: I haz the power, 2016-10-24 17:11:08]

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-10-24 18:09:13]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-24 18:22:42]

Description was changed from

==========
[Offline Pages] Defines longer processing budget for immediate bg loads.

BUG=655341
==========

to

==========
[Offline Pages] Defines longer processing budget for immediate bg loads.

BUG=655341

Committed: https://crrev.com/48475f48458467f0f196847a6d007dc444294be7
Cr-Commit-Position: refs/heads/master@{#427098}
==========

[commit-bot: I haz the power, 2016-10-24 18:22:43]

Patchset 5 (id:??) landed as
https://crrev.com/48475f48458467f0f196847a6d007dc444294be7
Cr-Commit-Position: refs/heads/master@{#427098}