[Bindings] Code cleanup: Early return if funtime enabled interface is false

Put an early return if an interface has a runtime enabled flag
and it is false.

It was confusing that the flag seemed to check if enable some
specific members (attributes, methods, and constants), but it
should check everything of the interface.
It means if the flag is false, we don't have to check all
members, e.g. indexed properties and iterators.


BUG=None

Review-Url: https://codereview.chromium.org/2620153004
Cr-Commit-Position: refs/heads/master@{#443179}
Committed: https://chromium.googlesource.com/chromium/src/+/c162e0c69d45b4bb191cb80ca9fde153950a270b

[peria, 2017-01-11 12:23:45]

Patchset #1 (id:1) has been deleted

[peria, 2017-01-11 12:24:02]

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

[commit-bot: I haz the power, 2017-01-11 12:24:21]

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

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

[commit-bot: I haz the power, 2017-01-11 13:10:20]

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

[peria, 2017-01-11 13:49: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 13:49:37]

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-01-11 14:26:54]

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

[commit-bot: I haz the power, 2017-01-11 14:26:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

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

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

[peria, 2017-01-11 17:04:33]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-01-11 17:04:42]

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 17:04:42]

Patchset #1 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-01-11 18:36:22]

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

[commit-bot: I haz the power, 2017-01-11 18:36:23]

Dry run: This issue passed the CQ dry run.

[peria, 2017-01-12 01:18:45]

Description was changed from

==========
Code Cleanup: Move runtime enabled flag check

BUG=None
==========

to

==========
Move runtime enabled flag check, if an interface has a runtime
enabled flag.

It was confusing that the flag seemed to check to enable some
specific members (attributes, methods, and constants), but it
should check everything of the interface.
It means if the flag is false, we don't have to check all
members, e.g. indexed properties and iterators. So I put an
early return in such a case.


BUG=None
==========

[peria, 2017-01-12 03:37:23]

Description was changed from

==========
Move runtime enabled flag check, if an interface has a runtime
enabled flag.

It was confusing that the flag seemed to check to enable some
specific members (attributes, methods, and constants), but it
should check everything of the interface.
It means if the flag is false, we don't have to check all
members, e.g. indexed properties and iterators. So I put an
early return in such a case.


BUG=None
==========

to

==========
Move runtime enabled flag check, if an interface has a runtime
enabled flag.

It was confusing that the flag seemed to check if enable some
specific members (attributes, methods, and constants), but it
should check everything of the interface.
It means if the flag is false, we don't have to check all
members, e.g. indexed properties and iterators. So I put an
early return in such a case.


BUG=None
==========

[peria, 2017-01-12 03:37:39]

Patchset #1 (id:60001) has been deleted

[peria, 2017-01-12 03:38:09]

Description was changed from

==========
Move runtime enabled flag check, if an interface has a runtime
enabled flag.

It was confusing that the flag seemed to check if enable some
specific members (attributes, methods, and constants), but it
should check everything of the interface.
It means if the flag is false, we don't have to check all
members, e.g. indexed properties and iterators. So I put an
early return in such a case.


BUG=None
==========

to

==========
Move runtime enabled flag check, if an interface has a runtime
enabled flag.

It was confusing that the flag seemed to check if enable some
specific members (attributes, methods, and constants), but it
should check everything of the interface.
It means if the flag is false, we don't have to check all
members, e.g. indexed properties and iterators.


BUG=None
==========

[peria, 2017-01-12 03:40:04]

Description was changed from

==========
Move runtime enabled flag check, if an interface has a runtime
enabled flag.

It was confusing that the flag seemed to check if enable some
specific members (attributes, methods, and constants), but it
should check everything of the interface.
It means if the flag is false, we don't have to check all
members, e.g. indexed properties and iterators.


BUG=None
==========

to

==========
Put an early return if an interface has a runtime enabled flag
and it is false.

It was confusing that the flag seemed to check if enable some
specific members (attributes, methods, and constants), but it
should check everything of the interface.
It means if the flag is false, we don't have to check all
members, e.g. indexed properties and iterators.


BUG=None
==========

[peria, 2017-01-12 03:44:34]

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

[peria, 2017-01-12 03:44:35]

