[Blink, RemotePlaybackAPI] Create WebRemotePlaybackClient in HTMLMediaElement ctor to avoid lazy in…

[Blink, RemotePlaybackAPI] Create WebRemotePlaybackClient in HTMLMediaElement ctor to avoid lazy initialization issues

Split off from https://codereview.chromium.org/2480003002 to make it smaller.

BUG=None
TEST=existing tests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/1775f467005c76d7b432fb59e25843931f9c52cf
Cr-Commit-Position: refs/heads/master@{#430864}

[whywhat, 2016-11-08 22:17:01]

Description was changed from

==========
[Blink, RemotePlaybackAPI] Create WebRemotePlaybackClient in HTMLMediaElement
ctor to avoid lazy initialization issues

Split off from https://codereview.chromium.org/2480003002 to make it smaller.

BUG=None
TEST=existing tests
==========

to

==========
[Blink, RemotePlaybackAPI] Create WebRemotePlaybackClient in HTMLMediaElement
ctor to avoid lazy initialization issues

Split off from https://codereview.chromium.org/2480003002 to make it smaller.

BUG=None
TEST=existing tests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[whywhat, 2016-11-08 22:26:42]

Fixed compile for EmptyClients.cpp

[whywhat, 2016-11-08 22:27:34]

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

[whywhat, 2016-11-08 22:27:35]

PTaL

[haraken, 2016-11-08 23:50:40]

https://codereview.chromium.org/2484973005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2484973005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:482: ScriptState*
scriptState = ScriptState::from(isolate->GetCurrentContext());

You shouldn't use isolate->GetCurrentContext() unless you're pretty sure that
this code is always called while you're already in a correct context.

Instead, you can pass in a ScriptState* parameter using
[ConstructorCallWith=ScriptState].

(Actually passing ScriptState* to HTMLMediaElement and then to RemotePlayback is
wrong for a complicated reason
(https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/nB...),
but that's a separate discussion.)

[whywhat, 2016-11-09 00:22:02]

https://codereview.chromium.org/2484973005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2484973005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:482: ScriptState*
scriptState = ScriptState::from(isolate->GetCurrentContext());
On 2016/11/08 at 23:50:40, haraken wrote:
> You shouldn't use isolate->GetCurrentContext() unless you're pretty sure that
this code is always called while you're already in a correct context.

Tests actually crash because context seems to be empty when the document is
being parsed and HTMLMediaElement is being created.

> 
> Instead, you can pass in a ScriptState* parameter using
[ConstructorCallWith=ScriptState].
> 
> (Actually passing ScriptState* to HTMLMediaElement and then to RemotePlayback
is wrong for a complicated reason
(https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/nB...),
but that's a separate discussion.)

So in my case I only keep ScriptState to run an async callback. Should I try to
pass ScriptState to the callback then? Does ScriptState support being passed
around like a bound object?

[haraken, 2016-11-09 00:53:26]

On 2016/11/09 00:22:02, whywhat wrote:
>
https://codereview.chromium.org/2484973005/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):
> 
>
https://codereview.chromium.org/2484973005/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:482: ScriptState*
> scriptState = ScriptState::from(isolate->GetCurrentContext());
> On 2016/11/08 at 23:50:40, haraken wrote:
> > You shouldn't use isolate->GetCurrentContext() unless you're pretty sure
that
> this code is always called while you're already in a correct context.
> 
> Tests actually crash because context seems to be empty when the document is
> being parsed and HTMLMediaElement is being created.
> 
> > 
> > Instead, you can pass in a ScriptState* parameter using
> [ConstructorCallWith=ScriptState].
> > 
> > (Actually passing ScriptState* to HTMLMediaElement and then to
RemotePlayback
> is wrong for a complicated reason
>
(https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/nB...),
> but that's a separate discussion.)
> 
> So in my case I only keep ScriptState to run an async callback. Should I try
to
> pass ScriptState to the callback then? Does ScriptState support being passed
> around like a bound object?

Yes, that's an ideal solution (rather than storing ScriptState on
RemoteCallback).

[whywhat, 2016-11-09 01:59:06]

On 2016/11/09 at 00:53:26, haraken wrote:
> On 2016/11/09 00:22:02, whywhat wrote:
> >
https://codereview.chromium.org/2484973005/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):
> > 
> >
https://codereview.chromium.org/2484973005/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:482: ScriptState*
> > scriptState = ScriptState::from(isolate->GetCurrentContext());
> > On 2016/11/08 at 23:50:40, haraken wrote:
> > > You shouldn't use isolate->GetCurrentContext() unless you're pretty sure
that
> > this code is always called while you're already in a correct context.
> > 
> > Tests actually crash because context seems to be empty when the document is
> > being parsed and HTMLMediaElement is being created.
> > 
> > > 
> > > Instead, you can pass in a ScriptState* parameter using
> > [ConstructorCallWith=ScriptState].
> > > 
> > > (Actually passing ScriptState* to HTMLMediaElement and then to
RemotePlayback
> > is wrong for a complicated reason
> >
(https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/nB...),
> > but that's a separate discussion.)
> > 
> > So in my case I only keep ScriptState to run an async callback. Should I try
to
> > pass ScriptState to the callback then? Does ScriptState support being passed
> > around like a bound object?
> 
> Yes, that's an ideal solution (rather than storing ScriptState on
RemoteCallback).

Any guidance about passing RefCounted object to a task? I remember not being
able to find a way to do it hence keeping the reference in RemotePlayback.
https://webkit.org/blog/5381/refptr-basics/ is still the most up-to-date
out-of-date docs about it AFAIK :/

[haraken, 2016-11-09 02:02:06]

On 2016/11/09 01:59:06, whywhat wrote:
> On 2016/11/09 at 00:53:26, haraken wrote:
> > On 2016/11/09 00:22:02, whywhat wrote:
> > >
>
https://codereview.chromium.org/2484973005/diff/20001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):
> > > 
> > >
>
https://codereview.chromium.org/2484973005/diff/20001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:482: ScriptState*
> > > scriptState = ScriptState::from(isolate->GetCurrentContext());
> > > On 2016/11/08 at 23:50:40, haraken wrote:
> > > > You shouldn't use isolate->GetCurrentContext() unless you're pretty sure
> that
> > > this code is always called while you're already in a correct context.
> > > 
> > > Tests actually crash because context seems to be empty when the document
is
> > > being parsed and HTMLMediaElement is being created.
> > > 
> > > > 
> > > > Instead, you can pass in a ScriptState* parameter using
> > > [ConstructorCallWith=ScriptState].
> > > > 
> > > > (Actually passing ScriptState* to HTMLMediaElement and then to
> RemotePlayback
> > > is wrong for a complicated reason
> > >
>
(https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/nB...),
> > > but that's a separate discussion.)
> > > 
> > > So in my case I only keep ScriptState to run an async callback. Should I
try
> to
> > > pass ScriptState to the callback then? Does ScriptState support being
passed
> > > around like a bound object?
> > 
> > Yes, that's an ideal solution (rather than storing ScriptState on
> RemoteCallback).
> 
> Any guidance about passing RefCounted object to a task? I remember not being
> able to find a way to do it hence keeping the reference in RemotePlayback.
> https://webkit.org/blog/5381/refptr-basics/ is still the most up-to-date
> out-of-date docs about it AFAIK :/

Can you just make the object a GarbageCollected and pass in
wrapPersistent(object) to postTask?

[whywhat, 2016-11-09 02:07:38]

On 2016/11/09 at 01:59:06, whywhat wrote:
> On 2016/11/09 at 00:53:26, haraken wrote:
> > On 2016/11/09 00:22:02, whywhat wrote:
> > >
https://codereview.chromium.org/2484973005/diff/20001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):
> > > 
> > >
https://codereview.chromium.org/2484973005/diff/20001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:482: ScriptState*
> > > scriptState = ScriptState::from(isolate->GetCurrentContext());
> > > On 2016/11/08 at 23:50:40, haraken wrote:
> > > > You shouldn't use isolate->GetCurrentContext() unless you're pretty sure
that
> > > this code is always called while you're already in a correct context.
> > > 
> > > Tests actually crash because context seems to be empty when the document
is
> > > being parsed and HTMLMediaElement is being created.
> > > 
> > > > 
> > > > Instead, you can pass in a ScriptState* parameter using
> > > [ConstructorCallWith=ScriptState].
> > > > 
> > > > (Actually passing ScriptState* to HTMLMediaElement and then to
RemotePlayback
> > > is wrong for a complicated reason
> > >
(https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/nB...),
> > > but that's a separate discussion.)
> > > 
> > > So in my case I only keep ScriptState to run an async callback. Should I
try to
> > > pass ScriptState to the callback then? Does ScriptState support being
passed
> > > around like a bound object?
> > 
> > Yes, that's an ideal solution (rather than storing ScriptState on
RemoteCallback).
> 
> Any guidance about passing RefCounted object to a task? I remember not being
able to find a way to do it hence keeping the reference in RemotePlayback.
> https://webkit.org/blog/5381/refptr-basics/ is still the most up-to-date
out-of-date docs about it AFAIK :/

Actually I don't need the ScriptState in the callback, not sure why I needed it
then o_O

[whywhat, 2016-11-09 02:21:44]

Removed m_scriptState from RemotePlayback

[whywhat, 2016-11-09 02:37:04]

PTAL

[whywhat, 2016-11-09 02:37:08]

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

[commit-bot: I haz the power, 2016-11-09 02:37:39]

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

[haraken, 2016-11-09 02:46:41]

LGTM

https://codereview.chromium.org/2484973005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2484973005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:99: #include <v8.h>

Remove this.

[whywhat, 2016-11-09 03:05:48]

Removed <v8.h> from HTMLMediaElement.cpp

[whywhat, 2016-11-09 03:06:37]

Thanks for the reviews!

https://codereview.chromium.org/2484973005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2484973005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:99: #include <v8.h>
On 2016/11/09 at 02:46:41, haraken wrote:
> Remove this.

Done.

[whywhat, 2016-11-09 03:06:49]

The CQ bit was checked by avayvod@chromium.org

[whywhat, 2016-11-09 03:06:50]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2484973005/#ps60001
(title: "Removed <v8.h> from HTMLMediaElement.cpp")

[commit-bot: I haz the power, 2016-11-09 03:07:20]

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-11-09 04:33:56]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-09 04:36:28]

Description was changed from

==========
[Blink, RemotePlaybackAPI] Create WebRemotePlaybackClient in HTMLMediaElement
ctor to avoid lazy initialization issues

Split off from https://codereview.chromium.org/2480003002 to make it smaller.

BUG=None
TEST=existing tests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
[Blink, RemotePlaybackAPI] Create WebRemotePlaybackClient in HTMLMediaElement
ctor to avoid lazy initialization issues

Split off from https://codereview.chromium.org/2480003002 to make it smaller.

BUG=None
TEST=existing tests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/1775f467005c76d7b432fb59e25843931f9c52cf
Cr-Commit-Position: refs/heads/master@{#430864}
==========

[commit-bot: I haz the power, 2016-11-09 04:36:29]

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