[Bindings] Implement installRuntimeEnabledFeatures method on some classes

Implement installRuntimeEnabledFeatures method
on V8{Window,HTMLDocument,Document,Node,EventTarget}.

It installs runtime enabled features on instances,
and its use case is limited to those classes.



BUG=617892
Review-Url: https://codereview.chromium.org/2622153002
Cr-Commit-Position: refs/heads/master@{#442939}
Committed: https://chromium.googlesource.com/chromium/src/+/aa5a228e4146b4cd7076319725f4ac09c231d098

[peria, 2017-01-11 05:03:15]

Description was changed from

==========
Implement installRuntimeEnabledFeatures methods

BUG=617892
==========

to

==========
Implement installRuntimeEnabledFeatures method
on V8{Window,HTMLDocument,Document,Node,EventTarget}.

It installs runtime enabled features on instances,
and its use case is limited to those classes.


BUG=617892
==========

[peria, 2017-01-11 05:04:00]

Description was changed from

==========
Implement installRuntimeEnabledFeatures method
on V8{Window,HTMLDocument,Document,Node,EventTarget}.

It installs runtime enabled features on instances,
and its use case is limited to those classes.


BUG=617892
==========

to

==========
Implement installRuntimeEnabledFeatures method
on V8{Window,HTMLDocument,Document,Node,EventTarget}.

It installs runtime enabled features on instances,
and its use case is limited to those classes.



BUG=617892
==========

[peria, 2017-01-11 05:04:16]

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

[commit-bot: I haz the power, 2017-01-11 05:04:34]

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

[peria, 2017-01-11 05:06:01]

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

[peria, 2017-01-11 05:06:02]

PTL.

This method will be needed to use context snapshots.

[haraken, 2017-01-11 05:27:14]

Ideally we want to avoid hard-coding interface names in the IDL compiler. What
makes it hard to implement installRuntimeEnabledFeatures for all interfaces not
limited to Window and Document?

[peria, 2017-01-11 05:51:37]

On 2017/01/11 05:27:14, haraken wrote:
> Ideally we want to avoid hard-coding interface names in the IDL compiler.
Agreed.

> What
> makes it hard to implement installRuntimeEnabledFeatures for all interfaces
not
> limited to Window and Document?
Because of our current design of Jinja template, it is difficult to choose
which attribute is runtime-enabled or not. For example, some attributes can be
installed in macros.
To verify the generated code and to catch up with updates of templates and IDLs,
I'd like to limit the target interfaces.

[Yuki, 2017-01-11 06:00:07]

On 2017/01/11 05:51:37, peria wrote:
> On 2017/01/11 05:27:14, haraken wrote:
> > Ideally we want to avoid hard-coding interface names in the IDL compiler.
> Agreed.
> 
> > What
> > makes it hard to implement installRuntimeEnabledFeatures for all interfaces
> not
> > limited to Window and Document?
> Because of our current design of Jinja template, it is difficult to choose
> which attribute is runtime-enabled or not. For example, some attributes can be
> installed in macros.
> To verify the generated code and to catch up with updates of templates and
IDLs,
> I'd like to limit the target interfaces.

I'd prefer not generating unused code in general, also prefer not generating
untested code in general.

Ideally, the code should look:
  {% if interface is saved in snapshot %}
  ...
  {% endif %}
However, I think it's acceptable to write:
  {% if interface is Window, HTMLDocument or their parent %}
  ...
  {% endif %}
given that we only take a snapshot for Window and HTMLDocument.

I agree that it's better to support "their parent" part in this CL.
(IIRC, someone tried a similar thing for [Global] or its parent.)

[commit-bot: I haz the power, 2017-01-11 06:36:27]

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

[commit-bot: I haz the power, 2017-01-11 06:36:28]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Yuki, 2017-01-11 07:06:52]

Very basic question: How do you take a snapshot *without* any of
conditionally-enabled things?

The new function adds runtime-enabled things, but how to exclude them when
taking a snapshot?

https://codereview.chromium.org/2622153002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl
(right):

https://codereview.chromium.org/2622153002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:531: {% if
v8_class_or_partial in ['V8Window', 'V8HTMLDocument', 'V8Document', 'V8Node',
'V8EventTarget'] %}
v8_class_or_partial can be V8WindowPartial, right?
You may need to take care of partial interfaces.

https://codereview.chromium.org/2622153002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:535:
v8::Local<v8::FunctionTemplate> interfaceTemplate =
{{v8_class}}::wrapperTypeInfo.domTemplate(isolate, world);
You may want to take interfaceTemplate as an argument.

