Implement '[SecureContext]' IDL attribute for interfaces, methods and attributes.

Implement '[SecureContext]' IDL attribute for methods and attributes.

This is step 1 of implementing '[SecureContext]'. It deals with interfaces,
attributes and methods, but does not yet correctly handle partial interfaces,
constants, dictionaries, etc.

BUG=634270
Committed: https://crrev.com/59dbf413248d36371afa36b4020c81dd42f72b69
Cr-Commit-Position: refs/heads/master@{#410019}

[Mike West, 2016-08-04 10:06:09]

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

[Mike West, 2016-08-04 10:06:09]

Hello, folks who know more about bindings than I do! This is a first stab at
getting partial support for '[SecureContext]' into our generator. Interfaces
(and in particular, partial interfaces that add to the global) are going to take
a bit more time. :/

Feedback would be totally appreciated. Thanks!

-mike

[Mike West, 2016-08-04 11:13:26]

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

[commit-bot: I haz the power, 2016-08-04 11:13:44]

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

[Yuki, 2016-08-04 12:36:39]

LGTM on my side, but please wait for bashi@'s review.

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

https://codereview.chromium.org/2207423002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface.cpp:914: {% for attribute
in attributes if (attribute.exposed_test or attribute.secure_context_test) and
attribute.on_prototype %}
Just a question, not a suggestion:
You seem defining method.conditionally_exposed.  Why don't you
attribute.conditionally_exposed?  Maybe because "exposed_test or
secure_context_test" looks more straightforward?

[Mike West, 2016-08-04 13:05:14]

On 2016/08/04 at 12:36:39, yukishiino wrote:
> LGTM on my side, but please wait for bashi@'s review.
> 
>
https://codereview.chromium.org/2207423002/diff/1/third_party/WebKit/Source/b...
> File third_party/WebKit/Source/bindings/templates/interface.cpp (right):
> 
>
https://codereview.chromium.org/2207423002/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/templates/interface.cpp:914: {% for
attribute in attributes if (attribute.exposed_test or
attribute.secure_context_test) and attribute.on_prototype %}
> Just a question, not a suggestion:
> You seem defining method.conditionally_exposed.  Why don't you
attribute.conditionally_exposed?  Maybe because "exposed_test or
secure_context_test" looks more straightforward?

I could certainly extract this up if that would be clearer. I'm happy to go
along with pretty much whatever style you'd prefer in the generator. Here, I
figured it better to just call out explicitly the pieces I was interested in,
since there wasn't a preexisting named attribute.

Part of the reason I haven't added any new concepts like that is that the
current set of concepts are super-confusing. :) We conditionally expose in three
different ways in the status quo (`Exposed`, `RuntimeEnabled`, and
`OriginTrial`), and each seems to be implemented in a different way. It isn't
too bad in this patch, but the next patch is getting hairy... :)

[Yuki, 2016-08-04 13:16:29]

On 2016/08/04 13:05:14, Mike West wrote:
> On 2016/08/04 at 12:36:39, yukishiino wrote:
> > LGTM on my side, but please wait for bashi@'s review.
> > 
> >
>
https://codereview.chromium.org/2207423002/diff/1/third_party/WebKit/Source/b...
> > File third_party/WebKit/Source/bindings/templates/interface.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2207423002/diff/1/third_party/WebKit/Source/b...
> > third_party/WebKit/Source/bindings/templates/interface.cpp:914: {% for
> attribute in attributes if (attribute.exposed_test or
> attribute.secure_context_test) and attribute.on_prototype %}
> > Just a question, not a suggestion:
> > You seem defining method.conditionally_exposed.  Why don't you
> attribute.conditionally_exposed?  Maybe because "exposed_test or
> secure_context_test" looks more straightforward?
> 
> I could certainly extract this up if that would be clearer. I'm happy to go
> along with pretty much whatever style you'd prefer in the generator. Here, I
> figured it better to just call out explicitly the pieces I was interested in,
> since there wasn't a preexisting named attribute.
> 
> Part of the reason I haven't added any new concepts like that is that the
> current set of concepts are super-confusing. :) We conditionally expose in
three
> different ways in the status quo (`Exposed`, `RuntimeEnabled`, and
> `OriginTrial`), and each seems to be implemented in a different way. It isn't
> too bad in this patch, but the next patch is getting hairy... :)

Agreed.  We're struggling with messy codebase in this area.  No need to make
more changes in this CL.  Let's go with this for the time being.

