The platform notification context should observe the Service Worker Context.

The platform notification context should observe the Service Worker Context.

When a Service Worker registration gets dropped, all notifications
associated with that notification should be removed as well.

Two cases which will be addressed in follow-up patches are when the
Service Worker database gets wiped, and granting the notification context
the ability to close deleted notifications by itself.

Design document:
  http://goo.gl/TciXVp

BUG=447628
Committed: https://crrev.com/f9ec7d0c69f83c869ef7d34d4cb60858be38d4c6
Cr-Commit-Position: refs/heads/master@{#322692}

[Peter Beverloo, 2015-03-19 18:06:47]

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

[Peter Beverloo, 2015-03-19 18:06:47]

+johnme for notifications

[johnme, 2015-03-20 14:56:36]

https://codereview.chromium.org/1014703007/diff/1/content/browser/notificatio...
File content/browser/notifications/platform_notification_context_impl.cc
(right):

https://codereview.chromium.org/1014703007/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context_impl.cc:17: void
EmptyFailureCallback() {}
Use DoNothing from base/bind_helpers.h instead.

https://codereview.chromium.org/1014703007/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context_impl.cc:57:
BrowserThread::PostTask(
My understanding is that there's now a race condition allowing the context to be
deleted on either the UI or IO threads. It seems this probably doesn't do any
harm (since you ensure NotificationDatabase is destroyed on the right thread),
but please add a comment* to the destructor documenting this, and please also
add BrowserThread::DeleteOnUIThread as the traits of
ServiceWorkerContextWrapper's RefCoountedThreadSafe superclass** (since
ServiceWorkerContextWrapper's ServiceWorkerProcessManager seems to assume it'll
always be deleted on the UI thread).

OR, more simply, add BrowserThread::DeleteOnUIThread as the traits of
PlatformNotificationContext's RefCountedThreadSafe superclass.

*: and/or a DCHECK:

DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO) ||
BrowserThread::CurrentlyOn(BrowserThread::UI));

**: or (though this seems worse) reset service_worker_context_ in Shutdown().

https://codereview.chromium.org/1014703007/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context_impl.cc:206: //
TODO(peter): Record UMA on status for deleting from the database.
Nit: add a matching "Do the DeleteAndStartOver dance for
STATUS_ERROR_CORRUPTED." TODO?

https://codereview.chromium.org/1014703007/diff/1/content/browser/notificatio...
File content/browser/notifications/platform_notification_context_impl.h (right):

https://codereview.chromium.org/1014703007/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context_impl.h:32: //
defined in this interface must only be called on the IO thread.
Please add "unless otherwise specified".

https://codereview.chromium.org/1014703007/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context_impl.h:37: //
Constructs a new platform notification context. If |path| is non-empty, the
Please add "on the UI thread"

https://codereview.chromium.org/1014703007/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context_impl.h:42: const
scoped_refptr<ServiceWorkerContextWrapper>& service_worker_context);
Why not pass the scoped_refptr by value?

https://codereview.chromium.org/1014703007/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context_impl.h:85: void
DoDeleteNotificationsForServiceWorkerRegistration(
Micro-nit: move this after DoDeleteNotificationData so it's easier to contrast
them?

https://codereview.chromium.org/1014703007/diff/1/content/browser/notificatio...
File content/browser/notifications/platform_notification_context_unittest.cc
(right):

https://codereview.chromium.org/1014703007/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context_unittest.cc:254:
base::RunLoop().RunUntilIdle();
This smells flaky; ditto elsewhere. Why not pass the RunLoop's QuitClosure to
DidWriteNotificationData?

https://codereview.chromium.org/1014703007/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context_unittest.cc:256:
ASSERT_TRUE(success());
Nit: EXPECT_TRUE seems more useful, unless this being false might cause the test
to timeout or crash horribly, as that way subsequent EXPECT/ASSERT statements
will give you more information about the state of the test that might make it
easier to debug failures.

[Peter Beverloo, 2015-03-20 18:39:05]

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

[Peter Beverloo, 2015-03-20 18:39:06]

+avi for moving lifetime semantics out of the content API, and a few minor
additions to storage_partition_impl.cc.

https://codereview.chromium.org/1014703007/diff/1/content/browser/notificatio...
File content/browser/notifications/platform_notification_context_impl.cc
(right):

https://codereview.chromium.org/1014703007/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context_impl.cc:17: void
EmptyFailureCallback() {}
On 2015/03/20 14:56:35, johnme wrote:
> Use DoNothing from base/bind_helpers.h instead.

Done.

https://codereview.chromium.org/1014703007/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context_impl.cc:57:
BrowserThread::PostTask(
On 2015/03/20 14:56:35, johnme wrote:
> My understanding is that there's now a race condition allowing the context to
be
> deleted on either the UI or IO threads. It seems this probably doesn't do any
> harm (since you ensure NotificationDatabase is destroyed on the right thread),
> but please add a comment* to the destructor documenting this, and please also
> add BrowserThread::DeleteOnUIThread as the traits of
> ServiceWorkerContextWrapper's RefCoountedThreadSafe superclass** (since
> ServiceWorkerContextWrapper's ServiceWorkerProcessManager seems to assume
it'll
> always be deleted on the UI thread).
> 
> OR, more simply, add BrowserThread::DeleteOnUIThread as the traits of
> PlatformNotificationContext's RefCountedThreadSafe superclass.
> 
> *: and/or a DCHECK:
> 
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO) ||
> BrowserThread::CurrentlyOn(BrowserThread::UI));
> 
> **: or (though this seems worse) reset service_worker_context_ in Shutdown().

