[Offline pages] Enabling notifications for pages saved later

[Offline pages] Enabling notifications for pages saved later

* Adding observer of Request Coordinator (as user data)
* Forwarding the notifications from observer to OPNotificationBridge
* Making RequestCoordinator capable of supporting user data
* Minor fixes to interfaces (virtual destructors added)

BUG=630817
R=dimich,petewil

Committed: https://crrev.com/3c871abdd9427c8e502131990d66415980f29c2d
Cr-Commit-Position: refs/heads/master@{#414162}

[fgorski, 2016-08-19 22:43:55]

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

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

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-20 01:00:56]

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

[commit-bot: I haz the power, 2016-08-20 01:00:57]

Dry run: This issue passed the CQ dry run.

[fgorski, 2016-08-23 22:10:01]

Description was changed from

==========
[Offline pages] Enabling notifications for pages saved later

* Adding observer of Request Coordinator (as user data)
* Forwarding the notifications from observer to OPNotificationBridge
* Making RequestCoordinator capable of supporting user data
* Minor fixes to interfaces (virtual destructors added)

BUG=630817
R=dimich,petewil
==========

to

==========
[Offline pages] Enabling notifications for pages saved later

* Adding observer of Request Coordinator (as user data)
* Forwarding the notifications from observer to OPNotificationBridge
* Making RequestCoordinator capable of supporting user data
* Minor fixes to interfaces (virtual destructors added)

BUG=630817
R=dimich,petewil
==========

[fgorski, 2016-08-23 22:10:01]

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

[fgorski, 2016-08-23 23:46:07]

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

[fgorski, 2016-08-23 23:46:25]

PTAL

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

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-23 23:50:46]

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

[commit-bot: I haz the power, 2016-08-23 23:50:52]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Dmitry Titov, 2016-08-24 00:29:36]

LGTM % dependency fix and couple of nits:

https://codereview.chromium.org/2260963003/diff/20001/components/offline_page...
File components/offline_pages/downloads/download_ui_item.cc (right):

https://codereview.chromium.org/2260963003/diff/20001/components/offline_page...
components/offline_pages/downloads/download_ui_item.cc:32: url(other.url),
I know it's not part of your patch, but I've noticed this ctor does not copy
'title'. Could you please just add it here?

https://codereview.chromium.org/2260963003/diff/20001/components/offline_page...
File components/offline_pages/downloads/download_ui_item.h (right):

https://codereview.chromium.org/2260963003/diff/20001/components/offline_page...
components/offline_pages/downloads/download_ui_item.h:25: explicit
DownloadUIItem(const SavePageRequest& request);
I wonder if we need a bool is_downloaded on the item, and initialize it one way
or another depending which ctor is used. Of course only if it's useflu in UI, so
can add later when we need it.

[Pete Williamson, 2016-08-24 17:02:41]

https://codereview.chromium.org/2260963003/diff/20001/components/components_t...
File components/components_tests.gyp (right):

https://codereview.chromium.org/2260963003/diff/20001/components/components_t...
components/components_tests.gyp:446:
'offline_pages/downloads/download_notifying_observer_unittest.cc',
This is fine, but we don't need to keep updating GYP files IIUC.

https://codereview.chromium.org/2260963003/diff/20001/components/offline_page...
File components/offline_pages/background/request_notifier.h (right):

https://codereview.chromium.org/2260963003/diff/20001/components/offline_page...
components/offline_pages/background/request_notifier.h:26: virtual
~RequestNotifier() = default;
This is fine, but why add it?  Should we be doing this everywhere?

https://codereview.chromium.org/2260963003/diff/20001/components/offline_page...
File components/offline_pages/downloads/download_notifying_observer.cc (right):

https://codereview.chromium.org/2260963003/diff/20001/components/offline_page...
components/offline_pages/downloads/download_notifying_observer.cc:50: //
TODO(fgorski): Discuss change related logic with the team.
What discussion do we need to have? (OK to answer offline)

https://codereview.chromium.org/2260963003/diff/20001/components/offline_page...
components/offline_pages/downloads/download_notifying_observer.cc:61: if (status
== RequestCoordinator::SavePageStatus::SUCCESS)
Is it OK at this level to treat removed requests as failed?

https://codereview.chromium.org/2260963003/diff/20001/components/offline_page...
File components/offline_pages/downloads/download_notifying_observer_unittest.cc
(right):

https://codereview.chromium.org/2260963003/diff/20001/components/offline_page...
components/offline_pages/downloads/download_notifying_observer_unittest.cc:227: 
It might be good to add a test for the deleted case of OnCompleted(), if
anything needs to be different than the failure case.

[fgorski, 2016-08-24 18:21:36]

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

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

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-24 18:25:01]

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

[fgorski, 2016-08-24 18:25:23]

Addressed, PTAL

https://codereview.chromium.org/2260963003/diff/20001/components/components_t...
File components/components_tests.gyp (right):

https://codereview.chromium.org/2260963003/diff/20001/components/components_t...
components/components_tests.gyp:446:
'offline_pages/downloads/download_notifying_observer_unittest.cc',
On 2016/08/24 17:02:41, Pete Williamson wrote:
> This is fine, but we don't need to keep updating GYP files IIUC.

Cool. I'll drop it then.