I think we need to refactor this area at some point.

Still LGTM.

[haraken, 2016-08-04 13:51:17]

LGTM

https://codereview.chromium.org/2207423002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/2207423002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/v8_methods.py:61: secure =
method['overloads']['secure_context_test_all'] if 'overloads' in method else
method['secure_context_test']

secure => secure_context

https://codereview.chromium.org/2207423002/diff/1/third_party/WebKit/Source/b...
File
third_party/WebKit/Source/bindings/tests/idls/core/TestInterfaceSecureContext.idl
(right):

https://codereview.chromium.org/2207423002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/tests/idls/core/TestInterfaceSecureContext.idl:6:
interface TestInterfaceSecureContext {

Instead of adding a new IDL file, can we add these attributes/methods to
existing IDL files? I don't want to bloat # of files in bindings/test/results/
:)

[commit-bot: I haz the power, 2016-08-04 14:26:15]

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

[commit-bot: I haz the power, 2016-08-04 14:26:16]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Mike West, 2016-08-04 17:38:15]

Thanks to you both! I'll give bashi@ a chance to look at this before addressing
nits and landing.

https://codereview.chromium.org/2207423002/diff/1/third_party/WebKit/Source/b...
File
third_party/WebKit/Source/bindings/tests/idls/core/TestInterfaceSecureContext.idl
(right):

https://codereview.chromium.org/2207423002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/tests/idls/core/TestInterfaceSecureContext.idl:6:
interface TestInterfaceSecureContext {
On 2016/08/04 at 13:51:17, haraken wrote:
> Instead of adding a new IDL file, can we add these attributes/methods to
existing IDL files? I don't want to bloat # of files in bindings/test/results/
:)

Is there really a cost to more test result files? It seems simplest to have
small tests that verify a certain type of behavior, rather than adding 12
attributes to an unrelated interface.

I actually split these out of `TestInterface.idl` because it had
`RuntimeEnabled` on the interface itself, which really confused me when I was
trying to determine if the results were correct (as it changed the generated
code).

I'll defer to y'all on this, but I'd prefer to keep this distinct.

[bashi, 2016-08-04 23:16:14]

lgtm

https://codereview.chromium.org/2207423002/diff/1/third_party/WebKit/Source/b...
File
third_party/WebKit/Source/bindings/tests/idls/core/TestInterfaceSecureContext.idl
(right):

