Support combination of [OriginTrialEnabled] and [SecureContext]

Support combination of [OriginTrialEnabled] and [SecureContext]

This CL fixes the bindings logic to allow the [OriginTrialEnabled] and
[SecureContext] to be used together on the same IDL members.

The bug was found when trying to use [SecureContext] for WebUSB.
The IDL definition for WebUSB is corrected to use [SecureContext]
as well.

Other cleanups in this CL:
- Remove unused check_origin_trial macro
- Remove obsolete attribute variable, and fix reference for generating include
- Apply some consistency to attribute and method helper functions

BUG=695123
Review-Url: https://codereview.chromium.org/2880713002
Cr-Commit-Position: refs/heads/master@{#472062}
Committed: https://chromium.googlesource.com/chromium/src/+/3326804ff57fe560d03b1d6b740c272c01082bfe

[chasej, 2017-05-12 04:08:24]

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

[commit-bot: I haz the power, 2017-05-12 04:08:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chasej, 2017-05-12 04:13:05]

Description was changed from

==========
Support combination of [OriginTrialEnabled] and [SecureContext]


BUG=695123
==========

to

==========
Support combination of [OriginTrialEnabled] and [SecureContext]

This CL fixes the bindings logic to allow the [OriginTrialEnabled] and
[SecureContext] to be used together on the same IDL members.

The bug was found when trying to use [SecureContext] for WebUSB.
The IDL definition for WebUSB is corrected to use [SecureContext]
as well.

BUG=695123
==========

[chasej, 2017-05-12 04:13:34]

chasej@chromium.org changed reviewers:
+ iclelland@chromium.org

[chasej, 2017-05-12 04:13:35]

iclelland, please take a look.

[chasej, 2017-05-12 04:33:23]

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

[commit-bot: I haz the power, 2017-05-12 04:33:32]

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-05-12 05:27:46]

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

[commit-bot: I haz the power, 2017-05-12 05:27:47]

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

[Yuki, 2017-05-12 09:10:04]

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

[Yuki, 2017-05-12 09:10:05]

Overall, l-g-t-m.  Defer to iclelland@.

https://codereview.chromium.org/2880713002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/code_generator.py (right):

https://codereview.chromium.org/2880713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/code_generator.py:67: def
secure_context_cached_if(code, secure_context_test):
Hmm, I think *cached* in the name is not easy to understand.  I took some time
to understand that *cached* means that we already have the result in
|isSecureContext|.  Also, it's not so good to have both of secure_context_if and
secure_context_cached_if.

What about to define the following?

  def secure_context_if(code, secure_context_test, test_result=None):
    if not secure_context_test:
      return code
    elif test_result:
      return generate_indented_conditional(code, test_result)
    else:
      return generate_indented_conditional(code, 'executionContext && (%s)' %
secure_context_test)

and call it as:

  secure_context(attribute.secure_context_test)

or

  secure_context(attribute.secure_context_test, 'isSecureContext')


Or, it would be good to replace the old use cases of |secure_context()| with the
new version so that we always use the |isSecureContext| version.

By the way, I am personally not a big fan to hard-code the variable names here
and there.  If we're going to pass |isSecureContext| in an argument, then we
don't need to hard-code it.

https://codereview.chromium.org/2880713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/code_generator.py:70: return
generate_indented_conditional(code, 'isSecureContext')
nit: s/isSecureContext/is_secure_context/

https://codereview.chromium.org/2880713002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/2880713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_interface.py:173: 
nit: two empty lines

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

https://codereview.chromium.org/2880713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:727: {%
endfilter %}{# secure_context #}
nit: secure_context_cached

https://codereview.chromium.org/2880713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:744: {%
endfilter %}{# secure_context #}
nit: secure_context_cached

[haraken, 2017-05-12 11:05:53]

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

[haraken, 2017-05-12 11:05:54]

LGTM

https://codereview.chromium.org/2880713002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/2880713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_interface.py:159: not
attribute['origin_trial_feature_name'])]

Can we create a helper function like v8_attributes.filter_conditionally_exposed?

Then I'd prefer inlining conditionally_enabled_attributes to the caller site.

https://codereview.chromium.org/2880713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_interface.py:415: method for
method in conditional_enabled_methods)

Is this equivalent to len(conditional_enabled_methods)?

