Remove ContextLifecycleObserver from PaymentAppManager

Remove ContextLifecycleObserver from PaymenAppManager

PaymenAppManager is inheriting from ContextLifecycleObserver just to
clear a service pointer (i.e., m_manager) when the context is destroyed.
This looks unnecessary, so this CL removes it and thus removes
ContextLifecycleObserver from PaymenAppManager.

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

[haraken, 2016-12-07 08:14:51]

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

[haraken, 2016-12-07 08:14:52]

PTAL

Other Mojo clients are not clearing service pointers in contextDestroyed(), so I
guess we won't need to reset m_manager in PaymentAppManager::contextDestroyed()
either. Correct me if I'm wrong.

[haraken, 2016-12-07 10:51:57]

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

[haraken, 2016-12-07 10:51:57]

+reillyg

I'm wondering if we need to clear the service ptr when the context is detached
on the Blink side.

[rwlbuis, 2016-12-07 13:34:39]

On 2016/12/07 08:14:52, haraken wrote:
> PTAL
> 
> Other Mojo clients are not clearing service pointers in contextDestroyed(), so
I
> guess we won't need to reset m_manager in
PaymentAppManager::contextDestroyed()
> either. Correct me if I'm wrong.

Note there is a small typo in Description -> "PaymenAppManager".

[please use gerrit instead, 2016-12-07 13:52:33]

rs lgtm. you're the expert in these matters, haraken.

[haraken, 2016-12-07 14:34:17]

On 2016/12/07 13:52:33, rouslan wrote:
> rs lgtm. you're the expert in these matters, haraken.

Actually I should ask Mojo experts (like reillyg@) if we need to clear service
pointers on a context detachment or not. If yes, we need to add
ContextLifecycleObserver to more things and take care of all service pointers.
If no, we can just land this CL.

[Reilly Grant (use Gerrit), 2016-12-07 19:56:25]

On 2016/12/07 at 14:34:17, haraken wrote:
> On 2016/12/07 13:52:33, rouslan wrote:
> > rs lgtm. you're the expert in these matters, haraken.
> 
> Actually I should ask Mojo experts (like reillyg@) if we need to clear service
pointers on a context detachment or not. If yes, we need to add
ContextLifecycleObserver to more things and take care of all service pointers.
If no, we can just land this CL.

You don't have to as long as you check for getExecutionContext() returning
nullptr inside any callbacks passed to Mojo methods. Since ScriptPromiseResolver
does this for you it is generally safe.

USBDevice closes pipes in contextDestroyed() because it wants to be sure that
the browser-side resources are freed immediately so that on page reload or
navigation there isn't a conflict between the old frame and the new frame. For
services where that isn't an issue it isn't necessary.

[haraken, 2016-12-08 02:29:24]

On 2016/12/07 19:56:25, Reilly Grant wrote:
> On 2016/12/07 at 14:34:17, haraken wrote:
> > On 2016/12/07 13:52:33, rouslan wrote:
> > > rs lgtm. you're the expert in these matters, haraken.
> > 
> > Actually I should ask Mojo experts (like reillyg@) if we need to clear
service
> pointers on a context detachment or not. If yes, we need to add
> ContextLifecycleObserver to more things and take care of all service pointers.
> If no, we can just land this CL.
> 
> You don't have to as long as you check for getExecutionContext() returning
> nullptr inside any callbacks passed to Mojo methods. Since
ScriptPromiseResolver
> does this for you it is generally safe.

Just to clarify: What you want to avoid is that Mojo calls Blink's C++ callback
and executes something after the context gets detached, right?

> 
> USBDevice closes pipes in contextDestroyed() because it wants to be sure that
> the browser-side resources are freed immediately so that on page reload or
> navigation there isn't a conflict between the old frame and the new frame. For
> services where that isn't an issue it isn't necessary.

Makes sense.

[haraken, 2016-12-08 02:29:27]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-12-08 02:30:17]

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

[Reilly Grant (use Gerrit), 2016-12-08 02:33:59]

On 2016/12/08 at 02:29:24, haraken wrote:
> On 2016/12/07 19:56:25, Reilly Grant wrote:
> > On 2016/12/07 at 14:34:17, haraken wrote:
> > > On 2016/12/07 13:52:33, rouslan wrote:
> > > > rs lgtm. you're the expert in these matters, haraken.
> > > 
> > > Actually I should ask Mojo experts (like reillyg@) if we need to clear
service
> > pointers on a context detachment or not. If yes, we need to add
> > ContextLifecycleObserver to more things and take care of all service
pointers.
> > If no, we can just land this CL.
> > 
> > You don't have to as long as you check for getExecutionContext() returning
> > nullptr inside any callbacks passed to Mojo methods. Since
ScriptPromiseResolver
> > does this for you it is generally safe.
> 
> Just to clarify: What you want to avoid is that Mojo calls Blink's C++
callback and executes something after the context gets detached, right?

Exactly. It's okay to execute something, just make sure that it doesn't assume
the context is still attached.

[commit-bot: I haz the power, 2016-12-08 03:57:40]

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

[commit-bot: I haz the power, 2016-12-08 03:57:41]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[haraken, 2016-12-08 04:09:02]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-12-08 04:09:31]

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-08 05:37:00]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1481170142811150, "parent_rev":
"c2115cfd1bedd5dfe70cbd0796c0d81bcb570bbc", "commit_rev":
"e28bdda77338663a99a70cea024bb16f7ec5fb68"}

[commit-bot: I haz the power, 2016-12-08 05:37:27]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-12-08 05:39:18]

Description was changed from

==========
Remove ContextLifecycleObserver from PaymenAppManager

PaymenAppManager is inheriting from ContextLifecycleObserver just to
clear a service pointer (i.e., m_manager) when the context is destroyed.
This looks unnecessary, so this CL removes it and thus removes
ContextLifecycleObserver from PaymenAppManager.

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

to

==========
Remove ContextLifecycleObserver from PaymenAppManager

PaymenAppManager is inheriting from ContextLifecycleObserver just to
clear a service pointer (i.e., m_manager) when the context is destroyed.
This looks unnecessary, so this CL removes it and thus removes
ContextLifecycleObserver from PaymenAppManager.

BUG=610176

Committed: https://crrev.com/66af5a98e7073a40f05ab8828ea135e6e61dd5b5
Cr-Commit-Position: refs/heads/master@{#437189}
==========

[commit-bot: I haz the power, 2016-12-08 05:39:19]

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