Empty reflected attributes and string literals in extended attributes.

Empty reflected attributes and string literals in extended attributes.

Encoding the CORS setting attribute

  http://www.whatwg.org/specs/web-apps/current-work/#cors-settings-attribute

as a reflected attribute exposed a missing piece in our reflected
attribute handling: attributes that are empty (no value) can
separately be mapped to an attribute state. Upon reflection via its
IDL attribute, that state must then be mapped to its canonical
value/keyword.

Support the expression of this through the setting of
[ReflectEmpty="value"].

Separately, but also induced by CORS attributes, an attribute value
may contain non-identifier characters (as defined by WebIDL), which
makes identifiers not a perfect fit for specifying these reflected
attribute values (the use of "-" being the direct problem for CORS
attribute values.)

To address, extended attributes can now take string literals on their
right hand sides (e.g., [Attr="B"|"C"|"D"].) Attribute value lists
provided with [ReflectOnly=<list>] can then contain the wider range of
characters.

To go with that, the [ReflectEmpty], [ReflectMissing] and
[ReflectInvalid] attributes can also be given string literals on their
right hand side, e.g.,

  [ReflectEmpty="Value1", ReflectInvalid="Value2"]

The string literal must match one of the values in the [ReflectOnly]
list.

R=haraken,nbarth
BUG=331694
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164776

[sof, 2014-01-08 10:19:43]

Please take a look when you next have a moment.

A missing piece & subtlety in the reflected attribute handling exposed by CORS'
crossOrigin attribute.

[haraken, 2014-01-08 10:45:37]

Specifically, on what DOM attributes are you going to use [ReflectEmpty],
[ReflectInvalid] etc? I'm not really happy to implement things that are not yet
used.

https://codereview.chromium.org/127903002/diff/1/Source/bindings/scripts/idl_...
File Source/bindings/scripts/idl_parser.pm (right):

https://codereview.chromium.org/127903002/diff/1/Source/bindings/scripts/idl_...
Source/bindings/scripts/idl_parser.pm:236: my $identifierTokenPattern =
'^([A-Z_a-z][0-9A-Z_a-z-]*)';

This isn't a right change.

The Web IDL grammar says the identifier should be [A-Z_a-z][0-9A-Z_a-z]*

http://www.w3.org/TR/WebIDL/#idl-grammar

[sof, 2014-01-08 10:49:58]

On 2014/01/08 10:45:37, haraken wrote:
> Specifically, on what DOM attributes are you going to use [ReflectEmpty],
> [ReflectInvalid] etc? I'm not really happy to implement things that are not
yet
> used.
> 

It makes sense to keep CLs apart.
http://code.google.com/p/chromium/issues/detail?id=332346

[sof, 2014-01-08 10:50:12]

https://codereview.chromium.org/127903002/diff/1/Source/bindings/scripts/idl_...
File Source/bindings/scripts/idl_parser.pm (right):

https://codereview.chromium.org/127903002/diff/1/Source/bindings/scripts/idl_...
Source/bindings/scripts/idl_parser.pm:236: my $identifierTokenPattern =
'^([A-Z_a-z][0-9A-Z_a-z-]*)';
On 2014/01/08 10:45:38, haraken wrote:
> 
> This isn't a right change.
> 
> The Web IDL grammar says the identifier should be [A-Z_a-z][0-9A-Z_a-z]*
> 
> http://www.w3.org/TR/WebIDL/#idl-grammar

Either the "identifier" syntactic category is reused to encompass this, or a new
one is needed.

[haraken, 2014-01-08 10:58:39]

On 2014/01/08 10:50:12, sof wrote:
>
https://codereview.chromium.org/127903002/diff/1/Source/bindings/scripts/idl_...
> File Source/bindings/scripts/idl_parser.pm (right):
> 
>
https://codereview.chromium.org/127903002/diff/1/Source/bindings/scripts/idl_...
> Source/bindings/scripts/idl_parser.pm:236: my $identifierTokenPattern =
> '^([A-Z_a-z][0-9A-Z_a-z-]*)';
> On 2014/01/08 10:45:38, haraken wrote:
> > 
> > This isn't a right change.
> > 
> > The Web IDL grammar says the identifier should be [A-Z_a-z][0-9A-Z_a-z]*
> > 
> > http://www.w3.org/TR/WebIDL/#idl-grammar
> 
> Either the "identifier" syntactic category is reused to encompass this, or a
new
> one is needed.