[Yuki, 2017-05-12 14:04:25]

https://codereview.chromium.org/2880713002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/2880713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_interface.py:415: method for
method in conditional_enabled_methods)
On 2017/05/12 11:05:54, haraken wrote:
> 
> Is this equivalent to len(conditional_enabled_methods)?

Python evaluates [] (empty list) to False, so
    if (has_conditional_methods):
and
    if (conditionally_enabled_methods):
should yield the same result.  So, no need to define has_conditional_methods, I
think.

If we're going to define has_conditional_methods, then boolean type is
preferred, i.e. bool(x) is preferred to len(x).

[iclelland, 2017-05-12 15:11:49]

Python style nits aside, this looks good from an origin trials perspective.

It looks like the WebUSB idlharness tests are failing now (or rather, more of
them are passing, but the test expectations indicate that failure is expected).
I am thinking that this is a result of better bindings generation, but I wasn't
expecting any changes to the exposed interfaces as a result of this CL. We
should check that we haven't accidentally exposed anything that we shouldn't
yet.

https://codereview.chromium.org/2880713002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/code_generator.py (right):

https://codereview.chromium.org/2880713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/code_generator.py:70: return
generate_indented_conditional(code, 'isSecureContext')
On 2017/05/12 09:10:05, Yuki wrote:
> nit: s/isSecureContext/is_secure_context/

This looks like it matches surrounding code -- especially the 'executionContext'
variable declared in many places in the generated interfaces.

Should all of the bindings templates be updated to the new Blink style in a
single CL? That seems better than making a mix of styles in the templates for
now.

https://codereview.chromium.org/2880713002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/2880713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_interface.py:415: method for
method in conditional_enabled_methods)
On 2017/05/12 14:04:25, Yuki wrote:
> On 2017/05/12 11:05:54, haraken wrote:
> > 
> > Is this equivalent to len(conditional_enabled_methods)?
> 
> Python evaluates [] (empty list) to False, so
>     if (has_conditional_methods):
> and
>     if (conditionally_enabled_methods):
> should yield the same result.  So, no need to define has_conditional_methods,
I
> think.
> 
> If we're going to define has_conditional_methods, then boolean type is
> preferred, i.e. bool(x) is preferred to len(x).

The template tags work the same way, so {% if conditionally_enabled_method %}
works as well as {% if has_conditional_methods %} in interface_base.cpp.tmpl

https://codereview.chromium.org/2880713002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/utilities.cpp.tmpl (left):

https://codereview.chromium.org/2880713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/utilities.cpp.tmpl:58: {% macro
check_origin_trial(member, isolate="info.GetIsolate()") -%}
Can you mention in the CL description that this unused macro is being deleted?

[Yuki, 2017-05-12 15:14:12]

https://codereview.chromium.org/2880713002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/code_generator.py (right):

https://codereview.chromium.org/2880713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/code_generator.py:70: return
generate_indented_conditional(code, 'isSecureContext')
On 2017/05/12 15:11:49, iclelland wrote:
> On 2017/05/12 09:10:05, Yuki wrote:
> > nit: s/isSecureContext/is_secure_context/
> 
> This looks like it matches surrounding code -- especially the
'executionContext'
> variable declared in many places in the generated interfaces.
> 
> Should all of the bindings templates be updated to the new Blink style in a
> single CL? That seems better than making a mix of styles in the templates for
> now.

Makes sense.  I'm fine with either way.

[chasej, 2017-05-12 17:55:46]

Description was changed from

==========
Support combination of [OriginTrialEnabled] and [SecureContext]

This CL fixes the bindings logic to allow the [OriginTrialEnabled] and
[SecureContext] to be used together on the same IDL members.

The bug was found when trying to use [SecureContext] for WebUSB.
The IDL definition for WebUSB is corrected to use [SecureContext]
as well.

BUG=695123
==========

to

==========
Support combination of [OriginTrialEnabled] and [SecureContext]

This CL fixes the bindings logic to allow the [OriginTrialEnabled] and
[SecureContext] to be used together on the same IDL members.

The bug was found when trying to use [SecureContext] for WebUSB.
The IDL definition for WebUSB is corrected to use [SecureContext]
as well.

Other cleanups in this CL:
- Remove unused check_origin_trial macro

BUG=695123
==========

[chasej, 2017-05-12 20:51:19]

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