Good point.

I have moved the RefCountedThreadSafe<> to PlatformNotificationContextImpl and
removed it from the public API, and added the BrowserThread::DeleteOnUIThread to
the definition.

Additionally, a new DCHECK has appeared in the destructor. This means that we
won't release a reference to the ServiceWorkerContextWrapper on any other thread
than the UI thread anymore, matching the StoragePartitionImpl semantics. Their
destruction semantics are their concern.

https://codereview.chromium.org/1014703007/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context_impl.cc:206: //
TODO(peter): Record UMA on status for deleting from the database.
On 2015/03/20 14:56:35, johnme wrote:
> Nit: add a matching "Do the DeleteAndStartOver dance for
> STATUS_ERROR_CORRUPTED." TODO?

Done.

https://codereview.chromium.org/1014703007/diff/1/content/browser/notificatio...
File content/browser/notifications/platform_notification_context_impl.h (right):

https://codereview.chromium.org/1014703007/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context_impl.h:32: //
defined in this interface must only be called on the IO thread.
On 2015/03/20 14:56:35, johnme wrote:
> Please add "unless otherwise specified".

Done.

https://codereview.chromium.org/1014703007/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context_impl.h:37: //
Constructs a new platform notification context. If |path| is non-empty, the
On 2015/03/20 14:56:35, johnme wrote:
> Please add "on the UI thread"

Done.

https://codereview.chromium.org/1014703007/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context_impl.h:42: const
scoped_refptr<ServiceWorkerContextWrapper>& service_worker_context);
On 2015/03/20 14:56:35, johnme wrote:
> Why not pass the scoped_refptr by value?

Per bauerb@'s suggestion in https://codereview.chromium.org/1006493005/.

https://codereview.chromium.org/1014703007/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context_impl.h:85: void
DoDeleteNotificationsForServiceWorkerRegistration(
On 2015/03/20 14:56:35, johnme wrote:
> Micro-nit: move this after DoDeleteNotificationData so it's easier to contrast
> them?

Done.

https://codereview.chromium.org/1014703007/diff/1/content/browser/notificatio...
File content/browser/notifications/platform_notification_context_unittest.cc
(right):

https://codereview.chromium.org/1014703007/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context_unittest.cc:254:
base::RunLoop().RunUntilIdle();
On 2015/03/20 14:56:36, johnme wrote:
> This smells flaky; ditto elsewhere. Why not pass the RunLoop's QuitClosure to
> DidWriteNotificationData?

Why would this be flaky? In these tests, the UI, IO and task runner threads are
a single thread :-).

https://codereview.chromium.org/1014703007/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context_unittest.cc:256:
ASSERT_TRUE(success());
On 2015/03/20 14:56:35, johnme wrote:
> Nit: EXPECT_TRUE seems more useful, unless this being false might cause the
test
> to timeout or crash horribly, as that way subsequent EXPECT/ASSERT statements
> will give you more information about the state of the test that might make it
> easier to debug failures.

Well, if we can't write a notification then the rest of the test doesn't make
sense anymore - there's nothing to delete when the Service Worker gets
unregistered!

[Avi (use Gerrit), 2015-03-20 19:05:59]

I am very much in favor of removing complexity from the API. If the embedder
doesn't need to know about the lifetime semantics of the context, why bother
them with it?

LGTM

[johnme, 2015-03-23 18:34:06]

On 2015/03/20 18:39:05, Peter Beverloo wrote:
> I have moved the RefCountedThreadSafe<> to PlatformNotificationContextImpl and
> removed it from the public API

Hmm, this means code in the chrome/ layer can no longer [safely] pass a
reference to PlatformNotificationContext from one thread to another.

Might be awkward later, but lgtm for now, and we can always change it back if
necessary.

[Peter Beverloo, 2015-03-27 01:35:34]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2015-03-27 01:35:34]

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

[commit-bot: I haz the power, 2015-03-27 01:36:04]

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

[commit-bot: I haz the power, 2015-03-27 03:06:53]

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

[commit-bot: I haz the power, 2015-03-27 03:06:54]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Peter Beverloo, 2015-03-28 00:49:17]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2015-03-28 00:49:17]

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

[commit-bot: I haz the power, 2015-03-28 00:49:25]

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

[commit-bot: I haz the power, 2015-03-28 01:37:00]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-28 01:38:24]

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