We want to keep the IDL parser strictly conformant with the IDL spec. From that
perspective, where is the right place to make your change?

[sof, 2014-01-08 11:01:16]

On 2014/01/08 10:58:39, haraken wrote:
> On 2014/01/08 10:50:12, sof wrote:
> >
>
https://codereview.chromium.org/127903002/diff/1/Source/bindings/scripts/idl_...
> > File Source/bindings/scripts/idl_parser.pm (right):
> > 
> >
>
https://codereview.chromium.org/127903002/diff/1/Source/bindings/scripts/idl_...
> > Source/bindings/scripts/idl_parser.pm:236: my $identifierTokenPattern =
> > '^([A-Z_a-z][0-9A-Z_a-z-]*)';
> > On 2014/01/08 10:45:38, haraken wrote:
> > > 
> > > This isn't a right change.
> > > 
> > > The Web IDL grammar says the identifier should be [A-Z_a-z][0-9A-Z_a-z]*
> > > 
> > > http://www.w3.org/TR/WebIDL/#idl-grammar
> > 
> > Either the "identifier" syntactic category is reused to encompass this, or a
> new
> > one is needed.
> 
> We want to keep the IDL parser strictly conformant with the IDL spec. From
that
> perspective, where is the right place to make your change?

Two options:

 - forgo & drop supporting attribute reflection.
 - allow stating reflected attribute values via string literals in these
identifier/string lists.

[haraken, 2014-01-08 11:03:10]

On 2014/01/08 10:49:58, sof wrote:
> On 2014/01/08 10:45:37, haraken wrote:
> > Specifically, on what DOM attributes are you going to use [ReflectEmpty],
> > [ReflectInvalid] etc? I'm not really happy to implement things that are not
> yet
> > used.
> > 
> 
> It makes sense to keep CLs apart.
> http://code.google.com/p/chromium/issues/detail?id=332346

Sounds OK. However, you might want to implement use cases of
[Reflect{Invalid|Only|Missing}] before implementing new things. We don't want to
have binding code that is unused, since it's easy to go rotten.

[sof, 2014-01-08 11:06:50]

On 2014/01/08 11:03:10, haraken wrote:
> On 2014/01/08 10:49:58, sof wrote:
> > On 2014/01/08 10:45:37, haraken wrote:
> > > Specifically, on what DOM attributes are you going to use [ReflectEmpty],
> > > [ReflectInvalid] etc? I'm not really happy to implement things that are
not
> > yet
> > > used.
> > > 
> > 
> > It makes sense to keep CLs apart.
> > http://code.google.com/p/chromium/issues/detail?id=332346
> 
> Sounds OK. However, you might want to implement use cases of
> [Reflect{Invalid|Only|Missing}] before implementing new things. We don't want
to
> have binding code that is unused, since it's easy to go rotten.

The use cases are what's in the spec, imo.

https://codereview.chromium.org/123833005/ is another one, and there's a bunch
more that could benefit. e.g.,
http://code.google.com/p/chromium/issues/detail?id=244688

If I combine changes that use the mechanism, I tend to be told to split CLs up.
So I don't.

[haraken, 2014-01-08 11:07:19]

> Two options:
> 
>  - forgo & drop supporting attribute reflection.
>  - allow stating reflected attribute values via string literals in these
> identifier/string lists.

I don't fully understand why you cannot implement CORS attribute following the
IDL spec. If the CORS attribute is speced correctly, you should be able to
implement it following the IDL spec. (Otherwise, it would be a spec bug.)

[sof, 2014-01-08 11:09:47]

On 2014/01/08 11:07:19, haraken wrote:
> > Two options:
> > 
> >  - forgo & drop supporting attribute reflection.
> >  - allow stating reflected attribute values via string literals in these
> > identifier/string lists.
> 
> I don't fully understand why you cannot implement CORS attribute following the
> IDL spec. If the CORS attribute is speced correctly, you should be able to
> implement it following the IDL spec. (Otherwise, it would be a spec bug.)

