Introduce [Exposed(Arguments)] in IDL code generator.

Introduce [Exposed(Arguments)] in IDL code generator.

This CL introduces [Exposed(Arguments)] form to "Exposed" extended attribute
in addition to the existing [Exposed=(Value)] form.
For example, [Exposed(Window f1, Worker f2)] exposes the qualified element to
Window if "f1" is enabled and to Worker if "f2" is enabled.

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

[yhirano, 2014-12-09 04:11:59]

Patchset #6 (id:100001) has been deleted

[yhirano, 2014-12-09 08:06:15]

yhirano@chromium.org changed reviewers:
+ bashi@google.com, jl@opera.com

[yhirano, 2014-12-09 08:06:15]

PTAL

I will update the wiki description after this CL lands.

[yhirano, 2014-12-09 08:11:50]

yhirano@chromium.org changed reviewers:
+ bashi@chromium.org
- bashi@google.com

[Jens Widell, 2014-12-09 08:25:12]

https://codereview.chromium.org/789473002/diff/80001/Source/bindings/scripts/...
File Source/bindings/scripts/generate_global_constructors.py (right):

https://codereview.chromium.org/789473002/diff/80001/Source/bindings/scripts/...
Source/bindings/scripts/generate_global_constructors.py:89: exposed_global_names
= extended_attributes.get('Exposed', 'Window').strip('()').split(',')
Does this really work now? It doesn't look like
get_interface_extended_attributes_from_idl() can quite cope with extended
attributes using the Name(Arguments) syntax.

Can you update a test IDL file to use the new syntax for an interface's Exposed?

https://codereview.chromium.org/789473002/diff/80001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/789473002/diff/80001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:266: self_set = set([e.exposed for e in
self.exposures])
Nit: You can drop the [] here:

  self_set = set(e.exposed for e in self.exposures)

https://codereview.chromium.org/789473002/diff/80001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:302: return ' ||
'.join(sorted([self._code(e) for e in self.exposures]))
Nit: Can drop the [] here as well.

