[Offline pages] Downloads UI: Adding bridge for issuing notifications

[Offline pages] Downloads UI: Adding bridge for issuing notifications

Changes:
* Updates DownloadNotifiactionSerivce to include handling of offline pages
* Adds openItem and destroy methods to DownloadServiceDelegate (to handle offline pages and prevent memory leaks)
* Adds notification bridge for offline pages related downloads (implemented on C++ and Java side) only calls from c++ to Java.
* Adds interface to components that will be implemented by the bridge
* Updating OPDownloadBridge to issue notification when current page is captured.
* Adding copy constructor to DownloadUIItem

BUG=630817
Committed: https://crrev.com/9e8c5f4e30270447969f04bfe4d676b6d94f25b9
Cr-Commit-Position: refs/heads/master@{#413214}

[fgorski, 2016-08-11 23:08:36]

fgorski@chromium.org changed reviewers:
+ dimich@google.com, qinmin@chromium.org

[fgorski, 2016-08-11 23:08:36]

This patch has ways to go, but demonstrates general idea of how to interact with
DownloadNotifier from offline pages side.

[fgorski, 2016-08-12 17:34:57]

Description was changed from

==========
[Offline pages] Downloads UI: Adding bridge for issuing notifications

BUG=
==========

to

==========
[Offline pages] Downloads UI: Adding bridge for issuing notifications

BUG=
==========

[fgorski, 2016-08-12 17:34:57]

fgorski@chromium.org changed reviewers:
+ dimich@chromium.org
- dimich@google.com

[fgorski, 2016-08-12 18:06:30]

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

[commit-bot: I haz the power, 2016-08-12 18:07:00]

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, 2016-08-12 19:00:02]

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

[commit-bot: I haz the power, 2016-08-12 19:00:03]

Dry run: This issue passed the CQ dry run.

[fgorski, 2016-08-12 19:41:59]

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

[commit-bot: I haz the power, 2016-08-12 19:42:46]

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

[Dmitry Titov, 2016-08-12 20:04:10]

looks ok so far

[commit-bot: I haz the power, 2016-08-12 20:41:35]

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

[commit-bot: I haz the power, 2016-08-12 20:41:36]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[fgorski, 2016-08-16 17:03:07]

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

[commit-bot: I haz the power, 2016-08-16 17:03:32]

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, 2016-08-16 17:06:37]

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

[commit-bot: I haz the power, 2016-08-16 17:06:38]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[fgorski, 2016-08-16 22:30:55]

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

[commit-bot: I haz the power, 2016-08-16 22:32:05]

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, 2016-08-16 23:38:36]

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

[commit-bot: I haz the power, 2016-08-16 23:38:37]

Dry run: This issue passed the CQ dry run.

[fgorski, 2016-08-17 04:57:23]

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

[commit-bot: I haz the power, 2016-08-17 04:57:34]

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, 2016-08-17 06:17:14]

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

[commit-bot: I haz the power, 2016-08-17 06:17:15]

Dry run: This issue passed the CQ dry run.

[fgorski, 2016-08-17 22:12:40]

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

[commit-bot: I haz the power, 2016-08-17 22:13:05]

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, 2016-08-17 23:07:45]

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

[commit-bot: I haz the power, 2016-08-17 23:07:46]

Dry run: This issue passed the CQ dry run.

[fgorski, 2016-08-18 22:59:37]

Description was changed from

==========
[Offline pages] Downloads UI: Adding bridge for issuing notifications

BUG=
==========

to

==========
[Offline pages] Downloads UI: Adding bridge for issuing notifications

Changes:
* Updates DownloadNotifiactionSerivce to include handling of offline pages
* Adds openItem and destroy methods to DownloadServiceDelegate (to handle
offline pages and prevent memory leaks)
* Adds notification bridge for offline pages related downloads (implemented on
C++ and Java side) only calls from c++ to Java.
* Adds interface to components that will be implemented by the bridge
* Updating OPDownloadBridge to issue notification when current page is captured.
* Adding copy constructor to DownloadUIItem 

BUG=630817
==========

[fgorski, 2016-08-18 22:59:51]

ptal

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

https://codereview.chromium.org/2239133002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:72:
return createDefault();
these 4 lines to be removed

[Dmitry Titov, 2016-08-18 23:30:18]

lgtm

I think request_coordinator.[h,cc] changes are unrelated to this patch (although
seem useful) - can split into another?

[qinmin, 2016-08-18 23:46:59]

lgtm % comments

https://codereview.chromium.org/2239133002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java
(right):

https://codereview.chromium.org/2239133002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:497:
? DownloadSharedPreferenceEntry.ITEM_TYPE_OFFLINE_PAGE
fix indentation

https://codereview.chromium.org/2239133002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageNotificationBridge.java
(right):

https://codereview.chromium.org/2239133002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageNotificationBridge.java:40:
notifier.notifyDownloadSuccessful(downloadInfo, systemDownloadId, canResolve);
just put -1 and false here without temporary variables

https://codereview.chromium.org/2239133002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageNotificationBridge.java:118:
final boolean isAutoResumable = true;
same here

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

