Set up a backup schedule request if we have deferred requests.

Set up a backup schedule request if we have deferred requests.

BUG=652889
Committed: https://crrev.com/731c9896052c3824ec4630a1461738ecbccb16b5
Cr-Commit-Position: refs/heads/master@{#424315}

[Pete Williamson, 2016-10-08 00:59:44]

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

[Pete Williamson, 2016-10-08 00:59:45]

This is patch 3 in the set to handle deferred requests, it depends on patch 2
and 1 (1 has already landed, 2 is currently out for review).

[Dmitry Titov, 2016-10-10 17:29:45]

https://codereview.chromium.org/2400393002/diff/1/chrome/browser/ui/webui/off...
File chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc
(right):

https://codereview.chromium.org/2400393002/diff/1/chrome/browser/ui/webui/off...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc:291:
ENABLED_FOR_OFFLINER) != 0));
I think ">" is even "stronger" condition here, why is it important to check for
"!="?

https://codereview.chromium.org/2400393002/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2400393002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:28: const int
kDisabledTaskRecheckSeconds = 5 * 60;
I think this needs to be a much shorter interval. Here is the logic:
- these tasks will be there only while disabled_requests_ is not empty, if it
becomes empty we'll clear them right away.
- that list will only be non-empty while Chrome is running and actively
snapshotting page(s) in tabs
- it's good for user if we keep download 'running' even if they switch to other
app.

So lets say we have this period to be 5 seconds. While Chrome is running,
responding to a once-in-5-seconds task is very cheap. Once Chrome is evicted
from RAM, the next wake up will be the last of those, and it'll run background
loader 'in parallel' with whatever activity is there - same as it may do in 5
minutes, but with better match of user expectations.

WDYT?

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

https://codereview.chromium.org/2400393002/diff/1/chrome/browser/ui/webui/off...
File chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc
(right):

https://codereview.chromium.org/2400393002/diff/1/chrome/browser/ui/webui/off...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc:291:
ENABLED_FOR_OFFLINER) != 0));
On 2016/10/10 17:29:45, Dmitry Titov wrote:
> I think ">" is even "stronger" condition here, why is it important to check
for
> "!="?

SavePageLater now returns a request_id.  The request id is an integer instead of
a uint, a randomly generated request id could be negative.

If desired, in another changelist, we could change it to uint everywhere, but
that will be a big change in both offline_page_model and request_coordinator, so
we should do it separately.

https://codereview.chromium.org/2400393002/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2400393002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:28: const int
kDisabledTaskRecheckSeconds = 5 * 60;
On 2016/10/10 17:29:45, Dmitry Titov wrote:
> I think this needs to be a much shorter interval. Here is the logic:
> - these tasks will be there only while disabled_requests_ is not empty, if it
> becomes empty we'll clear them right away.
> - that list will only be non-empty while Chrome is running and actively
> snapshotting page(s) in tabs
> - it's good for user if we keep download 'running' even if they switch to
other
> app.
> 
> So lets say we have this period to be 5 seconds. While Chrome is running,
> responding to a once-in-5-seconds task is very cheap. Once Chrome is evicted
> from RAM, the next wake up will be the last of those, and it'll run background
> loader 'in parallel' with whatever activity is there - same as it may do in 5
> minutes, but with better match of user expectations.
> 
> WDYT?

Done.

[Dmitry Titov, 2016-10-10 21:45:19]

On 2016/10/10 19:45:18, Pete Williamson wrote:
>
https://codereview.chromium.org/2400393002/diff/1/chrome/browser/ui/webui/off...
> File chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc
> (right):
> 
>
https://codereview.chromium.org/2400393002/diff/1/chrome/browser/ui/webui/off...
> chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc:291:
> ENABLED_FOR_OFFLINER) != 0));
> On 2016/10/10 17:29:45, Dmitry Titov wrote:
> > I think ">" is even "stronger" condition here, why is it important to check
> for
> > "!="?
> 
> SavePageLater now returns a request_id.  The request id is an integer instead
of
> a uint, a randomly generated request id could be negative.
> 
> If desired, in another changelist, we could change it to uint everywhere, but
> that will be a big change in both offline_page_model and request_coordinator,
so
> we should do it separately.

