API changes to support suspending requests

API changes to support suspending requests

We want to be able to suspend requests until the offline download
code is sure it won't be manually downloading them.  This API surface
change provides a mechanism to allow requests to be started in a
suspended state, and later transition to an available state.

BUG=652889
Committed: https://crrev.com/80979723f0248e791feae4829095b05aac1eb3a1
Cr-Commit-Position: refs/heads/master@{#423596}

[Pete Williamson, 2016-10-05 21:38:50]

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

[Dmitry Titov, 2016-10-05 22:28:19]

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

https://codereview.chromium.org/2395903002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:120: 
Lets add "DCHECK(id > 0);" here, just to be sure that GenerateOfflineId() still
conforms to this assumption.

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

https://codereview.chromium.org/2395903002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.h:82: bool
user_reqeusted,
typo: user_reqeusted -> user_requested

https://codereview.chromium.org/2395903002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.h:83:
RequestAvailability availability);
I'd rather have a "bool do_not_process" or "bool block_offlining".

It makes more sense to casual reader because the request is still created, and
even still causes the notification to pop up - but it is not being offlined yet.

[Pete Williamson, 2016-10-05 22:57:46]

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

https://codereview.chromium.org/2395903002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:120: 
On 2016/10/05 22:28:19, Dmitry Titov wrote:
> Lets add "DCHECK(id > 0);" here, just to be sure that GenerateOfflineId()
still
> conforms to this assumption.

Added DCHECK(id != 0) instead, since we use an int instead of a uint.

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

https://codereview.chromium.org/2395903002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.h:82: bool
user_reqeusted,
On 2016/10/05 22:28:19, Dmitry Titov wrote:
> typo: user_reqeusted -> user_requested

Nice catch!  Done.

https://codereview.chromium.org/2395903002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.h:83:
RequestAvailability availability);
On 2016/10/05 22:28:19, Dmitry Titov wrote:
> I'd rather have a "bool do_not_process" or "bool block_offlining".
> 
> It makes more sense to casual reader because the request is still created, and
> even still causes the notification to pop up - but it is not being offlined
yet.
> 

I think that FGorski had the opposite feedback in the design doc, to prefer an
enum here.  I prefer an enum, so I'd like to go with that unless you feel
strongly about this one.

[Dmitry Titov, 2016-10-05 23:39:35]

It sounds painful for me to have RequestAvailability parameter on
SavePageLater(url) call, since there is not 'request' parameter and it is
unclear what it is available for and who is "offliner".

But in the interests of stopping bikeshedding of a hack, I'd say:
lgtm

[Pete Williamson, 2016-10-05 23:51:09]

On 2016/10/05 23:39:35, Dmitry Titov wrote:
> It sounds painful for me to have RequestAvailability parameter on
> SavePageLater(url) call, since there is not 'request' parameter and it is
> unclear what it is available for and who is "offliner".
> 
> But in the interests of stopping bikeshedding of a hack, I'd say:
> lgtm

We discussed this offline, and agreed the best solution for now was to change
the name of the enum to make the two states seem more like opposites.

[Pete Williamson, 2016-10-05 23:51:58]

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

[Pete Williamson, 2016-10-05 23:51:59]

BauerB, please look at changes to offline_internals_ui_message_handler.cc

Thanks!

[Pete Williamson, 2016-10-06 00:23:44]

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

[commit-bot: I haz the power, 2016-10-06 00:24:18]

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

[Bernhard Bauer, 2016-10-06 15:22:09]

lgtm

[Pete Williamson, 2016-10-06 16:37:36]

The CQ bit was checked by petewil@chromium.org

[Pete Williamson, 2016-10-06 16:37:36]

The patchset sent to the CQ was uploaded after l-g-t-m from dimich@chromium.org
Link to the patchset: https://codereview.chromium.org/2395903002/#ps80001
(title: "Keep the JS with a boolean instead of forcing a double.")

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

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-06 18:34:24]

Description was changed from

==========
API changes to support suspending requests

We want to be able to suspend requests until the offline download
code is sure it won't be manually downloading them.  This API surface
change provides a mechanism to allow requests to be started in a
suspended state, and later transition to an available state.

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

to

==========
API changes to support suspending requests

We want to be able to suspend requests until the offline download
code is sure it won't be manually downloading them.  This API surface
change provides a mechanism to allow requests to be started in a
suspended state, and later transition to an available state.

BUG=652889

Committed: https://crrev.com/80979723f0248e791feae4829095b05aac1eb3a1
Cr-Commit-Position: refs/heads/master@{#423596}
==========

[commit-bot: I haz the power, 2016-10-06 18:34:25]

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