Move modules-dependent Event code out of core.

Move modules-dependent Event code out of core.
This is a follow-up of https://codereview.chromium.org/298893006/.

BUG=376506
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=174888

[c.shu, 2014-05-23 19:55:43]

only when you have time, Eric. Thanks.

[c.shu, 2014-05-24 01:00:22]

The failures on bots are real. The problem is that the generated V8Document.cpp
is still calling Document::createEvent.
Working on it.

[Nils Barth (inactive), 2014-05-27 05:59:59]

On 2014/05/24 01:00:22, c.shu wrote:
> The failures on bots are real. The problem is that the generated
V8Document.cpp
> is still calling Document::createEvent.
> Working on it.

+eseidel, thoughts?

The underlying problem is that modules can *extend* types in core.
For now we can work around this by calling the modules form, since the layering
is just core < modules,
but when we split into more components, this will be a serious problem:
imagine if the layering is:
modules/foo | modules/bar
-------------------------
core
...in which case we can't call createEventModulesFoo or createEventModulesBar if
there are events in both.

We'll either need some top-level (web? below_web?) component to hold all of
these,
or we'll need to restructure (e.g., use static member functions or have some
kind of registration step).
This shows up everywhere in componentization.

I don't think we need to block on this issue for this CL, but we'll need to
address this eventually.

[Nils Barth (inactive), 2014-05-27 06:01:27]

Couple comments

https://codereview.chromium.org/297103002/diff/60001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8DocumentCustom.cpp (right):

https://codereview.chromium.org/297103002/diff/60001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8DocumentCustom.cpp:88: void
V8Document::createEventMethodCustom(const v8::FunctionCallbackInfo<v8::Value>&
info)
Please add a comment explaining why there is custom code.

https://codereview.chromium.org/297103002/diff/60001/Source/build/scripts/tem...
File Source/build/scripts/templates/EventFactory.cpp.tmpl (right):

https://codereview.chromium.org/297103002/diff/60001/Source/build/scripts/tem...
Source/build/scripts/templates/EventFactory.cpp.tmpl:5: {% if suffix ==
"Modules" %}
Style: single quotes in Python code.

https://codereview.chromium.org/297103002/diff/60001/Source/build/scripts/tem...
Source/build/scripts/templates/EventFactory.cpp.tmpl:6: #include
"modules/{{namespace}}{{suffix}}Factory.h"
Should we have
modules/events/{{namespace}}{{suffix}}Factory.h
instead?
I.e., modules/events to line up with core/events?

https://codereview.chromium.org/297103002/diff/60001/Source/core/dom/Document...
File Source/core/dom/Document.idl (right):

https://codereview.chromium.org/297103002/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.idl:66: [Custom, RaisesException] Event
createEvent(DOMString eventType);
Please add a comment explaining why there's a [Custom] here.

Also, why doesn't [ImplementedAs=createEventModules] work?

[Nils Barth (inactive), 2014-05-27 06:02:19]

(Added blink-reviews@, seemed missing.)

[c.shu, 2014-05-27 15:27:51]

On 2014/05/27 05:59:59, Nils Barth wrote:
> On 2014/05/24 01:00:22, c.shu wrote:
> > The failures on bots are real. The problem is that the generated
> V8Document.cpp
> > is still calling Document::createEvent.
> > Working on it.
> 
> +eseidel, thoughts?
> 
> The underlying problem is that modules can *extend* types in core.
> For now we can work around this by calling the modules form, since the
layering
> is just core < modules,
> but when we split into more components, this will be a serious problem:
> imagine if the layering is:
> modules/foo | modules/bar
> -------------------------
> core
> ...in which case we can't call createEventModulesFoo or createEventModulesBar
if
> there are events in both.
> 
> We'll either need some top-level (web? below_web?) component to hold all of
> these,
> or we'll need to restructure (e.g., use static member functions or have some
> kind of registration step).
> This shows up everywhere in componentization.
> 
> I don't think we need to block on this issue for this CL, but we'll need to
> address this eventually.

Thanks, Nils. That's exactly true. I put some comments in the code and will
probably follow up with a method registration approach.

[c.shu, 2014-05-27 15:28:01]

https://codereview.chromium.org/297103002/diff/60001/Source/build/scripts/tem...
File Source/build/scripts/templates/EventFactory.cpp.tmpl (right):

