Abstract out Modulator from V8PerContextData

Abstract out Modulator from V8PerContextData

To move V8PerContextData to platform/bindings, we need to remove its dependency to Modulator. This CL creates a general abstraction for V8PerContextData to hold instances of garbage collected objects and makes Modulator use this abstraction.

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

[adithyas, 2017-04-04 17:55:04]

adithyas@chromium.org changed reviewers:
+ haraken@chromium.org, jbroman@chromium.org, kouhei@chromium.org

[esprehn, 2017-04-04 18:11:56]

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

[esprehn, 2017-04-04 18:11:57]

It seems like maybe this belongs in platform/bindings instead?

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Modul...
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Scrip...
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Modul...

All this code looks like it's a core concept of module loading we can put in
platorm/ for loading modules. I don't see any reason we'd need a dependency on
LocalFrame or stuff in core/ here.

[adithyas, 2017-04-04 20:38:26]

On 2017/04/04 at 18:11:57, esprehn wrote:
> It seems like maybe this belongs in platform/bindings instead?
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Modul...
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Scrip...
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Modul...
> 
> All this code looks like it's a core concept of module loading we can put in
platorm/ for loading modules. I don't see any reason we'd need a dependency on
LocalFrame or stuff in core/ here.

I don't know why Modulator has get/set methods that need LocalFrame* (the
methods are not used yet). Modulator does use ScriptModule in bindings (and in
fact, ScriptModule uses Modulator as well). I wasn't planning on moving
ScriptModule to platform/bindings right now, so this CL just focuses on
decoupling V8PerContextData from Modulator. I do think it's worth considering if
Modulator (and the other modules related classes) should be in bindings/core
(instead of core/dom) first. Once we make that move, maybe then it could move to
platform/bindings along with ScriptModule.

[jbroman, 2017-04-04 21:16:48]

On 2017/04/04 at 20:38:26, adithyas wrote:
> On 2017/04/04 at 18:11:57, esprehn wrote:
> > It seems like maybe this belongs in platform/bindings instead?
> > 
> >
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Modul...
> >
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Scrip...
> >
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Modul...
> > 
> > All this code looks like it's a core concept of module loading we can put in
platorm/ for loading modules. I don't see any reason we'd need a dependency on
LocalFrame or stuff in core/ here.
> 
> I don't know why Modulator has get/set methods that need LocalFrame* (the
methods are not used yet). Modulator does use ScriptModule in bindings (and in
fact, ScriptModule uses Modulator as well). I wasn't planning on moving
ScriptModule to platform/bindings right now, so this CL just focuses on
decoupling V8PerContextData from Modulator. I do think it's worth considering if
Modulator (and the other modules related classes) should be in bindings/core
(instead of core/dom) first. Once we make that move, maybe then it could move to
platform/bindings along with ScriptModule.

Maybe kouhei@ can provide context for whether Modulator will require core/
concepts. It's not apparent to me (or esprehn) that it will, in which case it
might be appropriate to move them over. So far, Modulator, ScriptModule,
ModuleScript, etc. don't seem to use core concepts at all, except for looking up
by LocalFrame* (which is interesting, as it implies that it's main-world only,
in which case I'm not sure whether it belongs on V8PerContextData).

[kouhei (in TOK), 2017-04-05 00:50:49]

PS4 lgtm if haraken is OK.

On 2017/04/04 21:16:48, jbroman wrote:
> On 2017/04/04 at 20:38:26, adithyas wrote:
> > On 2017/04/04 at 18:11:57, esprehn wrote:
> > > It seems like maybe this belongs in platform/bindings instead?
> > > 
> > >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Modul...
> > >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Scrip...
> > >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Modul...
> > > 
> > > All this code looks like it's a core concept of module loading we can put
in
> platorm/ for loading modules. I don't see any reason we'd need a dependency on
> LocalFrame or stuff in core/ here.
> > 
> > I don't know why Modulator has get/set methods that need LocalFrame* (the
> methods are not used yet). Modulator does use ScriptModule in bindings (and in
> fact, ScriptModule uses Modulator as well). I wasn't planning on moving
> ScriptModule to platform/bindings right now, so this CL just focuses on
> decoupling V8PerContextData from Modulator. I do think it's worth considering
if
> Modulator (and the other modules related classes) should be in bindings/core
> (instead of core/dom) first. Once we make that move, maybe then it could move
to
> platform/bindings along with ScriptModule.
This will be queried from core/dom/ScriptLoader.cpp to find its Modulator
attached to its LocalFrame.
> 
> Maybe kouhei@ can provide context for whether Modulator will require core/
> concepts. It's not apparent to me (or esprehn) that it will, in which case it
> might be appropriate to move them over. So far, Modulator, ScriptModule,
> ModuleScript, etc. don't seem to use core concepts at all, except for looking
up
> by LocalFrame* (which is interesting, as it implies that it's main-world only,
> in which case I'm not sure whether it belongs on V8PerContextData).

Please see https://codereview.chromium.org/2555653002/ and
bit.ly/modules-in-blink for more context.
I'm not 100% sure what is core/ concept in post-framification world, but it
doesn't heavily rely on DOM now.

For ScriptValue, I think it makes sense to place in the same dir as
Script{Value,Promise}.
For ModuleScript and Modulator, they are fairly isolated from bindings logic so
I'm not sure if it makes sense here.
I'd prefer core/dom as is (as esprehn@ originally suggested when we discussed
where the module code should be),
or core/loader/modulescript (as Modulator is mostly abstract interface to
ModuleScript loading and Loading team is long-term owner for the component).