https://codereview.chromium.org/2622153002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:538: 
Why don't you care about constants?

[peria, 2017-01-11 10:20:53]

https://codereview.chromium.org/2622153002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl
(right):

https://codereview.chromium.org/2622153002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:531: {% if
v8_class_or_partial in ['V8Window', 'V8HTMLDocument', 'V8Document', 'V8Node',
'V8EventTarget'] %}
On 2017/01/11 07:06:52, Yuki wrote:
> v8_class_or_partial can be V8WindowPartial, right?
> You may need to take care of partial interfaces.

Done.
Moved the logic into v8_interface.py.

https://codereview.chromium.org/2622153002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:535:
v8::Local<v8::FunctionTemplate> interfaceTemplate =
{{v8_class}}::wrapperTypeInfo.domTemplate(isolate, world);
On 2017/01/11 07:06:52, Yuki wrote:
> You may want to take interfaceTemplate as an argument.

Done.

https://codereview.chromium.org/2622153002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:538: 
On 2017/01/11 07:06:52, Yuki wrote:
> Why don't you care about constants?

It needs updates in V8DOMConfigurations,
while we don't need it for current situation.
Let me leave it as a TODO.

[peria, 2017-01-11 10:30:05]

On 2017/01/11 07:06:52, Yuki wrote:
> Very basic question: How do you take a snapshot *without* any of
> conditionally-enabled things?
> 
> The new function adds runtime-enabled things, but how to exclude them when
> taking a snapshot?
> 

RuntimeEnabledFeatures() has three methods to disable some flags.
There can be other flags but they are disabled by default.
A snapshot is taken in another process while a developer builds
chromium (or content_shell), and we can handle those flags independently
from users' settings.

Plus, we have no other conditionally enabled features in
install{{v8_class}}Template(). Hense we don't have to care them.

[Yuki, 2017-01-11 11:30:58]

LGTM.

https://codereview.chromium.org/2622153002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/interface.h.tmpl (right):

https://codereview.chromium.org/2622153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.h.tmpl:152: static void
installRuntimeEnabledFeatures(v8::Isolate*, const DOMWrapperWorld&,
v8::Local<v8::Object>, v8::Local<v8::Object>, v8::Local<v8::Function>,
v8::Local<v8::FunctionTemplate>);
Could you write argument names for non-trivial cases?
  v8::Local<v8::Object> instance,
  v8::Local<v8::Object> prototype,
  v8::Local<v8::Function> interface,
  v8::Local<v8::FunctionTemplate> interfaceTemplate

Also better to follow Chromium's coding style (i.e. wrap lines for each
argument).

https://codereview.chromium.org/2622153002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl
(right):

https://codereview.chromium.org/2622153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:581:
NOTREACHED();
You may want to replace this with #error, too.

https://codereview.chromium.org/2622153002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/partial_interface.h.tmpl
(right):

https://codereview.chromium.org/2622153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/partial_interface.h.tmpl:46: static
void installRuntimeEnabledFeatures(v8::Isolate*, const DOMWrapperWorld&,
v8::Local<v8::Object>, v8::Local<v8::Object>, v8::Local<v8::Function>,
v8::Local<v8::FunctionTemplate>);
Ditto.

[haraken, 2017-01-11 11:42:28]

> Because of our current design of Jinja template, it is difficult to choose
> which attribute is runtime-enabled or not. For example, some attributes can be
> installed in macros.

How hard will it be to make the IDL compiler understand which attribute/method
is runtime-enabled or not? Ideally I think we should generate
installDefaultFeatures and installRuntimeEnabledFeatures for all interfaces:

- The features installed by installDefaultFeatures and the features installed by
installRuntimeEnabledFeatures should be exclusive.
- When you instantiate a window object via Context::New, you call
installDefaultFeatures and installRuntimeEnabledFeatures.
- When you take a snapshot of a window object, you call only
installDefaultFeatures.
- When you instantiate a window object via Context::FromSnapshot, you call only
installRuntimeEnabledFeatures.

[haraken, 2017-01-11 12:16:13]

Discussed offline. LGTM.

https://codereview.chromium.org/2622153002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl
(right):