[commit-bot: I haz the power, 2017-05-12 20:52: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-05-12 21:07:59]

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

[commit-bot: I haz the power, 2017-05-12 21:08:00]

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

[chasej, 2017-05-15 18:37:09]

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

[commit-bot: I haz the power, 2017-05-15 18:37:48]

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-05-15 19:12:00]

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

[commit-bot: I haz the power, 2017-05-15 19:12:01]

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

[chasej, 2017-05-16 02:15:46]

Description was changed from

==========
Support combination of [OriginTrialEnabled] and [SecureContext]

This CL fixes the bindings logic to allow the [OriginTrialEnabled] and
[SecureContext] to be used together on the same IDL members.

The bug was found when trying to use [SecureContext] for WebUSB.
The IDL definition for WebUSB is corrected to use [SecureContext]
as well.

Other cleanups in this CL:
- Remove unused check_origin_trial macro

BUG=695123
==========

to

==========
Support combination of [OriginTrialEnabled] and [SecureContext]

This CL fixes the bindings logic to allow the [OriginTrialEnabled] and
[SecureContext] to be used together on the same IDL members.

The bug was found when trying to use [SecureContext] for WebUSB.
The IDL definition for WebUSB is corrected to use [SecureContext]
as well.

Other cleanups in this CL:
- Remove unused check_origin_trial macro
- Remove obsolete attribute variable, and fix reference for generating include
- Apply some consistency to attribute and method helper functions

BUG=695123
==========

[chasej, 2017-05-16 02:21:40]

iclelland, please take another look.

The external wpt test for WebUSB was failing after removing the
workaround in ConditionalFeaturesForModules.cpp. After discussion with
reillyg, we determined it's not as simple as updating the expectations
for the failing test. I've partially restored the workaround, and will
have to fully address WebUSB + secure context in a later CL.

https://codereview.chromium.org/2880713002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/code_generator.py (right):

https://codereview.chromium.org/2880713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/code_generator.py:67: def
secure_context_cached_if(code, secure_context_test):
On 2017/05/12 09:10:05, Yuki wrote:
> Hmm, I think *cached* in the name is not easy to understand.  I took some time
> to understand that *cached* means that we already have the result in
> |isSecureContext|.  Also, it's not so good to have both of secure_context_if
and
> secure_context_cached_if.
> 
> What about to define the following?
> 
>   def secure_context_if(code, secure_context_test, test_result=None):
>     if not secure_context_test:
>       return code
>     elif test_result:
>       return generate_indented_conditional(code, test_result)
>     else:
>       return generate_indented_conditional(code, 'executionContext && (%s)' %
> secure_context_test)
> 
> and call it as:
> 
>   secure_context(attribute.secure_context_test)
> 
> or
> 
>   secure_context(attribute.secure_context_test, 'isSecureContext')
> 
> 
> Or, it would be good to replace the old use cases of |secure_context()| with
the
> new version so that we always use the |isSecureContext| version.
> 
> By the way, I am personally not a big fan to hard-code the variable names here
> and there.  If we're going to pass |isSecureContext| in an argument, then we
> don't need to hard-code it.

I agree on many points here. In an interim version, I had made similar changes
to the existing secure_context_if() function to handle both scenarios. I also
didn't like the hard-coding of variable names, but it seemed to be the common
approach in existing code. The function I came up with didn't seem like enough
of an improvement (especially since it still had |executionContext| hard-coded).

Yuki's suggestion for secure_context_if() is better, so I've implemented that
and
removed secure_context_cached_if(). I have not replaced existing usages of the
secure_context_if() function, as that would require larger changes (perhaps in
a later CL).

https://codereview.chromium.org/2880713002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/2880713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_interface.py:159: not
attribute['origin_trial_feature_name'])]
On 2017/05/12 11:05:53, haraken wrote:
> 
> Can we create a helper function like
v8_attributes.filter_conditionally_exposed?
> 
> Then I'd prefer inlining conditionally_enabled_attributes to the caller site.

Done.

https://codereview.chromium.org/2880713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_interface.py:173: 
On 2017/05/12 09:10:05, Yuki wrote:
> nit: two empty lines

Done. Actually removed this function as well. The extra check for origin
trial was moved into the helper function in v8_methods.

