hook up background task's "startProcessing" from Java to request_coordinator

hook up background task's "startProcessing" from Java to request_coordinator

BUG=612325
Committed: https://crrev.com/1f7ec09af677ae2148a559cc8a6517505f0e52f9
Cr-Commit-Position: refs/heads/master@{#399204}

[Pete Williamson, 2016-06-09 18:26:27]

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

[commit-bot: I haz the power, 2016-06-09 18:27:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2054913002/1

[commit-bot: I haz the power, 2016-06-09 20:56:15]

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

[commit-bot: I haz the power, 2016-06-09 20:56:16]

Dry run: This issue passed the CQ dry run.

[Pete Williamson, 2016-06-09 21:00:33]

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

[Pete Williamson, 2016-06-09 21:00:34]

The final wire in the wireframe.

[dougarnett, 2016-06-09 21:54:27]

lgtm with comments

https://codereview.chromium.org/2054913002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/background_scheduler_bridge.cc
(right):

https://codereview.chromium.org/2054913002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/background_scheduler_bridge.cc:40: Profile*
profile = ProfileManager::GetLastUsedProfile();
indent

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

https://codereview.chromium.org/2054913002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:74: // Inform the
scheduler that we have an outstanding task.
thought this comment wording was confusing - the scheduler is dumb about why it
is scheduled. Isn't this is really about keeping the scheduler active in case we
crash before we know if we need to be rescheduled?

// While we have a pending request, be sure the Scheduler will trigger
// again in case we crash before we know if we are done with requests.

[Pete Williamson, 2016-06-09 22:09:48]

https://codereview.chromium.org/2054913002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/background_scheduler_bridge.cc
(right):

https://codereview.chromium.org/2054913002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/background_scheduler_bridge.cc:40: Profile*
profile = ProfileManager::GetLastUsedProfile();
On 2016/06/09 21:54:27, dougarnett wrote:
> indent

Done.

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

https://codereview.chromium.org/2054913002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:74: // Inform the
scheduler that we have an outstanding task.
On 2016/06/09 21:54:27, dougarnett wrote:
> thought this comment wording was confusing - the scheduler is dumb about why
it
> is scheduled. Isn't this is really about keeping the scheduler active in case
we
> crash before we know if we need to be rescheduled?
> 
> // While we have a pending request, be sure the Scheduler will trigger
> // again in case we crash before we know if we are done with requests. 

No, this is really about ensuring that the current request is scheduled. If this
is the first request, and no others are active, the scheduler may not have
anything at all on the list.  This will make sure it has something for the
current request.  True, it may already have something scheduled, so I can change
the wording to "Ensure that..."  The place for making sure we are scheduled is
when a new task arrives from the scheduler, not when we take something off the
request queue (I haven't added that yet).

[Pete Williamson, 2016-06-09 22:09:54]

The CQ bit was checked by petewil@chromium.org

[Pete Williamson, 2016-06-09 22:09:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
dougarnett@chromium.org
Link to the patchset: https://codereview.chromium.org/2054913002/#ps20001
(title: "CR feedback per DougArnett")

[commit-bot: I haz the power, 2016-06-09 22:10:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2054913002/20001

[dougarnett, 2016-06-09 22:27:06]

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

https://codereview.chromium.org/2054913002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:74: // Inform the
scheduler that we have an outstanding task.
On 2016/06/09 22:09:48, Pete Williamson wrote:
> On 2016/06/09 21:54:27, dougarnett wrote:
> > thought this comment wording was confusing - the scheduler is dumb about why
> it
> > is scheduled. Isn't this is really about keeping the scheduler active in
case
> we
> > crash before we know if we need to be rescheduled?
> > 
> > // While we have a pending request, be sure the Scheduler will trigger
> > // again in case we crash before we know if we are done with requests. 
> 
> No, this is really about ensuring that the current request is scheduled. If
this
> is the first request, and no others are active, the scheduler may not have
> anything at all on the list.  This will make sure it has something for the
> current request.  True, it may already have something scheduled, so I can
change
> the wording to "Ensure that..."  The place for making sure we are scheduled is
> when a new task arrives from the scheduler, not when we take something off the
> request queue (I haven't added that yet).

Hmm, not sure I understand your response. Might be easier to sync up on in
person sometime. 

It is not clear why we need something scheduled for the request we are
processing (since the scheduler just fired a StartProcessing to do it). We are
dispatching it to offliner below and it will possibly complete and not need to
be tried again. So I think this is all about the covering for the crash scenario
and, if so, then we should identify the purpose better. If not, then I am still
missing something here.

[Pete Williamson, 2016-06-09 22:30:17]

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

https://codereview.chromium.org/2054913002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:74: // Inform the
scheduler that we have an outstanding task.
On 2016/06/09 22:27:05, dougarnett wrote:
> On 2016/06/09 22:09:48, Pete Williamson wrote:
> > On 2016/06/09 21:54:27, dougarnett wrote:
> > > thought this comment wording was confusing - the scheduler is dumb about
why
> > it
> > > is scheduled. Isn't this is really about keeping the scheduler active in
> case
> > we
> > > crash before we know if we need to be rescheduled?
> > > 
> > > // While we have a pending request, be sure the Scheduler will trigger
> > > // again in case we crash before we know if we are done with requests. 
> > 
> > No, this is really about ensuring that the current request is scheduled. If
> this
> > is the first request, and no others are active, the scheduler may not have
> > anything at all on the list.  This will make sure it has something for the
> > current request.  True, it may already have something scheduled, so I can
> change
> > the wording to "Ensure that..."  The place for making sure we are scheduled
is
> > when a new task arrives from the scheduler, not when we take something off
the
> > request queue (I haven't added that yet).
> 
> Hmm, not sure I understand your response. Might be easier to sync up on in
> person sometime. 
> 
> It is not clear why we need something scheduled for the request we are
> processing (since the scheduler just fired a StartProcessing to do it). We are
> dispatching it to offliner below and it will possibly complete and not need to
> be tried again. So I think this is all about the covering for the crash
scenario
> and, if so, then we should identify the purpose better. If not, then I am
still
> missing something here.

I was thinking that we might also get here via an API call to offline a page
instead of just via the scheduler, but now that I think some more, that will go
through the scheduler too, so this always comes from the scheduler.  In that
case, I will remove this (later), and move the backup reschedule into the
scheduler module.

[commit-bot: I haz the power, 2016-06-10 00:12:48]

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

[commit-bot: I haz the power, 2016-06-10 00:12:49]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)

[Pete Williamson, 2016-06-10 00:15:02]

The CQ bit was checked by petewil@chromium.org

[commit-bot: I haz the power, 2016-06-10 00:15:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2054913002/20001

[commit-bot: I haz the power, 2016-06-10 02:17:44]

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

[commit-bot: I haz the power, 2016-06-10 02:17:45]

Exceeded global retry quota

[Pete Williamson, 2016-06-10 16:26:00]

The CQ bit was checked by petewil@chromium.org

[commit-bot: I haz the power, 2016-06-10 16:26:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2054913002/20001

[commit-bot: I haz the power, 2016-06-10 16:30:24]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-10 16:30:38]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-10 16:31:29]

Description was changed from

==========
hook up background task's "startProcessing" from Java to request_coordinator

BUG=612325
==========

to

==========
hook up background task's "startProcessing" from Java to request_coordinator

BUG=612325

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

[commit-bot: I haz the power, 2016-06-10 16:31:31]

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