[Offline Pages] Resume pending requests when chrome starts.

[Offline Pages] Resume pending requests when chrome starts.

Resume pending download requests for offline pages when chrome starts by
calling startImmediateProcessing. This should only work for user-requested
downloads.

BUG=695307
Review-Url: https://codereview.chromium.org/2714693003
Cr-Commit-Position: refs/heads/master@{#454655}
Committed: https://chromium.googlesource.com/chromium/src/+/5ca7b6b81b7dfb267481b28728d36ac3cef4b87c

[romax, 2017-02-23 20:56:01]

romax@chromium.org changed reviewers:
+ dougarnett@chromium.org, fgorski@chromium.org

[romax, 2017-02-23 20:56:02]

fgorski@, PTAL
dougarnett@, FYI

Thanks!

[fgorski, 2017-02-27 17:07:46]

On 2017/02/23 20:56:02, romax wrote:
> fgorski@, PTAL
> dougarnett@, FYI
> 
> Thanks!

I don't understand why you are making this change in the bridge.
Let's talk about that in the office.

[dougarnett, 2017-02-27 17:24:27]

https://codereview.chromium.org/2714693003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java
(right):

https://codereview.chromium.org/2714693003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:144:
nativeResumePendingRequestImmediately(mNativeOfflinePageDownloadBridge);
Some contextual comment may be helpful here. Eg, what does !hasUserGesture imply
here (web page download?). 

Also wonder about mentioning we expect resumeDownload to be called only for
user-requests (or requests with notifications) - not sure that is appropriate at
this level but wanted to get us to consider it.

[romax, 2017-03-02 22:00:14]

dtrainor@ said it's okay to have the change for now and they're looking into a
bigger refactoring of the structure design.
Added comments.
PTAL, thanks

https://codereview.chromium.org/2714693003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java
(right):

https://codereview.chromium.org/2714693003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:144:
nativeResumePendingRequestImmediately(mNativeOfflinePageDownloadBridge);
On 2017/02/27 17:24:27, dougarnett wrote:
> Some contextual comment may be helpful here. Eg, what does !hasUserGesture
imply
> here (web page download?). 
> 
> Also wonder about mentioning we expect resumeDownload to be called only for
> user-requests (or requests with notifications) - not sure that is appropriate
at
> this level but wanted to get us to consider it.

Added some comments.
Also I've thought about the issue here if I'm calling startImmediateProcessing()
then we might end up resuming some non-user-requested requests as well. It might
be a good thing along with considering the priorities of requests on
request_queue level but I'm told that we might only care about user-requested
pages in RC (maybe i misunderstood)?

[romax, 2017-03-02 22:00:31]

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

[commit-bot: I haz the power, 2017-03-02 22:01:07]

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, 2017-03-02 22:03:31]

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

[commit-bot: I haz the power, 2017-03-02 22:03:32]

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...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[dougarnett, 2017-03-02 22:59:42]

https://codereview.chromium.org/2714693003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java
(right):

https://codereview.chromium.org/2714693003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:144:
nativeResumePendingRequestImmediately(mNativeOfflinePageDownloadBridge);
On 2017/03/02 22:00:14, romax wrote:
> On 2017/02/27 17:24:27, dougarnett wrote:
> > Some contextual comment may be helpful here. Eg, what does !hasUserGesture
> imply
> > here (web page download?). 
> > 
> > Also wonder about mentioning we expect resumeDownload to be called only for
> > user-requests (or requests with notifications) - not sure that is
appropriate
> at
> > this level but wanted to get us to consider it.
> 
> Added some comments.
> Also I've thought about the issue here if I'm calling
startImmediateProcessing()
> then we might end up resuming some non-user-requested requests as well. It
might
> be a good thing along with considering the priorities of requests on
> request_queue level but I'm told that we might only care about user-requested
> pages in RC (maybe i misunderstood)?

Thanks for the comment. It is a good topic wrt support non-user-requested
requests. Are we going to do that? For Zine? If so, then it might be fine if we
let the current system conditions gate whether we do the low priority ones (eg,
if need wifi or good battery level). But something to decide when adding actual
use of non-user-initiated request (unless that is already in).

The basic idea here is that we won't spin up the RequestCoordinator unless there
was an active notification (for user-initiated request).

[romax, 2017-03-03 00:59:59]

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

[commit-bot: I haz the power, 2017-03-03 01:00:26]

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

[romax, 2017-03-03 01:39:12]

fixed merging issues and comments.

https://codereview.chromium.org/2714693003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java
(right):

https://codereview.chromium.org/2714693003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:144:
nativeResumePendingRequestImmediately(mNativeOfflinePageDownloadBridge);
On 2017/03/02 22:59:42, dougarnett wrote:
> On 2017/03/02 22:00:14, romax wrote:
> > On 2017/02/27 17:24:27, dougarnett wrote:
> > > Some contextual comment may be helpful here. Eg, what does !hasUserGesture
> > imply
> > > here (web page download?). 
> > > 
> > > Also wonder about mentioning we expect resumeDownload to be called only
for
> > > user-requests (or requests with notifications) - not sure that is
> appropriate
> > at
> > > this level but wanted to get us to consider it.
> > 
> > Added some comments.
> > Also I've thought about the issue here if I'm calling
> startImmediateProcessing()
> > then we might end up resuming some non-user-requested requests as well. It
> might
> > be a good thing along with considering the priorities of requests on
> > request_queue level but I'm told that we might only care about
user-requested
> > pages in RC (maybe i misunderstood)?
> 
> Thanks for the comment. It is a good topic wrt support non-user-requested
> requests. Are we going to do that? For Zine? If so, then it might be fine if
we
> let the current system conditions gate whether we do the low priority ones
(eg,
> if need wifi or good battery level). But something to decide when adding
actual
> use of non-user-initiated request (unless that is already in).
> 
> The basic idea here is that we won't spin up the RequestCoordinator unless
there
> was an active notification (for user-initiated request).  