[haraken, 2017-04-05 02:08:10]

LGTM

https://codereview.chromium.org/2795593006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8PerContextData.cpp (right):

https://codereview.chromium.org/2795593006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8PerContextData.cpp:75:
m_dataMap.clear();

Is this needed?

https://codereview.chromium.org/2795593006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8PerContextData.h (right):

https://codereview.chromium.org/2795593006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8PerContextData.h:146:
PersistentHeapHashMap<const char*, Member<Data>, PtrHash<const char>>;

Do we need PtrHash<const char>?

https://codereview.chromium.org/2795593006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Modulator.cpp (right):

https://codereview.chromium.org/2795593006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Modulator.cpp:14: V8PerContextData*
getPerContextData(LocalFrame* frame) {

I'm not sure if it makes sense to let Modulator take a LocalFrame* parameter
(and assume that we're in the main world). From the binding perspective,
Modulator should be 1:1 with v8::Context, so Modulator should take a
ScriptState* parameter.

I'd prefer changing the call site of Modulator to pass in
ScriptState::forMainWorld(frame). Modulator should not worry about LocalFrame.

https://codereview.chromium.org/2795593006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Modulator.cpp:26: Modulator*
Modulator::from(V8PerContextData* perContextData) {

Ditto. It would be nicer to pass in a ScriptState* parameter.

You can address these issues in a follow-up CL.

[kouhei (in TOK), 2017-04-05 02:09:05]

https://codereview.chromium.org/2795593006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Modulator.cpp (right):

https://codereview.chromium.org/2795593006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Modulator.cpp:14: V8PerContextData*
getPerContextData(LocalFrame* frame) {
On 2017/04/05 02:08:10, haraken wrote:
> 
> I'm not sure if it makes sense to let Modulator take a LocalFrame* parameter
> (and assume that we're in the main world). From the binding perspective,
> Modulator should be 1:1 with v8::Context, so Modulator should take a
> ScriptState* parameter.
> 
> I'd prefer changing the call site of Modulator to pass in
> ScriptState::forMainWorld(frame). Modulator should not worry about LocalFrame.
> 

Ah. This completely makes sense to me.

[jbroman, 2017-04-05 21:07:01]

lgtm

If we want to take another look at whether some parts of the modules code should
live in platform/, we can do so at a later time.

https://codereview.chromium.org/2795593006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8PerContextData.cpp (right):

https://codereview.chromium.org/2795593006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8PerContextData.cpp:75:
m_dataMap.clear();
On 2017/04/05 at 02:08:09, haraken wrote:
> Is this needed?

+1; the destructor should suffice

https://codereview.chromium.org/2795593006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Modulator.cpp (right):

https://codereview.chromium.org/2795593006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Modulator.cpp:26: Modulator*
Modulator::from(V8PerContextData* perContextData) {
On 2017/04/05 at 02:08:10, haraken wrote:
> Ditto. It would be nicer to pass in a ScriptState* parameter.
> 
> You can address these issues in a follow-up CL.

+1; ScriptState seems nicer here (and it's cheap to get V8PCD from a ScriptState
anyhow).

[dcheng, 2017-04-05 21:13:29]

https://codereview.chromium.org/2795593006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Modulator.cpp (right):

https://codereview.chromium.org/2795593006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Modulator.cpp:12: const char*
kPerContextDataKey = "Modulator";
Drive-by:
const char kPerContextDataKey[] = "Modulator";

or

const char* const kPerContextDataKey = "Modulator";

In general, the former is preferred. The reason for either of these over:
  const char* kPerContextDataKey = "Modulator";

is that the "constant" is still assignable here.

[adithyas, 2017-04-05 21:45:36]

https://codereview.chromium.org/2795593006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8PerContextData.cpp (right):

https://codereview.chromium.org/2795593006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8PerContextData.cpp:75:
m_dataMap.clear();
On 2017/04/05 at 21:07:00, jbroman wrote:
> On 2017/04/05 at 02:08:09, haraken wrote:
> > Is this needed?
> 
> +1; the destructor should suffice

Ok, removed.

https://codereview.chromium.org/2795593006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8PerContextData.h (right):

https://codereview.chromium.org/2795593006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8PerContextData.h:146:
PersistentHeapHashMap<const char*, Member<Data>, PtrHash<const char>>;
On 2017/04/05 at 02:08:10, haraken wrote:
> Do we need PtrHash<const char>?

Guess we don't, DefaultHash makes pointer types use PtrHash.

https://codereview.chromium.org/2795593006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Modulator.cpp (right):

https://codereview.chromium.org/2795593006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Modulator.cpp:12: const char*
kPerContextDataKey = "Modulator";
On 2017/04/05 at 21:13:29, dcheng wrote:
> Drive-by:
> const char kPerContextDataKey[] = "Modulator";
> 
> or
> 
> const char* const kPerContextDataKey = "Modulator";
> 
> In general, the former is preferred. The reason for either of these over:
>   const char* kPerContextDataKey = "Modulator";
> 
> is that the "constant" is still assignable here.

Fixed, nice catch :)

https://codereview.chromium.org/2795593006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Modulator.cpp:14: V8PerContextData*
getPerContextData(LocalFrame* frame) {
On 2017/04/05 at 02:08:10, haraken wrote:
> I'm not sure if it makes sense to let Modulator take a LocalFrame* parameter
(and assume that we're in the main world). From the binding perspective,
Modulator should be 1:1 with v8::Context, so Modulator should take a
ScriptState* parameter.
> 
> I'd prefer changing the call site of Modulator to pass in
ScriptState::forMainWorld(frame). Modulator should not worry about LocalFrame.

Yeah, ScriptState sounds appropriate, and it removes the need for 2 "from"
methods. I'll change this in a follow up.

[adithyas, 2017-04-05 21:45:44]

The CQ bit was checked by adithyas@chromium.org

[adithyas, 2017-04-05 21:45:44]

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

[commit-bot: I haz the power, 2017-04-05 21:46:25]

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-05 21:52:37]

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

[commit-bot: I haz the power, 2017-04-05 21:52:38]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[adithyas, 2017-04-06 14:33:58]

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

[commit-bot: I haz the power, 2017-04-06 14:34: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-06 18:28:45]

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

[commit-bot: I haz the power, 2017-04-06 18:28:47]

Dry run: This issue passed the CQ dry run.

[adithyas, 2017-04-06 18:32:22]

The CQ bit was checked by adithyas@chromium.org

[adithyas, 2017-04-06 18:32:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, jbroman@chromium.org, kouhei@chromium.org
Link to the patchset: https://codereview.chromium.org/2795593006/#ps120001
(title: "Fix tests")

[commit-bot: I haz the power, 2017-04-06 18:34:19]

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-06 18:54:12]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491503542626730,
"parent_rev": "7c6520814282912a6c6437e0c68537e11d77a9b6", "commit_rev":
"0e6bf6e952ea8db241c4972fdf97bb7309502f61"}

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

Description was changed from

==========
Abstract out Modulator from V8PerContextData

To move V8PerContextData to platform/bindings, we need to remove its dependency
to Modulator. This CL creates a general abstraction for V8PerContextData to hold
instances of garbage collected objects and makes Modulator use this abstraction.

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

to

==========
Abstract out Modulator from V8PerContextData

To move V8PerContextData to platform/bindings, we need to remove its dependency
to Modulator. This CL creates a general abstraction for V8PerContextData to hold
instances of garbage collected objects and makes Modulator use this abstraction.

BUG=682322

Review-Url: https://codereview.chromium.org/2795593006
Cr-Commit-Position: refs/heads/master@{#462564}
Committed:
https://chromium.googlesource.com/chromium/src/+/0e6bf6e952ea8db241c4972fdf97...
==========

[commit-bot: I haz the power, 2017-04-06 18:55:00]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/0e6bf6e952ea8db241c4972fdf97...