https://codereview.chromium.org/2260963003/diff/20001/components/offline_page...
File components/offline_pages/background/request_notifier.h (right):

https://codereview.chromium.org/2260963003/diff/20001/components/offline_page...
components/offline_pages/background/request_notifier.h:26: virtual
~RequestNotifier() = default;
On 2016/08/24 17:02:41, Pete Williamson wrote:
> This is fine, but why add it?  Should we be doing this everywhere?

http://stackoverflow.com/questions/461203/when-to-use-virtual-destructors
http://www.programmerinterview.com/index.php/c-cplusplus/virtual-destructors/

I agree that we might not need it here, because of how me manage RC lifecycle,
but given that you can still inherit from this one outside of RC, we probably
want to keep it virtual.

https://codereview.chromium.org/2260963003/diff/20001/components/offline_page...
File components/offline_pages/downloads/download_notifying_observer.cc (right):

https://codereview.chromium.org/2260963003/diff/20001/components/offline_page...
components/offline_pages/downloads/download_notifying_observer.cc:50: //
TODO(fgorski): Discuss change related logic with the team.
On 2016/08/24 17:02:41, Pete Williamson wrote:
> What discussion do we need to have? (OK to answer offline)

For now I think this is OK. I believe we actually discussed that when we covered
observer and I simply haven't removed TODO.

https://codereview.chromium.org/2260963003/diff/20001/components/offline_page...
components/offline_pages/downloads/download_notifying_observer.cc:61: if (status
== RequestCoordinator::SavePageStatus::SUCCESS)
On 2016/08/24 17:02:41, Pete Williamson wrote:
> Is it OK at this level to treat removed requests as failed?

Done. We can cancel as an alternative.

https://codereview.chromium.org/2260963003/diff/20001/components/offline_page...
File components/offline_pages/downloads/download_notifying_observer_unittest.cc
(right):

https://codereview.chromium.org/2260963003/diff/20001/components/offline_page...
components/offline_pages/downloads/download_notifying_observer_unittest.cc:227: 
On 2016/08/24 17:02:41, Pete Williamson wrote:
> It might be good to add a test for the deleted case of OnCompleted(), if
> anything needs to be different than the failure case.

Not sure what you mean. I think REMOVED covered above deals with that.

https://codereview.chromium.org/2260963003/diff/20001/components/offline_page...
File components/offline_pages/downloads/download_ui_item.cc (right):

https://codereview.chromium.org/2260963003/diff/20001/components/offline_page...
components/offline_pages/downloads/download_ui_item.cc:32: url(other.url),
On 2016/08/24 00:29:35, Dmitry Titov wrote:
> I know it's not part of your patch, but I've noticed this ctor does not copy
> 'title'. Could you please just add it here?

Done. Good catch.

https://codereview.chromium.org/2260963003/diff/20001/components/offline_page...
File components/offline_pages/downloads/download_ui_item.h (right):

https://codereview.chromium.org/2260963003/diff/20001/components/offline_page...
components/offline_pages/downloads/download_ui_item.h:25: explicit
DownloadUIItem(const SavePageRequest& request);
On 2016/08/24 00:29:36, Dmitry Titov wrote:
> I wonder if we need a bool is_downloaded on the item, and initialize it one
way
> or another depending which ctor is used. Of course only if it's useflu in UI,
so
> can add later when we need it.

Acknowledged. Let's add that later if/when we need it.

[commit-bot: I haz the power, 2016-08-24 18:25:52]

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-24 19:40:30]

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

[commit-bot: I haz the power, 2016-08-24 19:40:31]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Dmitry Titov, 2016-08-24 20:22:00]

still lgtm

[Pete Williamson, 2016-08-24 20:41:14]

lgtm

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

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2016-08-24 20:46:16]

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 21:59:53]

Description was changed from

==========
[Offline pages] Enabling notifications for pages saved later

* Adding observer of Request Coordinator (as user data)
* Forwarding the notifications from observer to OPNotificationBridge
* Making RequestCoordinator capable of supporting user data
* Minor fixes to interfaces (virtual destructors added)

BUG=630817
R=dimich,petewil
==========

to

==========
[Offline pages] Enabling notifications for pages saved later

* Adding observer of Request Coordinator (as user data)
* Forwarding the notifications from observer to OPNotificationBridge
* Making RequestCoordinator capable of supporting user data
* Minor fixes to interfaces (virtual destructors added)

BUG=630817
R=dimich,petewil
==========

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

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-24 22:01:40]

Description was changed from

==========
[Offline pages] Enabling notifications for pages saved later

* Adding observer of Request Coordinator (as user data)
* Forwarding the notifications from observer to OPNotificationBridge
* Making RequestCoordinator capable of supporting user data
* Minor fixes to interfaces (virtual destructors added)

BUG=630817
R=dimich,petewil
==========

to

==========
[Offline pages] Enabling notifications for pages saved later

* Adding observer of Request Coordinator (as user data)
* Forwarding the notifications from observer to OPNotificationBridge
* Making RequestCoordinator capable of supporting user data
* Minor fixes to interfaces (virtual destructors added)

BUG=630817
R=dimich,petewil

Committed: https://crrev.com/3c871abdd9427c8e502131990d66415980f29c2d
Cr-Commit-Position: refs/heads/master@{#414162}
==========

[commit-bot: I haz the power, 2016-08-24 22:01:42]

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