I think we're good at least for now, since there aren't too many clients
actually shipped (?) And TBH I'm not sure about future actual use for
non-user-requested ones or when they'll be added.
As for the patch, the call would only be made if there's an actual download item
(which is shown in notification bar) was called as argument in this method and
the immediate processing wouldn't be started if there's no *pending offline
page* downloads. So I guess your basic idea is the behavior here :)

[commit-bot: I haz the power, 2017-03-03 01:43:12]

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

[commit-bot: I haz the power, 2017-03-03 01:43:13]

Dry run: This issue passed the CQ dry run.

[dougarnett, 2017-03-03 16:52:02]

On 2017/03/03 01:39:12, romax wrote:
> fixed merging issues and comments.
> 
>
https://codereview.chromium.org/2714693003/diff/20001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java
> (right):
> 
>
https://codereview.chromium.org/2714693003/diff/20001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:144:
> nativeResumePendingRequestImmediately(mNativeOfflinePageDownloadBridge);
> On 2017/03/02 22:59:42, dougarnett wrote:
> > On 2017/03/02 22:00:14, romax wrote:
> > > On 2017/02/27 17:24:27, dougarnett wrote:
> > > > Some contextual comment may be helpful here. Eg, what does
!hasUserGesture
> > > imply
> > > > here (web page download?). 
> > > > 
> > > > Also wonder about mentioning we expect resumeDownload to be called only
> for
> > > > user-requests (or requests with notifications) - not sure that is
> > appropriate
> > > at
> > > > this level but wanted to get us to consider it.
> > > 
> > > Added some comments.
> > > Also I've thought about the issue here if I'm calling
> > startImmediateProcessing()
> > > then we might end up resuming some non-user-requested requests as well. It
> > might
> > > be a good thing along with considering the priorities of requests on
> > > request_queue level but I'm told that we might only care about
> user-requested
> > > pages in RC (maybe i misunderstood)?
> > 
> > Thanks for the comment. It is a good topic wrt support non-user-requested
> > requests. Are we going to do that? For Zine? If so, then it might be fine if
> we
> > let the current system conditions gate whether we do the low priority ones
> (eg,
> > if need wifi or good battery level). But something to decide when adding
> actual
> > use of non-user-initiated request (unless that is already in).
> > 
> > The basic idea here is that we won't spin up the RequestCoordinator unless
> there
> > was an active notification (for user-initiated request).  
> 
> I think we're good at least for now, since there aren't too many clients
> actually shipped (?) And TBH I'm not sure about future actual use for
> non-user-requested ones or when they'll be added.
> As for the patch, the call would only be made if there's an actual download
item
> (which is shown in notification bar) was called as argument in this method and
> the immediate processing wouldn't be started if there's no *pending offline
> page* downloads. So I guess your basic idea is the behavior here :)

Yes, I'm fine with this. I just wasn't sure if there had already been changes 
for non-user-initiated (for Zine) that I wasn't aware of.

Have you and Filip discussed this overall approach yet (have only seen the
comment about wanting to discuss in person)?

[fgorski, 2017-03-03 17:44:38]

lgtm

https://codereview.chromium.org/2714693003/diff/60001/chrome/browser/android/...
File
chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc
(right):

https://codereview.chromium.org/2714693003/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:448:
if (coordinator) {
nit: braces not needed.

https://codereview.chromium.org/2714693003/diff/60001/chrome/browser/android/...
File chrome/browser/android/offline_pages/offline_page_bridge.cc (left):

https://codereview.chromium.org/2714693003/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_bridge.cc:626: 
I'd skip changing this file for a simpler merge, in case we want this for 58.

[romax, 2017-03-03 18:53:17]

https://codereview.chromium.org/2714693003/diff/60001/chrome/browser/android/...
File
chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc
(right):

https://codereview.chromium.org/2714693003/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:448:
if (coordinator) {
On 2017/03/03 17:44:37, fgorski wrote:
> nit: braces not needed.

Done.

[romax, 2017-03-03 18:53:43]

The CQ bit was checked by romax@chromium.org

[romax, 2017-03-03 18:53:43]

The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/2714693003/#ps80001
(title: "Comments.")

[commit-bot: I haz the power, 2017-03-03 18:54:20]

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, 2017-03-03 19:57:07]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488567223338410,
"parent_rev": "de83b4c6dd4318bf362c7dda673a300ca29cd2b4", "commit_rev":
"5ca7b6b81b7dfb267481b28728d36ac3cef4b87c"}

[commit-bot: I haz the power, 2017-03-03 19:57:46]

Description was changed from

==========
[Offline Pages] Resume pending requests when chrome starts.

Resume pending download requests for offline pages when chrome starts by
calling startImmediateProcessing. This should only work for user-requested
downloads.

BUG=695307
==========

to

==========
[Offline Pages] Resume pending requests when chrome starts.

Resume pending download requests for offline pages when chrome starts by
calling startImmediateProcessing. This should only work for user-requested
downloads.

BUG=695307

Review-Url: https://codereview.chromium.org/2714693003
Cr-Commit-Position: refs/heads/master@{#454655}
Committed:
https://chromium.googlesource.com/chromium/src/+/5ca7b6b81b7dfb267481b28728d3...
==========

[commit-bot: I haz the power, 2017-03-03 19:57:47]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/5ca7b6b81b7dfb267481b28728d3...