From what I see we use uint random generator capped by int's max():
https://cs.chromium.org/chromium/src/components/offline_pages/background/requ...
That means they are always positive and >0. So instead of replacing them to be
uints, I'd add a DCHECK(id > 0) here:
https://cs.chromium.org/chromium/src/components/offline_pages/background/requ...
and support the positive random range. Since we started to have 'special values'
like 0, we might need more than one and it's weird to have only one value from a
range to be 'special'.

[Pete Williamson, 2016-10-10 22:53:12]

https://codereview.chromium.org/2400393002/diff/1/chrome/browser/ui/webui/off...
File chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc
(right):

https://codereview.chromium.org/2400393002/diff/1/chrome/browser/ui/webui/off...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc:291:
ENABLED_FOR_OFFLINER) != 0));
On 2016/10/10 19:45:17, Pete Williamson wrote:
> On 2016/10/10 17:29:45, Dmitry Titov wrote:
> > I think ">" is even "stronger" condition here, why is it important to check
> for
> > "!="?
> 
> SavePageLater now returns a request_id.  The request id is an integer instead
of
> a uint, a randomly generated request id could be negative.
> 
> If desired, in another changelist, we could change it to uint everywhere, but
> that will be a big change in both offline_page_model and request_coordinator,
so
> we should do it separately.

Done.  Ah, I didn't realize the random number generator promised to give us
positive numbers.

[Dmitry Titov, 2016-10-11 00:03:38]

lgtm

[Pete Williamson, 2016-10-11 00:08:29]

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

[commit-bot: I haz the power, 2016-10-11 00:08:54]

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

[Pete Williamson, 2016-10-11 00:09:08]

The CQ bit was unchecked by petewil@chromium.org

[Pete Williamson, 2016-10-11 00:09:22]

The CQ bit was checked by petewil@chromium.org

[commit-bot: I haz the power, 2016-10-11 00:09:50]

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-11 00:57:53]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-11 01:02:22]

Description was changed from

==========
Set up a backup schedule request if we have deferred requests.

BUG=652889
==========

to

==========
Set up a backup schedule request if we have deferred requests.

BUG=652889

Committed: https://crrev.com/731c9896052c3824ec4630a1461738ecbccb16b5
Cr-Commit-Position: refs/heads/master@{#424315}
==========

[commit-bot: I haz the power, 2016-10-11 01:02:24]

Patchset 3 (id:??) landed as
https://crrev.com/731c9896052c3824ec4630a1461738ecbccb16b5
Cr-Commit-Position: refs/heads/master@{#424315}

[dougarnett, 2016-10-17 18:33:39]

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

[dougarnett, 2016-10-17 18:33:40]

Late drive-by

https://codereview.chromium.org/2400393002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java
(right):

https://codereview.chromium.org/2400393002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:41:
BackgroundScheduler.schedule(ContextUtils.getApplicationContext(),
triggerConditions);
delayInSeconds not used here? Missing a TODO at least?

[Pete Williamson, 2016-10-21 18:56:35]

On 2016/10/17 18:33:40, dougarnett wrote:
> Late drive-by
> 
>
https://codereview.chromium.org/2400393002/diff/40001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java
> (right):
> 
>
https://codereview.chromium.org/2400393002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:41:
> BackgroundScheduler.schedule(ContextUtils.getApplicationContext(),
> triggerConditions);
> delayInSeconds not used here? Missing a TODO at least?

Looks like I forgot to change the call from schedule to backgroundSchedule in
one place.  Fixed by 
https://codereview.chromium.org/2400393002/