Remove persistent notifications from the database when they close.

Remove persistent notifications from the database when they close.

This CL implements said behavior both for Android (by directly
invoking PNSI::OnClose when an Intent is received) and for desktop
through the PersistentNotificationDelegate.

Tests will follow with a more elaborate suite for the database,
when it's clear what lives in //chrome and what lives in //content.

Depends on https://codereview.chromium.org/1099093003/
BUG=447628
Committed: https://crrev.com/f930f7d84a95ce4b73b487d892bce39184c0f828
Cr-Commit-Position: refs/heads/master@{#327912}

[Peter Beverloo, 2015-04-09 17:13:46]

peter@chromium.org changed reviewers:
+ dewittj@chromium.org, johnme@chromium.org

[johnme, 2015-04-10 11:54:39]

https://codereview.chromium.org/1071773003/diff/1/chrome/browser/notification...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/1071773003/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_impl.cc:105:
base::Unretained(context),
This doesn't look safe. Possible solution: move the RefCountedThreadSafe from
PlatformNotificationContextImpl to PlatformNotificationContext, being careful to
preserve what thread it gets deleted on.

https://codereview.chromium.org/1071773003/diff/1/chrome/browser/notification...
File chrome/browser/notifications/platform_notification_service_impl.h (right):

https://codereview.chromium.org/1071773003/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_impl.h:47: // case,
to make sure that it continues to be in sync.
Nit: Must be called on the UI thread.

https://codereview.chromium.org/1071773003/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_impl.h:49:
content::BrowserContext* browser_context,
Why not just take a Profile* since this is the chrome layer?

[dewittj, 2015-04-10 17:18:31]

https://codereview.chromium.org/1071773003/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_ui_manager_android.cc (right):

https://codereview.chromium.org/1071773003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_ui_manager_android.cc:104: //
TODO(peter): Rather than assuming that the last used profile is the
can this happen sooner rather than later?  Or do a dcheck that there is only one
profile?

https://codereview.chromium.org/1071773003/diff/1/chrome/browser/notification...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/1071773003/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_impl.cc:105:
base::Unretained(context),
On 2015/04/10 11:54:39, johnme wrote:
> This doesn't look safe. Possible solution: move the RefCountedThreadSafe from
> PlatformNotificationContextImpl to PlatformNotificationContext, being careful
to
> preserve what thread it gets deleted on.

Other classes hanging off of StoragePartition use this strategy (see
IndexedDbContext(Impl) and NavigatorConnectContext(Impl)

[Peter Beverloo, 2015-04-20 16:47:20]

https://codereview.chromium.org/1071773003/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_ui_manager_android.cc (right):

https://codereview.chromium.org/1071773003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_ui_manager_android.cc:104: //
TODO(peter): Rather than assuming that the last used profile is the
On 2015/04/10 17:18:30, dewittj wrote:
> can this happen sooner rather than later?  Or do a dcheck that there is only
one
> profile?

Chrome for Android only supports a single profile, if a second one were to be
introduced there would be many, many things that'd break. (Many feature don't
have the TODO either.) bauerb@ confirmed this in crbug.com/437574.

I'm not entirely sure yet about the appropriate fix - there are some privacy
concerns with storing the profile's path in the Intent extras.

In either case - yes, I agree that this is potentially fragile and can break
when somebody does try to introduce multiple profiles.

https://codereview.chromium.org/1071773003/diff/1/chrome/browser/notification...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/1071773003/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_impl.cc:105:
base::Unretained(context),
On 2015/04/10 17:18:30, dewittj wrote:
> On 2015/04/10 11:54:39, johnme wrote:
> > This doesn't look safe. Possible solution: move the RefCountedThreadSafe
from
> > PlatformNotificationContextImpl to PlatformNotificationContext, being
careful
> to
> > preserve what thread it gets deleted on.
> 
> Other classes hanging off of StoragePartition use this strategy (see
> IndexedDbContext(Impl) and NavigatorConnectContext(Impl)

Done.

https://codereview.chromium.org/1071773003/diff/1/chrome/browser/notification...
File chrome/browser/notifications/platform_notification_service_impl.h (right):

https://codereview.chromium.org/1071773003/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_impl.h:47: // case,
to make sure that it continues to be in sync.
On 2015/04/10 11:54:39, johnme wrote:
> Nit: Must be called on the UI thread.

Done.

https://codereview.chromium.org/1071773003/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_impl.h:49:
content::BrowserContext* browser_context,
On 2015/04/10 11:54:39, johnme wrote:
> Why not just take a Profile* since this is the chrome layer?

It only needs to know about the BrowserContext*, so best to keep it separate. We
try to avoid introducing further dependencies on Profile*, slowly working
towards componentization.

[dewittj, 2015-04-20 16:57:54]

lgtm

[johnme, 2015-04-20 16:59:31]

lgtm, thanks :)

[Peter Beverloo, 2015-04-20 17:00:42]

peter@chromium.org changed reviewers:
+ avi@chromium.org

[Peter Beverloo, 2015-04-20 17:00:43]

Thanks guys!

+avi for content/public/ change

[Avi (use Gerrit), 2015-04-20 17:34:34]

LGTM

[Peter Beverloo, 2015-04-20 17:38:53]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2015-04-20 17:39:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1071773003/20001

[Peter Beverloo, 2015-04-20 18:22:06]

The CQ bit was unchecked by peter@chromium.org

[Peter Beverloo, 2015-04-20 18:22:28]

On 2015/04/20 17:39:18, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1071773003/20001

The failure is genuine - I first need to update the forced Push notification to
use the database.

[Peter Beverloo, 2015-05-01 15:06:58]

Rebased on top of the following CL, which included the content/public/ changes:

https://codereview.chromium.org/1099093003/

[Peter Beverloo, 2015-05-01 15:07:02]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2015-05-01 15:07:03]

The patchset sent to the CQ was uploaded after l-g-t-m from johnme@chromium.org,
avi@chromium.org, dewittj@chromium.org
Link to the patchset: https://codereview.chromium.org/1071773003/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2015-05-01 15:07:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1071773003/40001

[commit-bot: I haz the power, 2015-05-01 16:03:50]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-01 16:04:49]

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