Adding a new extra field to intent for offline page downloads

Adding a new extra field to intent for offline page downloads

getDownloadEntryFromIntent() always assume intent is for regular
download.
This causes chrome to use DownloadManagerService rather than
OfflinePageDownloadBridge when clicking on a notification.
This change fixes the issue by adding an extra field for offline page.

BUG=640467
Committed: https://crrev.com/fe96fd4adb9967bd7aaacf7047eda65bce1510ba
Cr-Commit-Position: refs/heads/master@{#414182}

[qinmin, 2016-08-24 19:52:48]

qinmin@chromium.org changed reviewers:
+ dfalcantara@chromium.org, fgorski@chromium.org

[qinmin, 2016-08-24 19:52:49]

PTAL

[fgorski, 2016-08-24 20:00:08]

lgtm, but consider using this flag when handling intent.

https://codereview.chromium.org/2278663002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java
(right):

https://codereview.chromium.org/2278663002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:505:
int itemType = entry != null ? entry.itemType
should we use the new intent flag here?

[fgorski, 2016-08-24 20:01:33]

one more comment.

https://codereview.chromium.org/2278663002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java
(right):

https://codereview.chromium.org/2278663002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:285:
entry.itemType == DownloadSharedPreferenceEntry.ITEM_TYPE_OFFLINE_PAGE;
isOfflinePage(entry)

[gone, 2016-08-24 21:01:46]

lgtm

[qinmin, 2016-08-24 21:07:30]

https://codereview.chromium.org/2278663002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java
(right):

https://codereview.chromium.org/2278663002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:285:
entry.itemType == DownloadSharedPreferenceEntry.ITEM_TYPE_OFFLINE_PAGE;
On 2016/08/24 20:01:33, fgorski wrote:
> isOfflinePage(entry)

There is no such function in this file. Added
DownloadSharedPreferenceEntry.isOfflinePage() call to do this.

https://codereview.chromium.org/2278663002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:505:
int itemType = entry != null ? entry.itemType
On 2016/08/24 20:00:08, fgorski wrote:
> should we use the new intent flag here?

no, we shouldn't. Intent might not contain EXTRA_DOWNLOAD_IS_OFFLINE_PAGE. For
example, if the intent action is RESUME_ALL_DOWNLOADS.

[qinmin, 2016-08-24 21:07:39]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2016-08-24 21:07:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org, fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/2278663002/#ps20001
(title: "add isOfflinePage()")

[commit-bot: I haz the power, 2016-08-24 21:08:41]

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

[fgorski, 2016-08-24 21:09:33]

one more comment ;)

https://codereview.chromium.org/2278663002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java
(right):

https://codereview.chromium.org/2278663002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:731:
private boolean isOfflinePage(DownloadSharedPreferenceEntry entry) {
this. Please remove as it belongs on the entry.

[qinmin, 2016-08-24 21:12:11]

The CQ bit was unchecked by qinmin@chromium.org

[qinmin, 2016-08-24 21:18:39]

https://codereview.chromium.org/2278663002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java
(right):

https://codereview.chromium.org/2278663002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:731:
private boolean isOfflinePage(DownloadSharedPreferenceEntry entry) {
On 2016/08/24 21:09:33, fgorski wrote:
> this. Please remove as it belongs on the entry.

ah... Sorry, didn't find this call earlier. Removed.

[qinmin, 2016-08-24 21:18:48]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2016-08-24 21:18:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org, fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/2278663002/#ps40001
(title: "remove a redudant function")

[commit-bot: I haz the power, 2016-08-24 21:19:38]

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-08-24 22:44:03]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-24 22:47:09]

Description was changed from

==========
Adding a new extra field to intent for offline page downloads

getDownloadEntryFromIntent() always assume intent is for regular
download.
This causes chrome to use DownloadManagerService rather than
OfflinePageDownloadBridge when clicking on a notification.
This change fixes the issue by adding an extra field for offline page.

BUG=640467
==========

to

==========
Adding a new extra field to intent for offline page downloads

getDownloadEntryFromIntent() always assume intent is for regular
download.
This causes chrome to use DownloadManagerService rather than
OfflinePageDownloadBridge when clicking on a notification.
This change fixes the issue by adding an extra field for offline page.

BUG=640467

Committed: https://crrev.com/fe96fd4adb9967bd7aaacf7047eda65bce1510ba
Cr-Commit-Position: refs/heads/master@{#414182}
==========

[commit-bot: I haz the power, 2016-08-24 22:47:10]

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