[Offline Pages] Close race condition of multiple StartProcessing callers.

[Offline Pages] Close race condition of multiple StartProcessing callers.
Introduces new bool to track the starting/picking phase so that we avoid concurrent pickers
(and not overwrite the schedule_callback) in order to also avoid the DCHECK that guards concurrent
SendRequestToOffliner calls.

BUG=643455
Committed: https://crrev.com/81356643cfe6ffe145741fb4b8fef53a3db86a50
Cr-Commit-Position: refs/heads/master@{#416957}

[dougarnett, 2016-09-06 22:16:17]

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

[commit-bot: I haz the power, 2016-09-06 22:16:48]

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

[dougarnett, 2016-09-06 22:28:34]

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

[commit-bot: I haz the power, 2016-09-06 22:28:57]

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

[dougarnett, 2016-09-06 22:30:03]

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

[commit-bot: I haz the power, 2016-09-06 22:42:25]

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

[commit-bot: I haz the power, 2016-09-06 22:42:25]

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...)

[Pete Williamson, 2016-09-06 22:56:44]

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

https://codereview.chromium.org/2317893002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:349: if (is_starting_
|| is_busy_)
Please add a bug to refactor to a single state variable.

https://codereview.chromium.org/2317893002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:350: return false;
(as we discussed in person) BackgroundOfflinerTask.java is not checking the
return value, we should do that.

https://codereview.chromium.org/2317893002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:391: 
is_starting_ = false

[dougarnett, 2016-09-06 23:28:24]

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

[commit-bot: I haz the power, 2016-09-06 23:28:46]

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

[dougarnett, 2016-09-06 23:37:22]

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

[commit-bot: I haz the power, 2016-09-06 23:37:41]

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

[dougarnett, 2016-09-06 23:39:10]

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

https://codereview.chromium.org/2317893002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:349: if (is_starting_
|| is_busy_)
On 2016/09/06 22:56:44, Pete Williamson wrote:
> Please add a bug to refactor to a single state variable.

Done. https://bugs.chromium.org/p/chromium/issues/detail?id=644519

https://codereview.chromium.org/2317893002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:350: return false;
On 2016/09/06 22:56:44, Pete Williamson wrote:
> (as we discussed in person) BackgroundOfflinerTask.java is not checking the
> return value, we should do that.

Done.

https://codereview.chromium.org/2317893002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:391: 
On 2016/09/06 22:56:44, Pete Williamson wrote:
> is_starting_ = false

Done. Thanks!

[Pete Williamson, 2016-09-07 00:16:31]

mostly looks good, one nit.

https://codereview.chromium.org/2317893002/diff/60001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/StubBackgroundSchedulerProcessor.java
(right):

https://codereview.chromium.org/2317893002/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/StubBackgroundSchedulerProcessor.java:15:
private boolean mDidStartProcessing;
Should these two variables get initialized to false somewhere?  Even 8if Java
does that for us by default, it would be nice to be explicit here.

[dougarnett, 2016-09-07 00:41:29]

https://codereview.chromium.org/2317893002/diff/60001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/StubBackgroundSchedulerProcessor.java
(right):

https://codereview.chromium.org/2317893002/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/StubBackgroundSchedulerProcessor.java:15:
private boolean mDidStartProcessing;
On 2016/09/07 00:16:31, Pete Williamson wrote:
> Should these two variables get initialized to false somewhere?  Even 8if Java
> does that for us by default, it would be nice to be explicit here.

Not doing this for members defaults got pounded in me in old google java reviews
and I ended up liking it. I don't find any current rule about it though.

[Pete Williamson, 2016-09-07 00:46:38]

lgtm

[commit-bot: I haz the power, 2016-09-07 01:33:38]

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

[commit-bot: I haz the power, 2016-09-07 01:33:39]

Dry run: This issue passed the CQ dry run.

[dougarnett, 2016-09-07 16:26:07]

The CQ bit was checked by dougarnett@chromium.org

[commit-bot: I haz the power, 2016-09-07 16:26:44]

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-09-07 16:33:13]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-07 16:34:36]

Description was changed from

==========
[Offline Pages] Close race condition of multiple StartProcessing callers.
Introduces new bool to track the starting/picking phase so that we avoid
concurrent pickers
(and not overwrite the schedule_callback) in order to also avoid the DCHECK that
guards concurrent
SendRequestToOffliner calls.

BUG=643455
==========

to

==========
[Offline Pages] Close race condition of multiple StartProcessing callers.
Introduces new bool to track the starting/picking phase so that we avoid
concurrent pickers
(and not overwrite the schedule_callback) in order to also avoid the DCHECK that
guards concurrent
SendRequestToOffliner calls.

BUG=643455

Committed: https://crrev.com/81356643cfe6ffe145741fb4b8fef53a3db86a50
Cr-Commit-Position: refs/heads/master@{#416957}
==========

[commit-bot: I haz the power, 2016-09-07 16:34:37]

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