[Android, RemotePlayback] Make RemotePlayback classes ActiveScriptWrappable's

[Android, RemotePlayback] Make RemotePlayback classes ActiveScriptWrappable's

Keeps the V8 wrappers alive as long as there're event listeners or pending
promises and such.

BUG=616761
TEST=manual, see repro steps in the bug

Committed: https://crrev.com/10633dff101889d287c524266420f3b37dc7a23c
Cr-Commit-Position: refs/heads/master@{#398014}

[whywhat, 2016-06-02 15:02:49]

avayvod@chromium.org changed reviewers:
+ mlamouri@chromium.org

[whywhat, 2016-06-02 15:02:50]

PTaL

[whywhat, 2016-06-02 15:02:54]

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

[commit-bot: I haz the power, 2016-06-02 15:03:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2038573002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2038573002/1

[commit-bot: I haz the power, 2016-06-02 16:32:19]

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

[commit-bot: I haz the power, 2016-06-02 16:32:20]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-06-02 23:53:52]

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

[haraken, 2016-06-02 23:53:52]

https://codereview.chromium.org/2038573002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.h (right):

https://codereview.chromium.org/2038573002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.h:29: , public
LocalFrameLifecycleObserver

Would you help me understand why this needs to be LocalFrameLifecycleObserver?

[whywhat, 2016-06-03 13:27:49]

Changed RemotePlayback to be ContextLifecycleObserver

[whywhat, 2016-06-03 13:29:38]

PTaL

https://codereview.chromium.org/2038573002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.h (right):

https://codereview.chromium.org/2038573002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.h:29: , public
LocalFrameLifecycleObserver
On 2016/06/02 at 23:53:52, haraken wrote:
> Would you help me understand why this needs to be LocalFrameLifecycleObserver?

It was a natural switch from DOMWindowPropery to be able to access LocalFrame
(was m_frame, became frame()).
However, I only need it for executionContext() implementation so I switched to
ContextLifecycleObserver in the end.

[whywhat, 2016-06-03 13:29:42]

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

[commit-bot: I haz the power, 2016-06-03 13:29:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2038573002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2038573002/20001

[haraken, 2016-06-03 14:14:36]

https://codereview.chromium.org/2038573002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp
(right):

https://codereview.chromium.org/2038573002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:152: void
RemotePlayback::connectCancelled()

Not directly related to this CL, I think you need to call connectCancelled()
when ContextLifecycleObserver::stop() is called (i.e., when the execution
context stopped).

If you really need to have a pointer to ExecutionContext, you should just add
Member<ExecutionContext>.

[whywhat, 2016-06-03 14:32:02]

Resolve promises when context is destroyed.

[whywhat, 2016-06-03 14:33:45]

Thanks for the review!

https://codereview.chromium.org/2038573002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp
(right):

https://codereview.chromium.org/2038573002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:152: void
RemotePlayback::connectCancelled()
On 2016/06/03 at 14:14:36, haraken wrote:
> Not directly related to this CL, I think you need to call connectCancelled()
when ContextLifecycleObserver::stop() is called (i.e., when the execution
context stopped).

You mean LifecycleObserver::contextDestroyed() ? Done.

> 
> If you really need to have a pointer to ExecutionContext, you should just add
Member<ExecutionContext>.

I wish I knew what I really need. All I know is that executionContext() needs to
be implemented for EventTarget. I have no idea whether I need to keep the
context alive or observe it. Seems like the latter is what's used by other
modules.

Any clarifications are welcome :)

[whywhat, 2016-06-03 14:33:49]

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

[commit-bot: I haz the power, 2016-06-03 14:34:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2038573002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2038573002/40001

[haraken, 2016-06-03 14:36:11]

In terms of implementation, LGTM.

- EventTarget needs to implement executionContext().
- If you just need to have a strong reference to ExecutionContext, you should
just use Member<ExecutionContext>.
- If you need to observe contextDestroyed() to clean up something, you should
use ContextLifecycleObserver.

[whywhat, 2016-06-03 14:54:28]

On 2016/06/03 at 14:36:11, haraken wrote:
> In terms of implementation, LGTM.
> 
> - EventTarget needs to implement executionContext().
> - If you just need to have a strong reference to ExecutionContext, you should
just use Member<ExecutionContext>.
> - If you need to observe contextDestroyed() to clean up something, you should
use ContextLifecycleObserver.

The question then is if I need a strong reference to implement
executionContext().
Won't I "leak" the document by keeping a strong reference?
Should I just use WeakMember then?
Seems like I can't resolve promises in contextDestroyed() so I'll remove the
connectCancelled() call.

[whywhat, 2016-06-03 15:13:38]

Don't observe execution context, just return the element's document.

[whywhat, 2016-06-03 15:13:48]

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

[commit-bot: I haz the power, 2016-06-03 15:14:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2038573002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2038573002/60001

[whywhat, 2016-06-03 15:14:21]

On 2016/06/03 at 15:13:38, whywhat wrote:
> Don't observe execution context, just return the element's document.

Per discussion offline, we have a strong reference to the document via media
element anyway so just return element's document and don't use any lifecycle
observers.

[commit-bot: I haz the power, 2016-06-03 16:53:59]

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

[commit-bot: I haz the power, 2016-06-03 16:54:00]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-06-03 23:29:26]

LGTM

[mlamouri (slow - plz ping), 2016-06-04 02:02:38]

lgtm

[whywhat, 2016-06-06 11:11:55]

The CQ bit was checked by avayvod@chromium.org

[commit-bot: I haz the power, 2016-06-06 11:12:02]

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

[commit-bot: I haz the power, 2016-06-06 12:53:08]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-06 12:54:08]

Description was changed from

==========
[Android, RemotePlayback] Make RemotePlayback classes ActiveScriptWrappable's

Keeps the V8 wrappers alive as long as there're event listeners or pending
promises and such.

BUG=616761
TEST=manual, see repro steps in the bug
==========

to

==========
[Android, RemotePlayback] Make RemotePlayback classes ActiveScriptWrappable's

Keeps the V8 wrappers alive as long as there're event listeners or pending
promises and such.

BUG=616761
TEST=manual, see repro steps in the bug

Committed: https://crrev.com/10633dff101889d287c524266420f3b37dc7a23c
Cr-Commit-Position: refs/heads/master@{#398014}
==========

[commit-bot: I haz the power, 2016-06-06 12:54:09]

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