https://codereview.chromium.org/2880713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_interface.py:415: method for
method in conditional_enabled_methods)
On 2017/05/12 15:11:49, iclelland wrote:
> On 2017/05/12 14:04:25, Yuki wrote:
> > On 2017/05/12 11:05:54, haraken wrote:
> > > 
> > > Is this equivalent to len(conditional_enabled_methods)?
> > 
> > Python evaluates [] (empty list) to False, so
> >     if (has_conditional_methods):
> > and
> >     if (conditionally_enabled_methods):
> > should yield the same result.  So, no need to define
has_conditional_methods,
> I
> > think.
> > 
> > If we're going to define has_conditional_methods, then boolean type is
> > preferred, i.e. bool(x) is preferred to len(x).
> 
> The template tags work the same way, so {% if conditionally_enabled_method %}
> works as well as {% if has_conditional_methods %} in interface_base.cpp.tmpl

I've removed the has_conditional_methods variable entirely, both
here and in the templates.

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

https://codereview.chromium.org/2880713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:727: {%
endfilter %}{# secure_context #}
On 2017/05/12 09:10:05, Yuki wrote:
> nit: secure_context_cached

Removed secure_context_cached (as suggested above), so no change needed here.

https://codereview.chromium.org/2880713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:744: {%
endfilter %}{# secure_context #}
On 2017/05/12 09:10:05, Yuki wrote:
> nit: secure_context_cached

Removed secure_context_cached (as suggested above), so no change needed here.

https://codereview.chromium.org/2880713002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/utilities.cpp.tmpl (left):

https://codereview.chromium.org/2880713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/utilities.cpp.tmpl:58: {% macro
check_origin_trial(member, isolate="info.GetIsolate()") -%}
On 2017/05/12 15:11:49, iclelland wrote:
> Can you mention in the CL description that this unused macro is being deleted?

Done.

[iclelland, 2017-05-16 03:55:08]

LGTM -- can you update either crbug.com/695123 or crbug.com/691031 (whichever is
appropriate) with a note about the additional work that still needs to be done
to remove the other workaround code?

[chasej, 2017-05-16 03:56:33]

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

[commit-bot: I haz the power, 2017-05-16 03:57:12]

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-05-16 05:36:31]

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

[commit-bot: I haz the power, 2017-05-16 05:36:31]

Dry run: This issue passed the CQ dry run.

[chasej, 2017-05-16 11:41:58]

The CQ bit was checked by chasej@chromium.org

[chasej, 2017-05-16 11:41:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, iclelland@chromium.org
Link to the patchset: https://codereview.chromium.org/2880713002/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2017-05-16 11:42:17]

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-05-16 11:46:44]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494934918490150,
"parent_rev": "72ee324cfc8b665fead069db226dbf7ef6af7fec", "commit_rev":
"3326804ff57fe560d03b1d6b740c272c01082bfe"}

[commit-bot: I haz the power, 2017-05-16 11:46:54]

Description was changed from

==========
Support combination of [OriginTrialEnabled] and [SecureContext]

This CL fixes the bindings logic to allow the [OriginTrialEnabled] and
[SecureContext] to be used together on the same IDL members.

The bug was found when trying to use [SecureContext] for WebUSB.
The IDL definition for WebUSB is corrected to use [SecureContext]
as well.

Other cleanups in this CL:
- Remove unused check_origin_trial macro
- Remove obsolete attribute variable, and fix reference for generating include
- Apply some consistency to attribute and method helper functions

BUG=695123
==========

to

==========
Support combination of [OriginTrialEnabled] and [SecureContext]

This CL fixes the bindings logic to allow the [OriginTrialEnabled] and
[SecureContext] to be used together on the same IDL members.

The bug was found when trying to use [SecureContext] for WebUSB.
The IDL definition for WebUSB is corrected to use [SecureContext]
as well.

Other cleanups in this CL:
- Remove unused check_origin_trial macro
- Remove obsolete attribute variable, and fix reference for generating include
- Apply some consistency to attribute and method helper functions

BUG=695123

Review-Url: https://codereview.chromium.org/2880713002
Cr-Commit-Position: refs/heads/master@{#472062}
Committed:
https://chromium.googlesource.com/chromium/src/+/3326804ff57fe560d03b1d6b740c...
==========

[commit-bot: I haz the power, 2017-05-16 11:46:55]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/3326804ff57fe560d03b1d6b740c...