i.e., not using [Reflect] at all. ?

[haraken, 2014-01-08 11:12:22]

On 2014/01/08 11:06:50, sof wrote:
> On 2014/01/08 11:03:10, haraken wrote:
> > On 2014/01/08 10:49:58, sof wrote:
> > > On 2014/01/08 10:45:37, haraken wrote:
> > > > Specifically, on what DOM attributes are you going to use
[ReflectEmpty],
> > > > [ReflectInvalid] etc? I'm not really happy to implement things that are
> not
> > > yet
> > > > used.
> > > > 
> > > 
> > > It makes sense to keep CLs apart.
> > > http://code.google.com/p/chromium/issues/detail?id=332346
> > 
> > Sounds OK. However, you might want to implement use cases of
> > [Reflect{Invalid|Only|Missing}] before implementing new things. We don't
want
> to
> > have binding code that is unused, since it's easy to go rotten.
> 
> The use cases are what's in the spec, imo.
> 
> https://codereview.chromium.org/123833005/ is another one,

Thanks for the link! Makes sense.

(My point is just that we shouldn't have code that is unused. Even if the code
has use cases in the spec, we don't want to have the code in trunk unless we
implement the use cases.)

[sof, 2014-01-08 11:18:24]

On 2014/01/08 11:12:22, haraken wrote:
> On 2014/01/08 11:06:50, sof wrote:
> > On 2014/01/08 11:03:10, haraken wrote:
> > > On 2014/01/08 10:49:58, sof wrote:
> > > > On 2014/01/08 10:45:37, haraken wrote:
> > > > > Specifically, on what DOM attributes are you going to use
> [ReflectEmpty],
> > > > > [ReflectInvalid] etc? I'm not really happy to implement things that
are
> > not
> > > > yet
> > > > > used.
> > > > > 
> > > > 
> > > > It makes sense to keep CLs apart.
> > > > http://code.google.com/p/chromium/issues/detail?id=332346
> > > 
> > > Sounds OK. However, you might want to implement use cases of
> > > [Reflect{Invalid|Only|Missing}] before implementing new things. We don't
> want
> > to
> > > have binding code that is unused, since it's easy to go rotten.
> > 
> > The use cases are what's in the spec, imo.
> > 
> > https://codereview.chromium.org/123833005/ is another one,
> 
> Thanks for the link! Makes sense.
> 
> (My point is just that we shouldn't have code that is unused. Even if the code
> has use cases in the spec, we don't want to have the code in trunk unless we
> implement the use cases.)

Certainly, I am not landing dead-end extensions to the code generator. But if
they're of sufficient & broad enough value could be questioned.

[haraken, 2014-01-08 11:21:00]

On 2014/01/08 11:09:47, sof wrote:
> On 2014/01/08 11:07:19, haraken wrote:
> > > Two options:
> > > 
> > >  - forgo & drop supporting attribute reflection.
> > >  - allow stating reflected attribute values via string literals in these
> > > identifier/string lists.
> > 
> > I don't fully understand why you cannot implement CORS attribute following
the
> > IDL spec. If the CORS attribute is speced correctly, you should be able to
> > implement it following the IDL spec. (Otherwise, it would be a spec bug.)
> 
> i.e., not using [Reflect] at all. ?


