Remove ContextLifecycleObserver from NotificationManager

Remove ContextLifecycleObserver from NotificationManager

Currently NotificationManager::contextDestroyed is resetting service's pointers
but it looks unnecessary. This CL removes the code and stop inheriting from
ContextLifecycleObserver.

BUG=610176
Committed: https://crrev.com/abb83eaffd249c61928d3f1207054c24f5d0b56d
Cr-Commit-Position: refs/heads/master@{#437022}

[haraken, 2016-12-07 06:56:06]

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

[haraken, 2016-12-07 06:56:07]

PTAL

https://codereview.chromium.org/2557033002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/notifications/NotificationManager.cpp
(left):

https://codereview.chromium.org/2557033002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/notifications/NotificationManager.cpp:92:
m_permissionService.reset();

As far as I understand, there is no benefit in resetting the service pointers.
It looks like that most Mojo clients are not doing the resetting.

[Peter Beverloo, 2016-12-07 13:25:37]

lgtm

https://codereview.chromium.org/2557033002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/notifications/NotificationManager.h
(right):

https://codereview.chromium.org/2557033002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/notifications/NotificationManager.h:39:
mojom::blink::PermissionStatus permissionStatus(ExecutionContext*);
For future reference -

All potential methods that fit in this class (which supersedes the
WebNotificationManager) will need access to at least the SecurityOrigin, that
lives off ExecutionContext.

Is it safe to store a RefPtr<> to the SecurityOrigin set once in the
constructor, retrieved from the ExecutionContext that we supplement, and use
that instead of passing it explicitly to every call?

[haraken, 2016-12-07 15:27:13]

https://codereview.chromium.org/2557033002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/notifications/NotificationManager.h
(right):

https://codereview.chromium.org/2557033002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/notifications/NotificationManager.h:39:
mojom::blink::PermissionStatus permissionStatus(ExecutionContext*);
On 2016/12/07 13:25:36, Peter Beverloo wrote:
> For future reference -
> 
> All potential methods that fit in this class (which supersedes the
> WebNotificationManager) will need access to at least the SecurityOrigin, that
> lives off ExecutionContext.
> 
> Is it safe to store a RefPtr<> to the SecurityOrigin set once in the
> constructor, retrieved from the ExecutionContext that we supplement, and use
> that instead of passing it explicitly to every call?

It should be safe. The fact that NotificationManager is
Supplement<ExecutionContext> means that NotificationManager is "a part of"
ExecutionContext. The ExecutionContext should not change over the lifetime of
NotificationManager.

[haraken, 2016-12-07 15:27:17]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-12-07 15:27:43]

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-12-07 17:40:16]

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

[commit-bot: I haz the power, 2016-12-07 17:40:17]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Peter Beverloo, 2016-12-07 17:42:28]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2016-12-07 17:42:58]

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-12-07 19:01:07]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1481132548863790, "parent_rev":
"526e8f9f985d68bc765765be9c1bbfbd658f7c46", "commit_rev":
"f82efa481c4dc67627b794a3ec5c3c5d7c28a30b"}

[commit-bot: I haz the power, 2016-12-07 19:01:35]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-12-07 19:03:53]

Description was changed from

==========
Remove ContextLifecycleObserver from NotificationManager

Currently NotificationManager::contextDestroyed is resetting service's pointers
but it looks unnecessary. This CL removes the code and stop inheriting from
ContextLifecycleObserver.

BUG=610176
==========

to

==========
Remove ContextLifecycleObserver from NotificationManager

Currently NotificationManager::contextDestroyed is resetting service's pointers
but it looks unnecessary. This CL removes the code and stop inheriting from
ContextLifecycleObserver.

BUG=610176

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

[commit-bot: I haz the power, 2016-12-07 19:03:55]

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