bindings: Store ScriptState in generated callback functions

bindings: Store ScriptState in generated callback functions

As the spec says[1], we should store "callback context" (which is ScriptState
in Blink) when creating callback impl instances and use it to invoke
v8 functions, instead of passing ScriptState at invoke time.

[1] https://heycam.github.io/webidl/#es-callback-function

BUG=618672
Committed: https://crrev.com/1c5aa5f5af6a2246bb55a29862e6d9f42256c43a
Cr-Commit-Position: refs/heads/master@{#427918}

[bashi, 2016-10-27 00:49:33]

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

[commit-bot: I haz the power, 2016-10-27 00:50:19]

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

[bashi, 2016-10-27 00:52:20]

bashi@chromium.org changed reviewers:
+ haraken@chromium.org, peria@chromium.org, yukishiino@chromium.org

[bashi, 2016-10-27 00:52:21]

PTAL.

I didn't remove m_scriptState from PerformanceObserver and RemotePlayback
because m_scriptState is used not only for callbacks.

[bashi, 2016-10-27 00:56:23]

Description was changed from

==========
bindings: Store ScriptState in generated callback functions

As the spec says[1], we should store "callback context" (which is ScriptState
in Blink) when creating callback impl instances and use it to invoke
v8 functions, instead of passing ScriptState at invoke time.

BUG=618672
==========

to

==========
bindings: Store ScriptState in generated callback functions

As the spec says[1], we should store "callback context" (which is ScriptState
in Blink) when creating callback impl instances and use it to invoke
v8 functions, instead of passing ScriptState at invoke time.

[1] https://heycam.github.io/webidl/#es-callback-function

BUG=618672
==========

[haraken, 2016-10-27 01:02:34]

LGTM

Yes, this should make the context callback functions use more correct.

> I didn't remove m_scriptState from PerformanceObserver and RemotePlayback
> because m_scriptState is used not only for callbacks.

Actually they are bugs for the reason I wrote in platform-architecture-dev@
today...

[bashi, 2016-10-27 01:08:02]

On 2016/10/27 01:02:34, haraken wrote:
> LGTM
> 
> Yes, this should make the context callback functions use more correct.
> 
> > I didn't remove m_scriptState from PerformanceObserver and RemotePlayback
> > because m_scriptState is used not only for callbacks.
> 
> Actually they are bugs for the reason I wrote in platform-architecture-dev@
> today...

Yeah, I tried to remove them but wasn't as easy as I expected :( Follow-up CLs
should remove them I think.

[commit-bot: I haz the power, 2016-10-27 02:06:17]

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

[commit-bot: I haz the power, 2016-10-27 02:06:18]

Dry run: This issue passed the CQ dry run.

[bashi, 2016-10-27 02:10:53]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2016-10-27 02:11:22]

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-10-27 02:17:50]

Description was changed from

==========
bindings: Store ScriptState in generated callback functions

As the spec says[1], we should store "callback context" (which is ScriptState
in Blink) when creating callback impl instances and use it to invoke
v8 functions, instead of passing ScriptState at invoke time.

[1] https://heycam.github.io/webidl/#es-callback-function

BUG=618672
==========

to

==========
bindings: Store ScriptState in generated callback functions

As the spec says[1], we should store "callback context" (which is ScriptState
in Blink) when creating callback impl instances and use it to invoke
v8 functions, instead of passing ScriptState at invoke time.

[1] https://heycam.github.io/webidl/#es-callback-function

BUG=618672
==========

[commit-bot: I haz the power, 2016-10-27 02:17:51]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-10-27 02:21:53]

Description was changed from

==========
bindings: Store ScriptState in generated callback functions

As the spec says[1], we should store "callback context" (which is ScriptState
in Blink) when creating callback impl instances and use it to invoke
v8 functions, instead of passing ScriptState at invoke time.

[1] https://heycam.github.io/webidl/#es-callback-function

BUG=618672
==========

to

==========
bindings: Store ScriptState in generated callback functions

As the spec says[1], we should store "callback context" (which is ScriptState
in Blink) when creating callback impl instances and use it to invoke
v8 functions, instead of passing ScriptState at invoke time.

[1] https://heycam.github.io/webidl/#es-callback-function

BUG=618672

Committed: https://crrev.com/1c5aa5f5af6a2246bb55a29862e6d9f42256c43a
Cr-Commit-Position: refs/heads/master@{#427918}
==========

[commit-bot: I haz the power, 2016-10-27 02:21:54]

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

[Yuki, 2016-10-27 12:16:30]

I don't think we need to revert this CL immediately, but there seems
misunderstanding.

https://heycam.github.io/webidl/#es-callback-function
is saying that we need to save the incumbent settings object as the callback
context.
1. Neither of relevant realm nor current realm is the incumbent real in general.
You're saving a wrong settings object in this CL.

https://html.spec.whatwg.org/multipage/webappapis.html#prepare-to-run-a-callback
2. The callback context should be used to restore the incumbent settings object.
Shouldn't be used for any other purpose, like throwing an exception or creating
wrapper objects.

3. ScriptState::current is obsolete.

[bashi, 2016-10-27 22:08:27]

On 2016/10/27 12:16:30, Yuki wrote:
> I don't think we need to revert this CL immediately, but there seems
> misunderstanding.
> 
> https://heycam.github.io/webidl/#es-callback-function
> is saying that we need to save the incumbent settings object as the callback
> context.
> 1. Neither of relevant realm nor current realm is the incumbent real in
general.
> You're saving a wrong settings object in this CL.
> 
>
https://html.spec.whatwg.org/multipage/webappapis.html#prepare-to-run-a-callback
> 2. The callback context should be used to restore the incumbent settings
object.
> Shouldn't be used for any other purpose, like throwing an exception or
creating
> wrapper objects.
> 
> 3. ScriptState::current is obsolete.

Could you tell me the right fix? It's very confusing and I have no idea why this
is wrong.

Also, do we have documentation which descrives why current() is obsolete and
alternatives?

[bashi, 2016-10-27 23:25:18]

I've just read the thread you guys were discussing the issue. Still I couldn't
get a right solution:( I'd like to chat offline to understand what's we should
do in general (in next week).

[Yuki, 2016-10-28 09:25:27]

On 2016/10/27 23:25:18, bashi1 wrote:
> I've just read the thread you guys were discussing the issue. Still I couldn't
> get a right solution:( I'd like to chat offline to understand what's we should
> do in general (in next week).

Yes, let's discuss offline.