https://codereview.chromium.org/2207423002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/tests/idls/core/TestInterfaceSecureContext.idl:6:
interface TestInterfaceSecureContext {
Don't we need to have a test for an interface which has [SecureContext]  anyway?

[SecureContext]
interface TestInterfaceSecureContext2 {
 ...
}

[haraken, 2016-08-05 04:05:38]

https://codereview.chromium.org/2207423002/diff/1/third_party/WebKit/Source/b...
File
third_party/WebKit/Source/bindings/tests/idls/core/TestInterfaceSecureContext.idl
(right):

https://codereview.chromium.org/2207423002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/tests/idls/core/TestInterfaceSecureContext.idl:6:
interface TestInterfaceSecureContext {
On 2016/08/04 23:16:13, bashi1 wrote:
> Don't we need to have a test for an interface which has [SecureContext] 
anyway?
> 
> [SecureContext]
> interface TestInterfaceSecureContext2 {
>  ...
> }

Yeah, good point. So shall we add:

- these attributes/operations to an existing test IDL file
- a new IDL file for an interface with [SecureContext]

?

mkwst@: In general, I prefer not bloating # of testing IDL files because it
unnecessarily increases the CL size of changes to the IDL compiler.

[Mike West, 2016-08-05 07:42:01]

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

[commit-bot: I haz the power, 2016-08-05 07:42:17]

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

[Mike West, 2016-08-05 07:42:47]

Thanks, all. I've added `[SecureContext]` to the `TestInterfaceSecureContext`
IDL, and added the previous contents of that file to `TestInterface`.

https://codereview.chromium.org/2207423002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/2207423002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/v8_methods.py:61: secure =
method['overloads']['secure_context_test_all'] if 'overloads' in method else
method['secure_context_test']
On 2016/08/04 at 13:51:17, haraken wrote:
> secure => secure_context

Done.

https://codereview.chromium.org/2207423002/diff/1/third_party/WebKit/Source/b...
File
third_party/WebKit/Source/bindings/tests/idls/core/TestInterfaceSecureContext.idl
(right):

https://codereview.chromium.org/2207423002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/tests/idls/core/TestInterfaceSecureContext.idl:6:
interface TestInterfaceSecureContext {
On 2016/08/05 at 04:05:38, haraken wrote:
> On 2016/08/04 23:16:13, bashi1 wrote:
> > Don't we need to have a test for an interface which has [SecureContext] 
anyway?
> > 
> > [SecureContext]
> > interface TestInterfaceSecureContext2 {
> >  ...
> > }
> 
> Yeah, good point. So shall we add:
> 
> - these attributes/operations to an existing test IDL file
> - a new IDL file for an interface with [SecureContext]

Done.

[haraken, 2016-08-05 07:52:41]

On 2016/08/05 07:42:47, Mike West wrote:
> Thanks, all. I've added `[SecureContext]` to the `TestInterfaceSecureContext`
> IDL, and added the previous contents of that file to `TestInterface`.
> 
>
https://codereview.chromium.org/2207423002/diff/1/third_party/WebKit/Source/b...
> File third_party/WebKit/Source/bindings/scripts/v8_methods.py (right):
> 
>
https://codereview.chromium.org/2207423002/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/scripts/v8_methods.py:61: secure =
> method['overloads']['secure_context_test_all'] if 'overloads' in method else
> method['secure_context_test']
> On 2016/08/04 at 13:51:17, haraken wrote:
> > secure => secure_context
> 
> Done.
> 
>
https://codereview.chromium.org/2207423002/diff/1/third_party/WebKit/Source/b...
> File
>
third_party/WebKit/Source/bindings/tests/idls/core/TestInterfaceSecureContext.idl
> (right):
> 
>
https://codereview.chromium.org/2207423002/diff/1/third_party/WebKit/Source/b...
>
third_party/WebKit/Source/bindings/tests/idls/core/TestInterfaceSecureContext.idl:6:
> interface TestInterfaceSecureContext {
> On 2016/08/05 at 04:05:38, haraken wrote:
> > On 2016/08/04 23:16:13, bashi1 wrote:
> > > Don't we need to have a test for an interface which has [SecureContext] 
> anyway?
> > > 
> > > [SecureContext]
> > > interface TestInterfaceSecureContext2 {
> > >  ...
> > > }
> > 
> > Yeah, good point. So shall we add:
> > 
> > - these attributes/operations to an existing test IDL file
> > - a new IDL file for an interface with [SecureContext]
> 
> Done.

LGTM

[Mike West, 2016-08-05 08:04:30]

Description was changed from

==========
Implement '[SecureContext]' IDL attribute for methods and attributes.

This is step 1 of implementing '[SecureContext]'. It deals with attributes
and methods, but does not yet correctly handle interfaces, partial
interfaces, constants, etc.

BUG=634270
==========

to

==========
Implement '[SecureContext]' IDL attribute for methods and attributes.

This is step 1 of implementing '[SecureContext]'. It deals with interfaces, 
attributes and methods, but does not yet correctly handle partial interfaces,
constants, dictionaries, etc.

BUG=634270
==========

[Mike West, 2016-08-05 08:04:36]

The CQ bit was unchecked by mkwst@chromium.org

[Mike West, 2016-08-05 08:33:05]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2016-08-05 08:33:05]

The patchset sent to the CQ was uploaded after l-g-t-m from bashi@chromium.org,
yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2207423002/#ps20001
(title: "Feedback.")

[commit-bot: I haz the power, 2016-08-05 08:33:13]

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, 2016-08-05 09:17:27]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-05 09:20:42]

Description was changed from

==========
Implement '[SecureContext]' IDL attribute for methods and attributes.

This is step 1 of implementing '[SecureContext]'. It deals with interfaces, 
attributes and methods, but does not yet correctly handle partial interfaces,
constants, dictionaries, etc.

BUG=634270
==========

to

==========
Implement '[SecureContext]' IDL attribute for methods and attributes.

This is step 1 of implementing '[SecureContext]'. It deals with interfaces,
attributes and methods, but does not yet correctly handle partial interfaces,
constants, dictionaries, etc.

BUG=634270

Committed: https://crrev.com/59dbf413248d36371afa36b4020c81dd42f72b69
Cr-Commit-Position: refs/heads/master@{#410019}
==========

[commit-bot: I haz the power, 2016-08-05 09:20:44]

Patchset 2 (id:??) landed as
https://crrev.com/59dbf413248d36371afa36b4020c81dd42f72b69
Cr-Commit-Position: refs/heads/master@{#410019}