https://codereview.chromium.org/789473002/diff/80001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:306: '''Returns a C++ code that checks
if a method/attribute/etc is exposed.
Nit: Use """ instead of ''' for docstrings.

[yhirano, 2014-12-09 08:42:09]

https://codereview.chromium.org/789473002/diff/80001/Source/bindings/scripts/...
File Source/bindings/scripts/generate_global_constructors.py (right):

https://codereview.chromium.org/789473002/diff/80001/Source/bindings/scripts/...
Source/bindings/scripts/generate_global_constructors.py:89: exposed_global_names
= extended_attributes.get('Exposed', 'Window').strip('()').split(',')
On 2014/12/09 08:25:12, Jens Widell wrote:
> Does this really work now? It doesn't look like
> get_interface_extended_attributes_from_idl() can quite cope with extended
> attributes using the Name(Arguments) syntax.
> 
> Can you update a test IDL file to use the new syntax for an interface's
Exposed?

Yeah, I fixed this script in another
CL(https://codereview.chromium.org/771323004/).
I added Exposed(Arguments) in TestInterface2.idl, but it doesn't have any
effect.

https://codereview.chromium.org/789473002/diff/80001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/789473002/diff/80001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:266: self_set = set([e.exposed for e in
self.exposures])
On 2014/12/09 08:25:12, Jens Widell wrote:
> Nit: You can drop the [] here:
> 
>   self_set = set(e.exposed for e in self.exposures)

Done.

https://codereview.chromium.org/789473002/diff/80001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:302: return ' ||
'.join(sorted([self._code(e) for e in self.exposures]))
On 2014/12/09 08:25:12, Jens Widell wrote:
> Nit: Can drop the [] here as well.

Done.

https://codereview.chromium.org/789473002/diff/80001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:306: '''Returns a C++ code that checks
if a method/attribute/etc is exposed.
On 2014/12/09 08:25:12, Jens Widell wrote:
> Nit: Use """ instead of ''' for docstrings.

Done.

[Jens Widell, 2014-12-09 09:20:47]

https://codereview.chromium.org/789473002/diff/80001/Source/bindings/scripts/...
File Source/bindings/scripts/generate_global_constructors.py (right):

https://codereview.chromium.org/789473002/diff/80001/Source/bindings/scripts/...
Source/bindings/scripts/generate_global_constructors.py:89: exposed_global_names
= extended_attributes.get('Exposed', 'Window').strip('()').split(',')
On 2014/12/09 08:42:09, yhirano wrote:
> On 2014/12/09 08:25:12, Jens Widell wrote:
> > Does this really work now? It doesn't look like
> > get_interface_extended_attributes_from_idl() can quite cope with extended
> > attributes using the Name(Arguments) syntax.
> > 
> > Can you update a test IDL file to use the new syntax for an interface's
> Exposed?
> 
> Yeah, I fixed this script in another
> CL(https://codereview.chromium.org/771323004/).
> I added Exposed(Arguments) in TestInterface2.idl, but it doesn't have any
> effect.

It would make sense to move that fix over from that CL to this, I think.

[yhirano, 2014-12-09 09:31:33]

On 2014/12/09 09:20:47, Jens Widell wrote:
>
https://codereview.chromium.org/789473002/diff/80001/Source/bindings/scripts/...
> File Source/bindings/scripts/generate_global_constructors.py (right):
> 
>
https://codereview.chromium.org/789473002/diff/80001/Source/bindings/scripts/...
> Source/bindings/scripts/generate_global_constructors.py:89:
exposed_global_names
> = extended_attributes.get('Exposed', 'Window').strip('()').split(',')
> On 2014/12/09 08:42:09, yhirano wrote:
> > On 2014/12/09 08:25:12, Jens Widell wrote:
> > > Does this really work now? It doesn't look like
> > > get_interface_extended_attributes_from_idl() can quite cope with extended
> > > attributes using the Name(Arguments) syntax.
> > > 
> > > Can you update a test IDL file to use the new syntax for an interface's
> > Exposed?
> > 
> > Yeah, I fixed this script in another
> > CL(https://codereview.chromium.org/771323004/).
> > I added Exposed(Arguments) in TestInterface2.idl, but it doesn't have any
> > effect.
> 
> It would make sense to move that fix over from that CL to this, I think.

Done (I will address the point you pointed out in another CL soon).

[yhirano, 2014-12-09 10:12:00]

Fixed:
https://codereview.chromium.org/771323004/diff/1/Source/bindings/scripts/util...

[Jens Widell, 2014-12-09 11:30:31]

(Sorry that I'm producing comments in batches...)

https://codereview.chromium.org/789473002/diff/160001/Source/bindings/scripts...
File Source/bindings/scripts/generate_global_constructors.py (right):

https://codereview.chromium.org/789473002/diff/160001/Source/bindings/scripts...
Source/bindings/scripts/generate_global_constructors.py:89: exposed_global_names
= extended_attributes.get('Exposed', 'Window').strip('()').split(',')
You could move this line down to line 101 (in the else branch), and change the
condition on line 92 would then just be "if exposed_arguments is not None:".

https://codereview.chromium.org/789473002/diff/160001/Source/bindings/scripts...
File Source/bindings/scripts/idl_definitions.py (right):

https://codereview.chromium.org/789473002/diff/160001/Source/bindings/scripts...
Source/bindings/scripts/idl_definitions.py:654: '''An Exposure holds one Exposed
or RuntimeEnabled condition.
Nit: Change to """ here as well.

The Google Python style guide says "Doc strings must use """ regardless."

http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Strings

https://codereview.chromium.org/789473002/diff/160001/Source/bindings/scripts...
Source/bindings/scripts/idl_definitions.py:655: Each exposure has two property:
exposed and runtime_enabled. Exposed(e, r)
Nit: "Exposed(e, r)" => "Exposure(e, r)"

https://codereview.chromium.org/789473002/diff/160001/Source/bindings/scripts...
File Source/bindings/scripts/utilities.py (right):

https://codereview.chromium.org/789473002/diff/160001/Source/bindings/scripts...
Source/bindings/scripts/utilities.py:182: match =
re.search(r'[^\B=]Exposed\(([^)]*)\)', file_contents, flags=re.DOTALL)
Inside character sets, (\b and) \B has no special meaning, so [^\B] actually
means "not 'B'".

'[^=]\bExposed\(([^)]*)\)' will work better.