PTL

[haraken, 2017-01-12 04:06:49]

LGTM

[peria, 2017-01-12 04:49:09]

The CQ bit was checked by peria@chromium.org

[commit-bot: I haz the power, 2017-01-12 04:49:43]

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-12 07:35:20]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484196549433290,
"parent_rev": "0e882142615e4ca0d8e6c5715fde9393fcc232e7", "commit_rev":
"c162e0c69d45b4bb191cb80ca9fde153950a270b"}

[commit-bot: I haz the power, 2017-01-12 07:35:49]

Description was changed from

==========
Put an early return if an interface has a runtime enabled flag
and it is false.

It was confusing that the flag seemed to check if enable some
specific members (attributes, methods, and constants), but it
should check everything of the interface.
It means if the flag is false, we don't have to check all
members, e.g. indexed properties and iterators.


BUG=None
==========

to

==========
Put an early return if an interface has a runtime enabled flag
and it is false.

It was confusing that the flag seemed to check if enable some
specific members (attributes, methods, and constants), but it
should check everything of the interface.
It means if the flag is false, we don't have to check all
members, e.g. indexed properties and iterators.


BUG=None

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

[commit-bot: I haz the power, 2017-01-12 07:35:50]

Committed patchset #1 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/c162e0c69d45b4bb191cb80ca9fd...

[Yuki, 2017-01-13 02:38:37]

https://codereview.chromium.org/2620153004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/2620153004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_utilities.py:115: def
runtime_enabled_function(name):
I'm uneasy with this name because "runtime enabled function" reads a function
(or function name) which can be enabled at runtime.

This function is a predictor.  So, I'd recommend "is_runtime_enabled" instead.

  def is_runtime_enabled(feature_name):

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

https://codereview.chromium.org/2620153004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:410: if
(!{{runtime_enabled_feature_name | runtime_enabled_function}}) {
This is okay, but I guess you should be able to write:
  if (!{{is_runtime_enabled(runtime_enabled_feature_name)}}) {
  }
They're equivalent, though.

https://codereview.chromium.org/2620153004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:410: if
(!{{runtime_enabled_feature_name | runtime_enabled_function}}) {
If the feature is disabled, we shouldn't hit this line.  Do you have any clues
why we're calling V8Type::installTypeTemplate() even when Type is disabled?  Who
is calling the function?

[peria, 2017-01-13 05:54:26]

https://codereview.chromium.org/2620153004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/2620153004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_utilities.py:115: def
runtime_enabled_function(name):
On 2017/01/13 02:38:37, Yuki wrote:
> I'm uneasy with this name because "runtime enabled function" reads a function
> (or function name) which can be enabled at runtime.
> 
> This function is a predictor.  So, I'd recommend "is_runtime_enabled" instead.
> 
>   def is_runtime_enabled(feature_name):

I'll make a follow up CL.
But I think the prefix "is_" is not good for functions which return strings.

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

https://codereview.chromium.org/2620153004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:410: if
(!{{runtime_enabled_feature_name | runtime_enabled_function}}) {
On 2017/01/13 02:38:37, Yuki wrote:
> This is okay, but I guess you should be able to write:
>   if (!{{is_runtime_enabled(runtime_enabled_feature_name)}}) {
>   }
> They're equivalent, though.

As a context of Jinja, I think it better to make the content of
|runtime_enabled_feature_name| (e.g. "FooBar") to be the function name (e.g.
"RuntimeEnabledFeatures::fooBarEnabled()").

https://codereview.chromium.org/2620153004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:410: if
(!{{runtime_enabled_feature_name | runtime_enabled_function}}) {
On 2017/01/13 02:38:37, Yuki wrote:
> If the feature is disabled, we shouldn't hit this line.  Do you have any clues
> why we're calling V8Type::installTypeTemplate() even when Type is disabled? 
Who
> is calling the function?

V8Type::domTemplate() calls it through V8DOMConfiguration::domClassTemplate(),
and domTemplate()'s caller is handler of V8Type::wrapperTypeInfo.

So if we want to avoid calling V8Type::installV8TypeTemplate()
depending on runtime enabled flags, we need to make a branch in
V8Type::domTemplate().