Move ScriptState::forWorld/ScriptState::forMainWorld (Part 1)

Move ScriptState::forWorld/ScriptState::forMainWorld (Part 1)

To move ScriptState to platform/bindings, ScriptState::forWorld/ScriptState::forMainWorld need to be moved out of ScriptState.h/cpp (they depend on Frame). This CL creates two new functions in V8Binding, and replaces uses of ScriptState::forWorld with toScriptState.

BUG=682322
Review-Url: https://codereview.chromium.org/2794023002
Cr-Commit-Position: refs/heads/master@{#461796}
Committed: https://chromium.googlesource.com/chromium/src/+/cd81626138da7e7021b81a1415a06088302e958f

[adithyas, 2017-04-03 16:48:52]

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

[commit-bot: I haz the power, 2017-04-03 16:49:23]

Dry run: 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, 2017-04-03 18:54:02]

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

[commit-bot: I haz the power, 2017-04-03 18:54:03]

Dry run: This issue passed the CQ dry run.

[adithyas, 2017-04-03 19:09:03]

adithyas@chromium.org changed reviewers:
+ jbroman@chromium.org

[adithyas, 2017-04-03 19:09:04]

This CL only adds the new functions to a separate file (without removing the old
functions). I'll update remaining call sites & remove those functions in follow
up CLs (I wanted to bikeshed names/which file to move these functions too before
updating other files).

[jbroman, 2017-04-03 19:23:43]

https://codereview.chromium.org/2794023002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/ScriptStateStore.h (right):

https://codereview.chromium.org/2794023002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScriptStateStore.h:8: #include
"bindings/core/v8/DOMWrapperWorld.h"
This can be forward-declared as well.

https://codereview.chromium.org/2794023002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScriptStateStore.h:21: static
ScriptState* forWorld(LocalFrame*, DOMWrapperWorld&);
I think I'd put this next to toV8Context in V8Binding.h, as they do analogous
things.

We could call it "toScriptState". While there, you could probably refactor this
and toV8Context, as currently toV8Context already has the ScriptState*
internally, so hopping to a v8::Context and then back to ScriptState seems
silly.

[adithyas, 2017-04-03 20:12:30]

Description was changed from

==========
Move ScriptState::forWorld/ScriptState::forMainWorld (Part 1)

To move ScriptState to platform/bindings,
ScriptState::forWorld/ScriptState::forMainWorld need to be moved out of
ScriptState.h/cpp (they depend on Frame). This CL creates a new file
(ScriptStateStore) that holds these two functions, and replaces uses of
ScriptState::forWorld with ScriptStateStore::forWorld

BUG=682322
==========

to

==========
Move ScriptState::forWorld/ScriptState::forMainWorld (Part 1)

To move ScriptState to platform/bindings,
ScriptState::forWorld/ScriptState::forMainWorld need to be moved out of
ScriptState.h/cpp (they depend on Frame). This CL creates two new functions in
V8Binding, and replaces uses of ScriptState::forWorld with toScriptState.

BUG=682322
==========

[adithyas, 2017-04-03 21:42:00]

https://codereview.chromium.org/2794023002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/ScriptStateStore.h (right):

https://codereview.chromium.org/2794023002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScriptStateStore.h:21: static
ScriptState* forWorld(LocalFrame*, DOMWrapperWorld&);
On 2017/04/03 at 19:23:42, jbroman wrote:
> I think I'd put this next to toV8Context in V8Binding.h, as they do analogous
things.
> 
> We could call it "toScriptState". While there, you could probably refactor
this and toV8Context, as currently toV8Context already has the ScriptState*
internally, so hopping to a v8::Context and then back to ScriptState seems
silly.

Okay, moved it to V8Binding. As for the refactoring, I made toV8PerContext call
toScriptState (instead of the other way around). This still involves a
conversion to ScriptState and then back to a context object, but I think
converting it to a ScriptState at least once is necessary (to check if the
context is valid).

[jbroman, 2017-04-04 01:16:28]