(Also, re.DOTALL is noise, since there's no dot in the expression.)

https://codereview.chromium.org/789473002/diff/160001/Source/bindings/scripts...
Source/bindings/scripts/utilities.py:187: exposed, runtime_enabled =
argument.split(' ')
If you use ".split()" instead of ".split(' ')", double spaces and other
white-space characters work too.

https://codereview.chromium.org/789473002/diff/160001/Source/bindings/scripts...
File Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/789473002/diff/160001/Source/bindings/scripts...
Source/bindings/scripts/v8_utilities.py:267: other_set = set(e.exposed for e in
self.exposures)
"self.exposures" => "other.exposures"

https://codereview.chromium.org/789473002/diff/160001/Source/bindings/scripts...
Source/bindings/scripts/v8_utilities.py:273: def _extend(self, target):
I'd make this method static, name it "extended" instead of "extend" and made it
return the result instead. Due to naming of built-in methods,
"self._extend(...)" looks very much like it's extending 'self'.

You could do something like

  if workers.issubset(target):
    return target | set(['Worker'])
  elif 'Worker' in target:
    return target | workers
  return target

and at the call

  self_set = ExposureSet.extended(set(e.exposed for e in self.exposures))

https://codereview.chromium.org/789473002/diff/160001/Source/bindings/scripts...
Source/bindings/scripts/v8_utilities.py:289: def _code(self, exposure):
Make this method static too?

[yhirano, 2014-12-09 11:50:15]

Patchset #9 (id:180001) has been deleted

[yhirano, 2014-12-09 11:59:14]

https://codereview.chromium.org/789473002/diff/160001/Source/bindings/scripts...
File Source/bindings/scripts/generate_global_constructors.py (right):

https://codereview.chromium.org/789473002/diff/160001/Source/bindings/scripts...
Source/bindings/scripts/generate_global_constructors.py:89: exposed_global_names
= extended_attributes.get('Exposed', 'Window').strip('()').split(',')
On 2014/12/09 11:30:31, Jens Widell wrote:
> You could move this line down to line 101 (in the else branch), and change the
> condition on line 92 would then just be "if exposed_arguments is not None:".

Done.

https://codereview.chromium.org/789473002/diff/160001/Source/bindings/scripts...
File Source/bindings/scripts/idl_definitions.py (right):

https://codereview.chromium.org/789473002/diff/160001/Source/bindings/scripts...
Source/bindings/scripts/idl_definitions.py:654: '''An Exposure holds one Exposed
or RuntimeEnabled condition.
On 2014/12/09 11:30:31, Jens Widell wrote:
> Nit: Change to """ here as well.
> 
> The Google Python style guide says "Doc strings must use """ regardless."
> 
> http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Strings

Done.

https://codereview.chromium.org/789473002/diff/160001/Source/bindings/scripts...
Source/bindings/scripts/idl_definitions.py:655: Each exposure has two property:
exposed and runtime_enabled. Exposed(e, r)
On 2014/12/09 11:30:31, Jens Widell wrote:
> Nit: "Exposed(e, r)" => "Exposure(e, r)"

Done.

https://codereview.chromium.org/789473002/diff/160001/Source/bindings/scripts...
File Source/bindings/scripts/utilities.py (right):

https://codereview.chromium.org/789473002/diff/160001/Source/bindings/scripts...
Source/bindings/scripts/utilities.py:182: match =
re.search(r'[^\B=]Exposed\(([^)]*)\)', file_contents, flags=re.DOTALL)
On 2014/12/09 11:30:31, Jens Widell wrote:
> Inside character sets, (\b and) \B has no special meaning, so [^\B] actually
> means "not 'B'".
> 
> '[^=]\bExposed\(([^)]*)\)' will work better.
> 
> (Also, re.DOTALL is noise, since there's no dot in the expression.)

Thanks, done.

https://codereview.chromium.org/789473002/diff/160001/Source/bindings/scripts...
Source/bindings/scripts/utilities.py:187: exposed, runtime_enabled =
argument.split(' ')
On 2014/12/09 11:30:31, Jens Widell wrote:
> If you use ".split()" instead of ".split(' ')", double spaces and other
> white-space characters work too.

Done.

https://codereview.chromium.org/789473002/diff/160001/Source/bindings/scripts...
File Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/789473002/diff/160001/Source/bindings/scripts...
Source/bindings/scripts/v8_utilities.py:267: other_set = set(e.exposed for e in
self.exposures)
On 2014/12/09 11:30:31, Jens Widell wrote:
> "self.exposures" => "other.exposures"

Done.

https://codereview.chromium.org/789473002/diff/160001/Source/bindings/scripts...
Source/bindings/scripts/v8_utilities.py:273: def _extend(self, target):
On 2014/12/09 11:30:31, Jens Widell wrote:
> I'd make this method static, name it "extended" instead of "extend" and made
it
> return the result instead. Due to naming of built-in methods,
> "self._extend(...)" looks very much like it's extending 'self'.
> 
> You could do something like
> 
>   if workers.issubset(target):
>     return target | set(['Worker'])
>   elif 'Worker' in target:
>     return target | workers
>   return target
> 
> and at the call
> 
>   self_set = ExposureSet.extended(set(e.exposed for e in self.exposures))

Done.

https://codereview.chromium.org/789473002/diff/160001/Source/bindings/scripts...
Source/bindings/scripts/v8_utilities.py:289: def _code(self, exposure):
On 2014/12/09 11:30:31, Jens Widell wrote:
> Make this method static too?

Done.

[Jens Widell, 2014-12-09 12:02:12]

LGTM

[haraken, 2014-12-09 16:07:55]

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

[haraken, 2014-12-09 16:07:56]

LGTM with nits.

https://codereview.chromium.org/789473002/diff/220001/Source/bindings/scripts...
File Source/bindings/scripts/idl_definitions.py (right):

https://codereview.chromium.org/789473002/diff/220001/Source/bindings/scripts...
Source/bindings/scripts/idl_definitions.py:655: Each exposure has two property:
exposed and runtime_enabled. Exposure(e, r)

property => properties

https://codereview.chromium.org/789473002/diff/220001/Source/bindings/scripts...
Source/bindings/scripts/idl_definitions.py:656: corresponds to [Exposed(e r)].
Exposure(e) corresponds to [Exposed=e].

[Exposed(e r)] => [Exposed(e, r)] ?

https://codereview.chromium.org/789473002/diff/220001/Source/bindings/scripts...
File Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/789473002/diff/220001/Source/bindings/scripts...
Source/bindings/scripts/v8_utilities.py:263: """Returns true if |self|'s
exposure set is subset of

a subset

https://codereview.chromium.org/789473002/diff/220001/Source/bindings/scripts...
Source/bindings/scripts/v8_utilities.py:264: |other|'s exposure set. This
function doesn't case about

care about

https://codereview.chromium.org/789473002/diff/220001/Source/bindings/scripts...
Source/bindings/scripts/v8_utilities.py:305: def exposed(definition_or_member,
interface):

What does 'definition' mean? It looks a bit confusing that methods, attributes
and constants are included in 'definition' but interfaces and members are not
included in 'definition'. Probably we should rename the variable.

https://codereview.chromium.org/789473002/diff/220001/Source/bindings/scripts...
Source/bindings/scripts/v8_utilities.py:313: => context->isDocument()

context->isDocument() && RuntimeEnabledFeatures::feature1Enabled()

[Jens Widell, 2014-12-09 16:19:23]

https://codereview.chromium.org/789473002/diff/220001/Source/bindings/scripts...
File Source/bindings/scripts/idl_definitions.py (right):

https://codereview.chromium.org/789473002/diff/220001/Source/bindings/scripts...
Source/bindings/scripts/idl_definitions.py:656: corresponds to [Exposed(e r)].
Exposure(e) corresponds to [Exposed=e].
On 2014/12/09 16:07:56, haraken wrote:
> 
> [Exposed(e r)] => [Exposed(e, r)] ?

The extended attribute syntax is [Exposed(e r)], or, more generally, [Exposed(e1
r1, e2 r2, ...)].

https://codereview.chromium.org/789473002/diff/220001/Source/bindings/scripts...
File Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/789473002/diff/220001/Source/bindings/scripts...
Source/bindings/scripts/v8_utilities.py:305: def exposed(definition_or_member,
interface):
On 2014/12/09 16:07:56, haraken wrote:
> 
> What does 'definition' mean? It looks a bit confusing that methods, attributes
> and constants are included in 'definition' but interfaces and members are not
> included in 'definition'. Probably we should rename the variable.

Yeah, it ought to just be 'member'.

[Exposed] is relevant for definitions (i.e. interfaces) as well, but that's
handled in generate_global_constructors.py and doesn't use this code at all.

The name probably comes from e.g. v8_utilities.has_extended_attribute_value()'s
argument, which is anything that has an 'extended_attributes' attribute, i.e. a
definition or a member.

https://codereview.chromium.org/789473002/diff/220001/Source/bindings/scripts...
Source/bindings/scripts/v8_utilities.py:313: => context->isDocument()
On 2014/12/09 16:07:56, haraken wrote:
> 
> context->isDocument() && RuntimeEnabledFeatures::feature1Enabled()

While that's the actual logic in the end, it's not the code returned by this
function. [RuntimeEnabledFeature=X] is handled as before, via a separate {%
filter %} in the template producing a separate if-statement.

[bashi, 2014-12-09 23:42:19]

LGTM

https://codereview.chromium.org/789473002/diff/220001/Source/bindings/scripts...
File Source/bindings/scripts/generate_global_constructors.py (right):

https://codereview.chromium.org/789473002/diff/220001/Source/bindings/scripts...
Source/bindings/scripts/generate_global_constructors.py:85: if exposed_arguments
is not None:
nit:

if exposed_arguments:
    ...

https://codereview.chromium.org/789473002/diff/220001/Source/bindings/scripts...
File Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/789473002/diff/220001/Source/bindings/scripts...
Source/bindings/scripts/v8_utilities.py:272: workers = set(['DedicatedWorker',
'SharedWorker', 'ServiceWorker'])
I prefer putting this set next to EXPOSED_EXECUTION_CONTEXT_METHOD.

[yhirano, 2014-12-11 01:38:25]

https://codereview.chromium.org/789473002/diff/220001/Source/bindings/scripts...
File Source/bindings/scripts/generate_global_constructors.py (right):

https://codereview.chromium.org/789473002/diff/220001/Source/bindings/scripts...
Source/bindings/scripts/generate_global_constructors.py:85: if exposed_arguments
is not None:
On 2014/12/09 23:42:19, bashi1 wrote:
> nit:
> 
> if exposed_arguments:
>     ...

Done.

https://codereview.chromium.org/789473002/diff/220001/Source/bindings/scripts...
File Source/bindings/scripts/idl_definitions.py (right):

https://codereview.chromium.org/789473002/diff/220001/Source/bindings/scripts...
Source/bindings/scripts/idl_definitions.py:655: Each exposure has two property:
exposed and runtime_enabled. Exposure(e, r)
On 2014/12/09 16:07:55, haraken wrote:
> 
> property => properties

Done.

https://codereview.chromium.org/789473002/diff/220001/Source/bindings/scripts...
Source/bindings/scripts/idl_definitions.py:656: corresponds to [Exposed(e r)].
Exposure(e) corresponds to [Exposed=e].
On 2014/12/09 16:19:23, Jens Widell wrote:
> On 2014/12/09 16:07:56, haraken wrote:
> > 
> > [Exposed(e r)] => [Exposed(e, r)] ?
> 
> The extended attribute syntax is [Exposed(e r)], or, more generally,
[Exposed(e1
> r1, e2 r2, ...)].

It's like Exposed(Window GlobalFetch, ServiceWorker ServiceWorker)

https://codereview.chromium.org/789473002/diff/220001/Source/bindings/scripts...
File Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/789473002/diff/220001/Source/bindings/scripts...
Source/bindings/scripts/v8_utilities.py:263: """Returns true if |self|'s
exposure set is subset of
On 2014/12/09 16:07:56, haraken wrote:
> 
> a subset

Done.

https://codereview.chromium.org/789473002/diff/220001/Source/bindings/scripts...
Source/bindings/scripts/v8_utilities.py:264: |other|'s exposure set. This
function doesn't case about
On 2014/12/09 16:07:56, haraken wrote:
> 
> care about

Done.

https://codereview.chromium.org/789473002/diff/220001/Source/bindings/scripts...
Source/bindings/scripts/v8_utilities.py:272: workers = set(['DedicatedWorker',
'SharedWorker', 'ServiceWorker'])
On 2014/12/09 23:42:19, bashi1 wrote:
> I prefer putting this set next to EXPOSED_EXECUTION_CONTEXT_METHOD.

Done.

https://codereview.chromium.org/789473002/diff/220001/Source/bindings/scripts...
Source/bindings/scripts/v8_utilities.py:305: def exposed(definition_or_member,
interface):
On 2014/12/09 16:19:23, Jens Widell wrote:
> On 2014/12/09 16:07:56, haraken wrote:
> > 
> > What does 'definition' mean? It looks a bit confusing that methods,
attributes
> > and constants are included in 'definition' but interfaces and members are
not
> > included in 'definition'. Probably we should rename the variable.
> 
> Yeah, it ought to just be 'member'.
> 
> [Exposed] is relevant for definitions (i.e. interfaces) as well, but that's
> handled in generate_global_constructors.py and doesn't use this code at all.
> 
> The name probably comes from e.g.
v8_utilities.has_extended_attribute_value()'s
> argument, which is anything that has an 'extended_attributes' attribute, i.e.
a
> definition or a member.

Done.

https://codereview.chromium.org/789473002/diff/220001/Source/bindings/scripts...
Source/bindings/scripts/v8_utilities.py:313: => context->isDocument()
On 2014/12/09 16:19:23, Jens Widell wrote:
> On 2014/12/09 16:07:56, haraken wrote:
> > 
> > context->isDocument() && RuntimeEnabledFeatures::feature1Enabled()
> 
> While that's the actual logic in the end, it's not the code returned by this
> function. [RuntimeEnabledFeature=X] is handled as before, via a separate {%
> filter %} in the template producing a separate if-statement.

Yeah, so I would like to leave it be.

[yhirano, 2014-12-11 08:07:49]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-12-11 08:08:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/789473002/240001

[commit-bot: I haz the power, 2014-12-11 11:51:40]

Committed patchset #11 (id:240001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186945