(Sorry I don't have time to read the spec now.)

Q: The "identifier" in the IDL spec doesn't allow "-". However, a CORS attribute
in the HTML spec uses "-" (e.g., "use-credential"). How are these specs handling
this difference?

[haraken, 2014-01-08 11:22:00]

> > Thanks for the link! Makes sense.
> > 
> > (My point is just that we shouldn't have code that is unused. Even if the
code
> > has use cases in the spec, we don't want to have the code in trunk unless we
> > implement the use cases.)
> 
> Certainly, I am not landing dead-end extensions to the code generator. But if
> they're of sufficient & broad enough value could be questioned.

yeah, sorry I didn't know your changes and was a bit too harsh.

[Nils Barth (inactive), 2014-01-08 11:28:28]

Comment on the grammar.

https://codereview.chromium.org/127903002/diff/1/Source/bindings/scripts/idl_...
File Source/bindings/scripts/idl_parser.pm (right):

https://codereview.chromium.org/127903002/diff/1/Source/bindings/scripts/idl_...
Source/bindings/scripts/idl_parser.pm:236: my $identifierTokenPattern =
'^([A-Z_a-z][0-9A-Z_a-z-]*)';
To concur with haraken:
AFAICT, we want this to hold string values, so we should use string literals,
not abuse identifiers.

Simplest would be to allow extended attribute values to be (lists of)
identifiers *or* string literals; that's not much of a stretch from where we
currently are.

Other thoughts:

For ReflectOnly, where we want to give an enumerated list of values, most
natural seems to be:
define an enumeration for the allowed values
(the enumeration has an name, which is an identifier),
and use the enumeration name as the value.
E.g.,
enum CorsValues {"anonymous", "use-credentials"};

...
    [Reflect, ReflectOnly=CorsValues]
(This doesn't handle ReflectEmpty though.)

The other alternative that comes to mind is to allow string literals in extended
attribute values.
This seems ok, as we're already abusing extended attribute values to allow
A=X|Y, which violates spec, and extended attributes are exactly the place that
we do implementation-specific things.

...so we'd write:
[Reflect, ReflectOnly="anonymous"|"use-credentials", ReflectEmpty="anonymous",
ReflectInvalid="anonymous"]

The basic problem is that we want to include more complex data in extended
attributes, so key-value pairs (where value is an identifier) is really
limiting.

[sof, 2014-01-08 11:30:20]

On 2014/01/08 11:21:00, haraken wrote:
> On 2014/01/08 11:09:47, sof wrote:
> > On 2014/01/08 11:07:19, haraken wrote:
> > > > Two options:
> > > > 
> > > >  - forgo & drop supporting attribute reflection.
> > > >  - allow stating reflected attribute values via string literals in these
> > > > identifier/string lists.
> > > 
> > > I don't fully understand why you cannot implement CORS attribute following
> the
> > > IDL spec. If the CORS attribute is speced correctly, you should be able to
> > > implement it following the IDL spec. (Otherwise, it would be a spec bug.)
> > 
> > i.e., not using [Reflect] at all. ?
> 
> 
> (Sorry I don't have time to read the spec now.)
> 
> Q: The "identifier" in the IDL spec doesn't allow "-". However, a CORS
attribute
> in the HTML spec uses "-" (e.g., "use-credential"). How are these specs
handling
> this difference?

The CORS attribute keywords are given in the HTML spec for the reflected content
attribute, and not specified via WebIDL. The IDL attribute 'crossOrigin' is just
loosely specified via a DOMString (along with side conditions in prose.)

If it were to be encoded more fully in WebIDL, it'd be mapped to something like
an "enum" type there, I think. But that would still leave out details
surrounding empty/invalid/missing states.

[haraken, 2014-01-08 12:38:41]

> The basic problem is that we want to include more complex data in extended
> attributes, so key-value pairs (where value is an identifier) is really
> limiting.

Yeah, probably we want to use strings for values. We're already using various
values that are not allowed in the IDL spec on purpose, so it would make sense
to use strings for values (instead of identifiers).

[haraken, 2014-01-08 12:40:57]

(BTW, it's a bit sad we need so many IDL attributes just for [Reflect], although
I believe all of them make sense:-)

[sof, 2014-01-08 12:43:19]

On 2014/01/08 12:38:41, haraken wrote:
> > The basic problem is that we want to include more complex data in extended
> > attributes, so key-value pairs (where value is an identifier) is really
> > limiting.
> 
> Yeah, probably we want to use strings for values. We're already using various
> values that are not allowed in the IDL spec on purpose, so it would make sense
> to use strings for values (instead of identifiers).

I could switch to using an (otherwise unused) enum type for [ReflectOnly] &
state the attribute values via it. It would still require allowing string
literals on the RHS of [ReflectMissing] et al. This would need to be specified
somehow via the markup in IDLExtendedAttributes.txt.

[sof, 2014-01-08 12:52:45]

On 2014/01/08 12:40:57, haraken wrote:
> (BTW, it's a bit sad we need so many IDL attributes just for [Reflect],
although
> I believe all of them make sense:-)

As mentioned earlier, an option is to encode special properties of some values
by using special markup. e.g.,

  enum CORSAttributes {
      "{Empty, Invalid}anonymous",
      "use-credentials"
  };

  ...
  [Reflect, ReflectOnly=CORSAttributes] attribute DOMString crossOrigin;

which would work unless someone does the unusual thing of using "{}" in
attribute values somewhere.

[haraken, 2014-01-08 12:55:25]

On 2014/01/08 12:52:45, sof wrote:
> On 2014/01/08 12:40:57, haraken wrote:
> > (BTW, it's a bit sad we need so many IDL attributes just for [Reflect],
> although
> > I believe all of them make sense:-)
> 
> As mentioned earlier, an option is to encode special properties of some values
> by using special markup. e.g.,
> 
>   enum CORSAttributes {
>       "{Empty, Invalid}anonymous",
>       "use-credentials"
>   };
> 
>   ...
>   [Reflect, ReflectOnly=CORSAttributes] attribute DOMString crossOrigin;
> 
> which would work unless someone does the unusual thing of using "{}" in
> attribute values somewhere.

Yeah. I feel IDL attributes are more straightforward and thus better, but others
might have other opinions.

[sof, 2014-01-08 14:44:39]

On 2014/01/08 11:30:20, sof wrote:
> On 2014/01/08 11:21:00, haraken wrote:
> > On 2014/01/08 11:09:47, sof wrote:
> > > On 2014/01/08 11:07:19, haraken wrote:
> > > > > Two options:
> > > > > 
> > > > >  - forgo & drop supporting attribute reflection.
> > > > >  - allow stating reflected attribute values via string literals in
these
> > > > > identifier/string lists.
> > > > 
> > > > I don't fully understand why you cannot implement CORS attribute
following
> > the
> > > > IDL spec. If the CORS attribute is speced correctly, you should be able
to
> > > > implement it following the IDL spec. (Otherwise, it would be a spec
bug.)
> > > 
> > > i.e., not using [Reflect] at all. ?
> > 
> > 
> > (Sorry I don't have time to read the spec now.)
> > 
> > Q: The "identifier" in the IDL spec doesn't allow "-". However, a CORS
> attribute
> > in the HTML spec uses "-" (e.g., "use-credential"). How are these specs
> handling
> > this difference?
> 
> The CORS attribute keywords are given in the HTML spec for the reflected
content
> attribute, and not specified via WebIDL. The IDL attribute 'crossOrigin' is
just
> loosely specified via a DOMString (along with side conditions in prose.)
> 
> If it were to be encoded more fully in WebIDL, it'd be mapped to something
like
> an "enum" type there, I think. But that would still leave out details
> surrounding empty/invalid/missing states.

Done, switched [ReflectOnly] over to take an enumeration type, along with the
[ReflectEmpty] et al attributes using string literals on their right hand sides.

This is very much along the lines of nbarth's (fine) suggestion - is this
heading in the right direction overall?

[Nils Barth (inactive), 2014-01-09 00:55:18]

On 2014/01/08 14:44:39, sof wrote:
> Done, switched [ReflectOnly] over to take an enumeration type, along with the
> [ReflectEmpty] et al attributes using string literals on their right hand
sides.
> 
> This is very much along the lines of nbarth's (fine) suggestion - is this
> heading in the right direction overall?

Thanks Sigbjørn - from the POV of grammar/parser this looks fine!
(LGTM for grammar.)
(Very minimal change, just adding one rule to the ExtendedAttribute grammar.)

I'm happy to handle the Python side once this lands,
unless you want to have a look at the Python parser (it's quite legible,
really).

[haraken, 2014-01-09 01:12:31]

I read the spec and probably I'm changing my mind (sorry about that).

- The HTML spec doesn't have enum. Thus we shouldn't have the enum either. We
don't want to introduce our new rule to Blink IDLs as much as possible.

- We need to allow "-" in extended attribute values. This is not speced in the
IDL spec, but we want to allow it to auto-generate code as much as possible.

- However, we shouldn't change the grammar of "identifier". There are two
options to address this issue. The first option is to allow "string" in extended
attribute values. However, this is confusing because developers sometimes use
"string" and sometimes use "identifier" in Blink IDL files. The second option is
to define "identifierAndMinus" and use it in extended attribute values. I'd
prefer this approach.

WDYT?

I'm sorry that my opinions are going back and forth.

[Nils Barth (inactive), 2014-01-09 01:18:40]

On 2014/01/09 01:12:31, haraken wrote:
> I read the spec and probably I'm changing my mind (sorry about that).
> 
> - The HTML spec doesn't have enum. Thus we shouldn't have the enum either. We
> don't want to introduce our new rule to Blink IDLs as much as possible.

I was thinking this as well;
this is easily handled by instead inlining the list, as
[ReflectOnly="Foo"|"Bar"]
This is also clearer b/c it keeps the values *at* the relevant line, not
somewhere else.

> - However, we shouldn't change the grammar of "identifier". There are two
> options to address this issue. The first option is to allow "string" in
extended
> attribute values. However, this is confusing because developers sometimes use
> "string" and sometimes use "identifier" in Blink IDL files.

I think using string literals is clearer: these values are strings, so
representing them as string literals is correct.
This allows minimal change to the grammar -- just allow string literals in
extended attributes -- without introducing a new token class.
Note that currently our lexical grammar is 100% compliant to spec, so (once we
upstream one fix), we don't need a Blink-specific lexer, which is a big code
savings.

[haraken, 2014-01-09 01:31:20]

On 2014/01/09 01:18:40, Nils Barth wrote:
> On 2014/01/09 01:12:31, haraken wrote:
> > I read the spec and probably I'm changing my mind (sorry about that).
> > 
> > - The HTML spec doesn't have enum. Thus we shouldn't have the enum either.
We
> > don't want to introduce our new rule to Blink IDLs as much as possible.
> 
> I was thinking this as well;
> this is easily handled by instead inlining the list, as
> [ReflectOnly="Foo"|"Bar"]
> This is also clearer b/c it keeps the values *at* the relevant line, not
> somewhere else.
> 
> > - However, we shouldn't change the grammar of "identifier". There are two
> > options to address this issue. The first option is to allow "string" in
> extended
> > attribute values. However, this is confusing because developers sometimes
use
> > "string" and sometimes use "identifier" in Blink IDL files.
> 
> I think using string literals is clearer: these values are strings, so
> representing them as string literals is correct.
> This allows minimal change to the grammar -- just allow string literals in
> extended attributes -- without introducing a new token class.
> Note that currently our lexical grammar is 100% compliant to spec, so (once we
> upstream one fix), we don't need a Blink-specific lexer, which is a big code
> savings.

This makes sense. Let's go with [ReflectOnly="Foo"|"Bar"].

[sof, 2014-01-09 09:51:48]

On 2014/01/09 01:31:20, haraken wrote:
> On 2014/01/09 01:18:40, Nils Barth wrote:
> > On 2014/01/09 01:12:31, haraken wrote:
> > > I read the spec and probably I'm changing my mind (sorry about that).
> > > 
> > > - The HTML spec doesn't have enum. Thus we shouldn't have the enum either.
> We
> > > don't want to introduce our new rule to Blink IDLs as much as possible.
> > 
> > I was thinking this as well;
> > this is easily handled by instead inlining the list, as
> > [ReflectOnly="Foo"|"Bar"]
> > This is also clearer b/c it keeps the values *at* the relevant line, not
> > somewhere else.
> > 
> > > - However, we shouldn't change the grammar of "identifier". There are two
> > > options to address this issue. The first option is to allow "string" in
> > extended
> > > attribute values. However, this is confusing because developers sometimes
> use
> > > "string" and sometimes use "identifier" in Blink IDL files.
> > 
> > I think using string literals is clearer: these values are strings, so
> > representing them as string literals is correct.
> > This allows minimal change to the grammar -- just allow string literals in
> > extended attributes -- without introducing a new token class.
> > Note that currently our lexical grammar is 100% compliant to spec, so (once
we
> > upstream one fix), we don't need a Blink-specific lexer, which is a big code
> > savings.
> 
> This makes sense. Let's go with [ReflectOnly="Foo"|"Bar"].

Done. Should the inlining lead to excessive duplication, then that can be
addressed later.

I've not added the required parser productions to the python parser. Want to at
least wait until the syntax issues have been resolved here.

[haraken, 2014-01-09 10:19:04]

LGTM

https://codereview.chromium.org/127903002/diff/320001/Source/bindings/scripts...
File Source/bindings/scripts/idl_parser.pm (right):

https://codereview.chromium.org/127903002/diff/320001/Source/bindings/scripts...
Source/bindings/scripts/idl_parser.pm:1623: if ($next->type() == StringToken) {

Would you add a comment that this is a syntax extension in Blink IDL files?

[sof, 2014-01-09 11:13:16]

https://codereview.chromium.org/127903002/diff/320001/Source/bindings/scripts...
File Source/bindings/scripts/idl_parser.pm (right):

https://codereview.chromium.org/127903002/diff/320001/Source/bindings/scripts...
Source/bindings/scripts/idl_parser.pm:1623: if ($next->type() == StringToken) {
On 2014/01/09 10:19:05, haraken wrote:
> 
> Would you add a comment that this is a syntax extension in Blink IDL files?

certainly; added.

I also added productions to the Python parser; please have a look that no undue
liberties were taken. (there is no [ReflectOnly] et al support in its backend
yet, though.)

[Nils Barth (inactive), 2014-01-09 11:24:16]

On 2014/01/09 11:13:16, sof wrote:
> I also added productions to the Python parser; please have a look that no
undue
> liberties were taken.

Thanks, looks fine!
(Except for comment formatting; sent separately.)
Agreed with evaluating the string literals; that's what we're doing for
identifier lists also.
(We're not returning a tree with nodes the individual terms, which would be
gratuitous.)

[Nils Barth (inactive), 2014-01-09 11:24:26]

https://codereview.chromium.org/127903002/diff/380001/Source/bindings/scripts...
File Source/bindings/scripts/unstable/blink_idl_parser.py (right):

https://codereview.chromium.org/127903002/diff/380001/Source/bindings/scripts...
Source/bindings/scripts/unstable/blink_idl_parser.py:387: """
One-line docstring please:
http://www.python.org/dev/peps/pep-0257/#one-line-docstrings

This docstring can be one line:
"""Reach in..."""

BTW,
Generally multiline are:
"""Foo

Bar
Zork
"""
(without the leading newline)

Ref:
http://www.python.org/dev/peps/pep-0257/

[haraken, 2014-01-09 11:24:29]

> I also added productions to the Python parser; please have a look that no
undue
> liberties were taken. (there is no [ReflectOnly] et al support in its backend
> yet, though.)

nbarth@ ?

[sof, 2014-01-09 11:42:34]

https://codereview.chromium.org/127903002/diff/380001/Source/bindings/scripts...
File Source/bindings/scripts/unstable/blink_idl_parser.py (right):

https://codereview.chromium.org/127903002/diff/380001/Source/bindings/scripts...
Source/bindings/scripts/unstable/blink_idl_parser.py:387: """
On 2014/01/09 11:24:27, Nils Barth wrote:
> One-line docstring please:
> http://www.python.org/dev/peps/pep-0257/#one-line-docstrings
> 
> This docstring can be one line:
> """Reach in..."""
> 

thanks, updated.

[sof, 2014-01-09 12:35:35]

Assuming we're ok now & pushing this one along.

Thanks both, really appreciate it -- I'll update the extended attributes
document.

[commit-bot: I haz the power, 2014-01-09 12:36:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/127903002/460001

[commit-bot: I haz the power, 2014-01-09 13:45:23]

Change committed as 164776

[Nils Barth (inactive), 2014-02-05 07:13:19]

Python side is:
IDL compiler: [ReflectEmpty]
https://codereview.chromium.org/155623004/