https://codereview.chromium.org/2239133002/diff/160001/chrome/browser/android...
chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:204:
if (result == SavePageResult::SUCCESS) {
no need for {} for if-else statements that has only 1 lines in it

[fgorski, 2016-08-19 17:00:22]

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

[commit-bot: I haz the power, 2016-08-19 17:00:56]

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

[fgorski, 2016-08-19 17:03:24]

comments addressed. running bots now.

https://codereview.chromium.org/2239133002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java
(right):

https://codereview.chromium.org/2239133002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:497:
? DownloadSharedPreferenceEntry.ITEM_TYPE_OFFLINE_PAGE
On 2016/08/18 23:46:59, qinmin wrote:
> fix indentation

Done. Problem here is that this is the product of "git cl format"
I'll try to check this in without an automated "fix".

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

https://codereview.chromium.org/2239133002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:72:
return createDefault();
On 2016/08/18 22:59:51, fgorski wrote:
> these 4 lines to be removed

Done.

https://codereview.chromium.org/2239133002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageNotificationBridge.java
(right):

https://codereview.chromium.org/2239133002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageNotificationBridge.java:40:
notifier.notifyDownloadSuccessful(downloadInfo, systemDownloadId, canResolve);
On 2016/08/18 23:46:59, qinmin wrote:
> just put -1 and false here without temporary variables

Done.

https://codereview.chromium.org/2239133002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageNotificationBridge.java:118:
final boolean isAutoResumable = true;
On 2016/08/18 23:46:59, qinmin wrote:
> same here

Done.

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

https://codereview.chromium.org/2239133002/diff/160001/chrome/browser/android...
chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:204:
if (result == SavePageResult::SUCCESS) {
On 2016/08/18 23:46:59, qinmin wrote:
> no need for {} for if-else statements that has only 1 lines in it

Done.

[commit-bot: I haz the power, 2016-08-19 18:14:00]

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

[commit-bot: I haz the power, 2016-08-19 18:14:01]

Dry run: Exceeded global retry quota

[fgorski, 2016-08-19 18:33:23]

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2016-08-19 18:33:24]

The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org,
dimich@chromium.org
Link to the patchset: https://codereview.chromium.org/2239133002/#ps180001
(title: "Addressing CR feedback")

[commit-bot: I haz the power, 2016-08-19 18:33:59]

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-19 19:49:36]

Description was changed from

==========
[Offline pages] Downloads UI: Adding bridge for issuing notifications

Changes:
* Updates DownloadNotifiactionSerivce to include handling of offline pages
* Adds openItem and destroy methods to DownloadServiceDelegate (to handle
offline pages and prevent memory leaks)
* Adds notification bridge for offline pages related downloads (implemented on
C++ and Java side) only calls from c++ to Java.
* Adds interface to components that will be implemented by the bridge
* Updating OPDownloadBridge to issue notification when current page is captured.
* Adding copy constructor to DownloadUIItem 

BUG=630817
==========

to

==========
[Offline pages] Downloads UI: Adding bridge for issuing notifications

Changes:
* Updates DownloadNotifiactionSerivce to include handling of offline pages
* Adds openItem and destroy methods to DownloadServiceDelegate (to handle
offline pages and prevent memory leaks)
* Adds notification bridge for offline pages related downloads (implemented on
C++ and Java side) only calls from c++ to Java.
* Adds interface to components that will be implemented by the bridge
* Updating OPDownloadBridge to issue notification when current page is captured.
* Adding copy constructor to DownloadUIItem 

BUG=630817
==========

[commit-bot: I haz the power, 2016-08-19 19:49:37]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-08-19 19:51:50]

Description was changed from

==========
[Offline pages] Downloads UI: Adding bridge for issuing notifications

Changes:
* Updates DownloadNotifiactionSerivce to include handling of offline pages
* Adds openItem and destroy methods to DownloadServiceDelegate (to handle
offline pages and prevent memory leaks)
* Adds notification bridge for offline pages related downloads (implemented on
C++ and Java side) only calls from c++ to Java.
* Adds interface to components that will be implemented by the bridge
* Updating OPDownloadBridge to issue notification when current page is captured.
* Adding copy constructor to DownloadUIItem 

BUG=630817
==========

to

==========
[Offline pages] Downloads UI: Adding bridge for issuing notifications

Changes:
* Updates DownloadNotifiactionSerivce to include handling of offline pages
* Adds openItem and destroy methods to DownloadServiceDelegate (to handle
offline pages and prevent memory leaks)
* Adds notification bridge for offline pages related downloads (implemented on
C++ and Java side) only calls from c++ to Java.
* Adds interface to components that will be implemented by the bridge
* Updating OPDownloadBridge to issue notification when current page is captured.
* Adding copy constructor to DownloadUIItem

BUG=630817

Committed: https://crrev.com/9e8c5f4e30270447969f04bfe4d676b6d94f25b9
Cr-Commit-Position: refs/heads/master@{#413214}
==========

[commit-bot: I haz the power, 2016-08-19 19:51:51]

Patchset 10 (id:??) landed as
https://crrev.com/9e8c5f4e30270447969f04bfe4d676b6d94f25b9
Cr-Commit-Position: refs/heads/master@{#413214}