(cc haraken in case he has any objections)

This lgtm. It's analogous to how we get other related objects from a
LocalFrame*, and it seems to extricate ScriptState from frames.

https://codereview.chromium.org/2794023002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptState.h (right):

https://codereview.chromium.org/2794023002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptState.h:142: // DEPRECATED: Use
toScriptStateForMainWorld.
nit: make the implementations of these use toScriptState until this goes away?

[haraken, 2017-04-04 02:26:44]

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

[haraken, 2017-04-04 02:26:45]

LGTM

https://codereview.chromium.org/2794023002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/2794023002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8Binding.h:1010: CORE_EXPORT
ScriptState* toScriptStateForMainWorld(LocalFrame*);

Nit: I'd add:

  // Do not use this method unless you're pretty sure that you should use the
main world's ScriptState.

People have sometimes abused this method...

[adithyas, 2017-04-04 14:22:41]

The CQ bit was checked by adithyas@chromium.org

[adithyas, 2017-04-04 14:22:42]

https://codereview.chromium.org/2794023002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptState.h (right):

https://codereview.chromium.org/2794023002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptState.h:142: // DEPRECATED: Use
toScriptStateForMainWorld.
On 2017/04/04 at 01:16:28, jbroman wrote:
> nit: make the implementations of these use toScriptState until this goes away?

Done.

https://codereview.chromium.org/2794023002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/2794023002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8Binding.h:1010: CORE_EXPORT
ScriptState* toScriptStateForMainWorld(LocalFrame*);
On 2017/04/04 at 02:26:44, haraken wrote:
> Nit: I'd add:
> 
>   // Do not use this method unless you're pretty sure that you should use the
main world's ScriptState.
> 
> People have sometimes abused this method...

Done.

[adithyas, 2017-04-04 14:22:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2794023002/#ps2 (title:
"Code review changes")

[commit-bot: I haz the power, 2017-04-04 14:22: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, 2017-04-04 15:38:21]

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

[commit-bot: I haz the power, 2017-04-04 15:38:22]

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_...)

[adithyas, 2017-04-04 18:34:35]

The CQ bit was checked by adithyas@chromium.org

[commit-bot: I haz the power, 2017-04-04 18:35:01]

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, 2017-04-04 19:10:21]

CQ is committing da patch.

Bot data: {"patchset_id": 2, "attempt_start_ts": 1491330875326020, "parent_rev":
"aa950cad2761723ce5586bf8fe086ea8d54fad91", "commit_rev":
"cd81626138da7e7021b81a1415a06088302e958f"}

[commit-bot: I haz the power, 2017-04-04 19:11:09]

Description was changed from

==========
Move ScriptState::forWorld/ScriptState::forMainWorld (Part 1)

To move ScriptState to platform/bindings,
ScriptState::forWorld/ScriptState::forMainWorld need to be moved out of
ScriptState.h/cpp (they depend on Frame). This CL creates two new functions in
V8Binding, and replaces uses of ScriptState::forWorld with toScriptState.

BUG=682322
==========

to

==========
Move ScriptState::forWorld/ScriptState::forMainWorld (Part 1)

To move ScriptState to platform/bindings,
ScriptState::forWorld/ScriptState::forMainWorld need to be moved out of
ScriptState.h/cpp (they depend on Frame). This CL creates two new functions in
V8Binding, and replaces uses of ScriptState::forWorld with toScriptState.

BUG=682322

Review-Url: https://codereview.chromium.org/2794023002
Cr-Commit-Position: refs/heads/master@{#461796}
Committed:
https://chromium.googlesource.com/chromium/src/+/cd81626138da7e7021b81a1415a0...
==========

[commit-bot: I haz the power, 2017-04-04 19:11:10]

Committed patchset #4 (id:2) as
https://chromium.googlesource.com/chromium/src/+/cd81626138da7e7021b81a1415a0...

[Yuki, 2017-04-05 08:06:18]

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

[Yuki, 2017-04-05 08:06:19]

LGTM.