https://codereview.chromium.org/297103002/diff/60001/Source/build/scripts/tem...
Source/build/scripts/templates/EventFactory.cpp.tmpl:5: {% if suffix ==
"Modules" %}
On 2014/05/27 06:01:27, Nils Barth wrote:
> Style: single quotes in Python code.

Done.

https://codereview.chromium.org/297103002/diff/60001/Source/build/scripts/tem...
Source/build/scripts/templates/EventFactory.cpp.tmpl:6: #include
"modules/{{namespace}}{{suffix}}Factory.h"
On 2014/05/27 06:01:27, Nils Barth wrote:
> Should we have
> modules/events/{{namespace}}{{suffix}}Factory.h
> instead?
> I.e., modules/events to line up with core/events?

Sounds good, Nils. Is it Ok I move all event related files under modules/ to
modules/events in a separate patch? The patch will impact many files in modules.

https://codereview.chromium.org/297103002/diff/60001/Source/core/dom/Document...
File Source/core/dom/Document.idl (right):

https://codereview.chromium.org/297103002/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.idl:66: [Custom, RaisesException] Event
createEvent(DOMString eventType);
On 2014/05/27 06:01:27, Nils Barth wrote:
> Please add a comment explaining why there's a [Custom] here.
> 
> Also, why doesn't [ImplementedAs=createEventModules] work?

[ImplementedAs=createEventModules] does not work because createEventModules is
not a member of WebCore::Document and the header file under modules cannot
automatically be included.

[c.shu, 2014-05-27 15:34:07]

https://codereview.chromium.org/297103002/diff/60001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8DocumentCustom.cpp (right):

https://codereview.chromium.org/297103002/diff/60001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8DocumentCustom.cpp:88: void
V8Document::createEventMethodCustom(const v8::FunctionCallbackInfo<v8::Value>&
info)
On 2014/05/27 06:01:27, Nils Barth wrote:
> Please add a comment explaining why there is custom code.

Done.

[eseidel, 2014-05-27 17:36:08]

lgtm

[c.shu, 2014-05-27 17:39:22]

On 2014/05/27 17:36:08, eseidel wrote:
> lgtm

Thanks, Eric.
Nils, do you have additional comments?

[c.shu, 2014-05-27 18:34:39]

On 2014/05/27 17:39:22, c.shu wrote:
> On 2014/05/27 17:36:08, eseidel wrote:
> > lgtm
> 
> Thanks, Eric.
> Nils, do you have additional comments?

Nils, if you don't mind, I will commit this CL now. I roughly have follow-up
patches that address your other comments in the review. I hope to upload them
asap. Thanks!

[c.shu, 2014-05-27 18:34:53]

The CQ bit was checked by c.shu@samsung.com

[commit-bot: I haz the power, 2014-05-27 18:36:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/c.shu@samsung.com/297103002/80001

[commit-bot: I haz the power, 2014-05-27 19:55:50]

Change committed as 174888

[Nils Barth (inactive), 2014-05-28 00:55:26]

On 2014/05/27 15:27:51, c.shu wrote:
> Thanks, Nils. That's exactly true. I put some comments in the code and will
> probably follow up with a method registration approach.

Thanks!

[Nils Barth (inactive), 2014-05-28 00:57:58]

On 2014/05/27 18:34:39, c.shu wrote:
> On 2014/05/27 17:39:22, c.shu wrote:
> > Nils, do you have additional comments?
> 
> Nils, if you don't mind, I will commit this CL now. I roughly have follow-up
> patches that address your other comments in the review. I hope to upload them
> asap. Thanks!

No problem (Tokyo time zone difference, so fine to use your judgement re: having
addressed concerns)!
Oh, and LGTM, thanks for fixes!

[Nils Barth (inactive), 2014-05-28 00:58:06]

https://codereview.chromium.org/297103002/diff/60001/Source/build/scripts/tem...
File Source/build/scripts/templates/EventFactory.cpp.tmpl (right):

https://codereview.chromium.org/297103002/diff/60001/Source/build/scripts/tem...
Source/build/scripts/templates/EventFactory.cpp.tmpl:6: #include
"modules/{{namespace}}{{suffix}}Factory.h"
On 2014/05/27 15:28:02, c.shu wrote:
> Sounds good, Nils. Is it Ok I move all event related files under modules/ to
> modules/events in a separate patch? The patch will impact many files in
modules.

Yup, that's fine!