https://codereview.chromium.org/2622153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:534: void
{{v8_class_or_partial}}::installRuntimeEnabledFeatures(v8::Isolate* isolate,
const DOMWrapperWorld& world, v8::Local<v8::Object> instance,
v8::Local<v8::Object> prototype, v8::Local<v8::Function> interface,
v8::Local<v8::FunctionTemplate> interfaceTemplate) {

As discussed offline, simply drop the |interfaceTemplate| parameter (to align
with what the below install{{feature.name}} is doing), or pass in |signature|.

[peria, 2017-01-11 12:41:17]

https://codereview.chromium.org/2622153002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/interface.h.tmpl (right):

https://codereview.chromium.org/2622153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.h.tmpl:152: static void
installRuntimeEnabledFeatures(v8::Isolate*, const DOMWrapperWorld&,
v8::Local<v8::Object>, v8::Local<v8::Object>, v8::Local<v8::Function>,
v8::Local<v8::FunctionTemplate>);
On 2017/01/11 11:30:57, Yuki wrote:
> Could you write argument names for non-trivial cases?
>   v8::Local<v8::Object> instance,
>   v8::Local<v8::Object> prototype,
>   v8::Local<v8::Function> interface,
>   v8::Local<v8::FunctionTemplate> interfaceTemplate
> 
> Also better to follow Chromium's coding style (i.e. wrap lines for each
> argument).

Done.

https://codereview.chromium.org/2622153002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl
(right):

https://codereview.chromium.org/2622153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:534: void
{{v8_class_or_partial}}::installRuntimeEnabledFeatures(v8::Isolate* isolate,
const DOMWrapperWorld& world, v8::Local<v8::Object> instance,
v8::Local<v8::Object> prototype, v8::Local<v8::Function> interface,
v8::Local<v8::FunctionTemplate> interfaceTemplate) {
On 2017/01/11 12:16:13, haraken wrote:
> 
> As discussed offline, simply drop the |interfaceTemplate| parameter (to align
> with what the below install{{feature.name}} is doing), or pass in |signature|.
> 

Done.
In case if we implement installRuntimeEnabledFeature for templates,
I drop it.

https://codereview.chromium.org/2622153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:581:
NOTREACHED();
On 2017/01/11 11:30:58, Yuki wrote:
> You may want to replace this with #error, too.

Done.

https://codereview.chromium.org/2622153002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/partial_interface.h.tmpl
(right):

https://codereview.chromium.org/2622153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/partial_interface.h.tmpl:46: static
void installRuntimeEnabledFeatures(v8::Isolate*, const DOMWrapperWorld&,
v8::Local<v8::Object>, v8::Local<v8::Object>, v8::Local<v8::Function>,
v8::Local<v8::FunctionTemplate>);
On 2017/01/11 11:30:58, Yuki wrote:
> Ditto.

Done.

[Yuki, 2017-01-11 12:44:40]

LGTM.

[peria, 2017-01-11 13:33:59]

The CQ bit was checked by peria@chromium.org

[peria, 2017-01-11 13:33:59]

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/2622153002/#ps40001
(title: "Work for comments")

[commit-bot: I haz the power, 2017-01-11 13:34:18]

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-01-11 13:57:11]

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

[commit-bot: I haz the power, 2017-01-11 13:57:12]

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

[peria, 2017-01-11 14:10:31]

The CQ bit was checked by peria@chromium.org

[peria, 2017-01-11 14:10:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2622153002/#ps60001
(title: "Fix build error")

[commit-bot: I haz the power, 2017-01-11 14:10:49]

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-01-11 15:34:37]

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

[commit-bot: I haz the power, 2017-01-11 15:34:38]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[peria, 2017-01-11 16:46:13]

The CQ bit was checked by peria@chromium.org

[commit-bot: I haz the power, 2017-01-11 16:46:37]

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-01-11 17:18:13]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484153173053860,
"parent_rev": "ed3c975463926a72676db3793bdbed76926086b0", "commit_rev":
"aa5a228e4146b4cd7076319725f4ac09c231d098"}

[commit-bot: I haz the power, 2017-01-11 17:18:45]

Description was changed from

==========
Implement installRuntimeEnabledFeatures method
on V8{Window,HTMLDocument,Document,Node,EventTarget}.

It installs runtime enabled features on instances,
and its use case is limited to those classes.



BUG=617892
==========

to

==========
Implement installRuntimeEnabledFeatures method
on V8{Window,HTMLDocument,Document,Node,EventTarget}.

It installs runtime enabled features on instances,
and its use case is limited to those classes.



BUG=617892

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

[commit-bot: I haz the power, 2017-01-11 17:18:46]

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