Support WebIDL's ExtendedAttributeIdentList in the Blink parser

Support WebIDL's ExtendedAttributeIdentList in the Blink parser

A change was made to the WebIDL parser on July 29 to support a list of
identifiers as part of an extended attribute's value. This cleanly maps
to the concept of the IdentAndList (a Blink extension) we already had.

All uses of the IdentAndList have been updated to use the specification-
compliant syntax. The IdentOrList remains in place.

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

[Peter Beverloo, 2014-08-01 16:14:03]

https://codereview.chromium.org/434133002/diff/1/Source/bindings/scripts/util...
File Source/bindings/scripts/utilities.py (right):

https://codereview.chromium.org/434133002/diff/1/Source/bindings/scripts/util...
Source/bindings/scripts/utilities.py:157: # multiple values are used, which are
seperated by a comma and a space.
Are there plans to make these scripts use the actual IDL parser instead of
regular expressions (I presume this was done for performance reasons)?

Going from a bad hack to another (arguably slightly less) bad hack is sad :-(.

[Jens Widell, 2014-08-01 16:41:20]

Haven't looked at the patch yet, but an initial comment: if at all possible,
it's nicer IMO if the syntax can be supported in the base IDL parser (in
tools/idl_parser/) rather than as a blink extension. Having extensions can make
it more complicated to add (standard) features to the base parser in the future.

See https://codereview.chromium.org/333853002/ for an example of when grammar
extensions are somewhat boring.

[Peter Beverloo, 2014-08-01 16:52:25]

On 2014/08/01 16:41:20, Jens Lindström wrote:
> Haven't looked at the patch yet, but an initial comment: if at all possible,
> it's nicer IMO if the syntax can be supported in the base IDL parser (in
> tools/idl_parser/) rather than as a blink extension. Having extensions can
make
> it more complicated to add (standard) features to the base parser in the
future.
> 
> See https://codereview.chromium.org/333853002/ for an example of when grammar
> extensions are somewhat boring.

Ah, yes, agreed. I will move the implementation there. Thanks!

[haraken, 2014-08-01 16:55:55]

On 2014/08/01 16:52:25, Peter Beverloo wrote:
> On 2014/08/01 16:41:20, Jens Lindström wrote:
> > Haven't looked at the patch yet, but an initial comment: if at all possible,
> > it's nicer IMO if the syntax can be supported in the base IDL parser (in
> > tools/idl_parser/) rather than as a blink extension. Having extensions can
> make
> > it more complicated to add (standard) features to the base parser in the
> future.
> > 
> > See https://codereview.chromium.org/333853002/ for an example of when
grammar
> > extensions are somewhat boring.
> 
> Ah, yes, agreed. I will move the implementation there. Thanks!

Agreed2.

[Peter Beverloo, 2014-08-05 19:54:13]

I've moved the main parsing implementation to Chromium:
  https://codereview.chromium.org/447543002/

This patch now depends on that one.

We still need to override ExtendedAttributeIdentList in this patch, in order to
continue supporting OR lists. Blink also coalesces the values into a string,
whereas Chromium keeps them as a list.

In the future, we may want to look into making the syntax for OR lists more
similar. Perhaps we can support the following?

AND:
  Foo=(Bar,Baz)

OR:
  Foo=(Bar|Baz)

[haraken, 2014-08-06 00:58:47]

On 2014/08/05 19:54:13, Peter Beverloo wrote:
> I've moved the main parsing implementation to Chromium:
>   https://codereview.chromium.org/447543002/
> 
> This patch now depends on that one.
> 
> We still need to override ExtendedAttributeIdentList in this patch, in order
to
> continue supporting OR lists. Blink also coalesces the values into a string,
> whereas Chromium keeps them as a list.
> 
> In the future, we may want to look into making the syntax for OR lists more
> similar. Perhaps we can support the following?
> 
> AND:
>   Foo=(Bar,Baz)
> 
> OR:
>   Foo=(Bar|Baz)

I prefer following the spec, so "Foo=(Bar,Baz)".

[Peter Beverloo, 2014-08-06 01:00:33]

On 2014/08/06 00:58:47, haraken wrote:
> On 2014/08/05 19:54:13, Peter Beverloo wrote:
> > I've moved the main parsing implementation to Chromium:
> >   https://codereview.chromium.org/447543002/
> > 
> > This patch now depends on that one.
> > 
> > We still need to override ExtendedAttributeIdentList in this patch, in order
> to
> > continue supporting OR lists. Blink also coalesces the values into a string,
> > whereas Chromium keeps them as a list.
> > 
> > In the future, we may want to look into making the syntax for OR lists more
> > similar. Perhaps we can support the following?
> > 
> > AND:
> >   Foo=(Bar,Baz)
> > 
> > OR:
> >   Foo=(Bar|Baz)
> 
> I prefer following the spec, so "Foo=(Bar,Baz)".

Yes, but these are for AND lists. We're replacing Blink's "Foo=A&B" syntax with
"Foo=(A,B)".

For OR lists, we currently support "Foo=A|B". We could match the new syntax
closer by considering "Foo=(A|B)" -- the specification does not define how OR
lists should work. But that's orthogonal to this CL :-).

[haraken, 2014-08-06 01:03:40]

On 2014/08/06 01:00:33, Peter Beverloo wrote:
> On 2014/08/06 00:58:47, haraken wrote:
> > On 2014/08/05 19:54:13, Peter Beverloo wrote:
> > > I've moved the main parsing implementation to Chromium:
> > >   https://codereview.chromium.org/447543002/
> > > 
> > > This patch now depends on that one.
> > > 
> > > We still need to override ExtendedAttributeIdentList in this patch, in
order
> > to
> > > continue supporting OR lists. Blink also coalesces the values into a
string,
> > > whereas Chromium keeps them as a list.
> > > 
> > > In the future, we may want to look into making the syntax for OR lists
more
> > > similar. Perhaps we can support the following?
> > > 
> > > AND:
> > >   Foo=(Bar,Baz)
> > > 
> > > OR:
> > >   Foo=(Bar|Baz)
> > 
> > I prefer following the spec, so "Foo=(Bar,Baz)".
> 
> Yes, but these are for AND lists. We're replacing Blink's "Foo=A&B" syntax
with
> "Foo=(A,B)".
> 
> For OR lists, we currently support "Foo=A|B". We could match the new syntax
> closer by considering "Foo=(A|B)" -- the specification does not define how OR
> lists should work. But that's orthogonal to this CL :-).

ah, understood. I agree with you. "Foo=(A|B)" sounds nicer (than "Foo=A|B") once
we support "Foo=(Bar,Baz)".

[haraken, 2014-08-06 01:08:49]

LGTM

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

https://codereview.chromium.org/434133002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/utilities.py:159: for extended_attribute in
re.split(',\s+', extended_attributes_string)

I wonder if this should be re.split(',\s*', ...)

You want to split:

"a,b" => ["a", "b"]
"a, b" => ["a", "b"]

don't you?

[Jens Widell, 2014-08-06 08:25:05]

On 2014/08/06 01:00:33, Peter Beverloo wrote:
> On 2014/08/06 00:58:47, haraken wrote:
> > I prefer following the spec, so "Foo=(Bar,Baz)".

I do too.

> Yes, but these are for AND lists. We're replacing Blink's "Foo=A&B" syntax
with
> "Foo=(A,B)".
> 
> For OR lists, we currently support "Foo=A|B". We could match the new syntax
> closer by considering "Foo=(A|B)" -- the specification does not define how OR
> lists should work. But that's orthogonal to this CL :-).

Do we have any attribute that could take *either* an AND list or an OR list? If
not, then we don't really need different syntax for the two types of list; both
would just be a list of identifiers, and the attribute's consumer determines
what that list means (which it of course does either way.)

If it was free (had no complexity cost) I'd say having an AND syntax and an OR
syntax seems like a good idea, but it isn't: WebIDL only has one,
tools/idl_parser/ only has one (and should, assuming we want it to parse WebIDL
grammar and nothing else) meaning we still need to override and extend parts of
the grammar in the Blink IDL parser, which is non-free.

[Peter Beverloo, 2014-08-07 13:32:37]

Thanks for the comments! I'll put this on the commit queue since it's causing
some build spew.

On 2014/08/06 08:25:05, Jens Lindström wrote:
> Do we have any attribute that could take *either* an AND list or an OR list?
If
> not, then we don't really need different syntax for the two types of list;
both
> would just be a list of identifiers, and the attribute's consumer determines
> what that list means (which it of course does either way.)

I'm inclined to disagree because it would significantly hurt readability of IDL
files.

One situation in which we support both are compile-time conditionals (see
v8_utilities.py).

[Conditional=Foo&Bar|Baz]

Would expand to:

#if ENABLE(FOO) && ENABLE(BAR) || ENABLE(BAZ)

This mechanism isn't being used anymore by any of our IDL files, however :-).

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

https://codereview.chromium.org/434133002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/utilities.py:159: for extended_attribute in
re.split(',\s+', extended_attributes_string)
On 2014/08/06 01:08:48, haraken wrote:
> 
> I wonder if this should be re.split(',\s*', ...)
> 
> You want to split:
> 
> "a,b" => ["a", "b"]
> "a, b" => ["a", "b"]
> 
> don't you?

This regular expression essentially is mimicing the ExtendedAttributeList IDL
rule -- it creates a list of the extended attributes, together with their
values.

Given the following IDL extended attribute block:

[Foo,
 Bar=Baz(a, b),
 Cond=(A,B),]

In the previous hack, the following would happen:
    len=5, having ("Foo", "Bar=Baz(a", "b)", "Cond=(A", "B)")

In the new hack, the following will happen:
    len=4, having ("Foo", "Bar=Baz(a", "b)", "Cond=(A,B)")

Since \s* means zero or more spaces, the expression would be identical to the
comma-split we previously did.

It's a hack, but it's compatible with the Blink IDL style. The real solution
here would be to have a smarter parsing mechanism, ideally using the normal IDL
parser, but I suspect that there will be performance worries.

[Peter Beverloo, 2014-08-07 13:32:45]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2014-08-07 13:33:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/peter@chromium.org/434133002/20001

[commit-bot: I haz the power, 2014-08-07 13:33:35]

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

[commit-bot: I haz the power, 2014-08-07 13:33:36]

Failed to apply patch for Source/modules/geolocation/CircularRegion.idl:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: Source/modules/geolocation/CircularRegion.idl
  |diff --git a/Source/modules/geolocation/CircularRegion.idl
b/Source/modules/geolocation/CircularRegion.idl
  |index
c4121e13ed840cfbc941438b75321cb870dce747..19278fdead00c2288e8bb6c12ce30983be822739
100644
  |--- a/Source/modules/geolocation/CircularRegion.idl
  |+++ b/Source/modules/geolocation/CircularRegion.idl
  --------------------------
  No file to patch.  Skipping patch.
  1 out of 1 hunk ignored

Patch:       Source/modules/geolocation/CircularRegion.idl
Index: Source/modules/geolocation/CircularRegion.idl
diff --git a/Source/modules/geolocation/CircularRegion.idl
b/Source/modules/geolocation/CircularRegion.idl
index
c4121e13ed840cfbc941438b75321cb870dce747..19278fdead00c2288e8bb6c12ce30983be822739
100644
--- a/Source/modules/geolocation/CircularRegion.idl
+++ b/Source/modules/geolocation/CircularRegion.idl
@@ -4,7 +4,7 @@
 
 [
     RuntimeEnabled=Geofencing,
-    Exposed=Window&Worker,
+    Exposed=(Window,Worker),
     GarbageCollected,
     Constructor(Dictionary init)
 ] interface CircularRegion : GeofencingRegion {

[Jens Widell, 2014-08-07 13:42:49]

On 2014/08/07 13:32:37, Peter Beverloo wrote:
> Thanks for the comments! I'll put this on the commit queue since it's causing
> some build spew.

Yes, we should get that in ASAP.

> On 2014/08/06 08:25:05, Jens Lindström wrote:
> > Do we have any attribute that could take *either* an AND list or an OR list?
> If
> > not, then we don't really need different syntax for the two types of list;
> both
> > would just be a list of identifiers, and the attribute's consumer determines
> > what that list means (which it of course does either way.)
> 
> I'm inclined to disagree because it would significantly hurt readability of
IDL
> files.
> 
> One situation in which we support both are compile-time conditionals (see
> v8_utilities.py).
> 
> [Conditional=Foo&Bar|Baz]
> 
> Would expand to:
> 
> #if ENABLE(FOO) && ENABLE(BAR) || ENABLE(BAZ)
> 
> This mechanism isn't being used anymore by any of our IDL files, however :-).

The [Conditional] extended attribute supported *either* A&B&C or A|B|C, but not
A&B|C, I think. (And if it was meant to support arbitrary expressions, it ought
to have needed to support A&(B|C) as well, which it also didn't.)

It still supports both A&B&C and A|B|C, but the former is now expressed as
(A,B,C) instead, which I reckon already hurts readability quite a lot.
Supporting [ConditionalOr=(A,B,C)] and [ConditionalAnd=(A,B,C)] would be better
than what we have after this patch, and not really a whole lot worse than we had
before, IMO.

[haraken, 2014-08-07 14:16:35]

> > I'm inclined to disagree because it would significantly hurt readability of
> IDL
> > files.
> > 
> > One situation in which we support both are compile-time conditionals (see
> > v8_utilities.py).
> > 
> > [Conditional=Foo&Bar|Baz]
> > 
> > Would expand to:
> > 
> > #if ENABLE(FOO) && ENABLE(BAR) || ENABLE(BAZ)
> > 
> > This mechanism isn't being used anymore by any of our IDL files, however
:-).
> 
> The [Conditional] extended attribute supported *either* A&B&C or A|B|C, but
not
> A&B|C, I think. (And if it was meant to support arbitrary expressions, it
ought
> to have needed to support A&(B|C) as well, which it also didn't.)
> 
> It still supports both A&B&C and A|B|C, but the former is now expressed as
> (A,B,C) instead, which I reckon already hurts readability quite a lot.
> Supporting [ConditionalOr=(A,B,C)] and [ConditionalAnd=(A,B,C)] would be
better
> than what we have after this patch, and not really a whole lot worse than we
had
> before, IMO.

Since we're replacing [Conditional] with [RuntimeEnabled], a right thing to do
would be to deprecate [Conditional] (instead of making [Conditional] more
powerful).

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

https://codereview.chromium.org/434133002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/utilities.py:159: for extended_attribute in
re.split(',\s+', extended_attributes_string)
On 2014/08/07 13:32:37, Peter Beverloo wrote:
> On 2014/08/06 01:08:48, haraken wrote:
> > 
> > I wonder if this should be re.split(',\s*', ...)
> > 
> > You want to split:
> > 
> > "a,b" => ["a", "b"]
> > "a, b" => ["a", "b"]
> > 
> > don't you?
> 
> This regular expression essentially is mimicing the ExtendedAttributeList IDL
> rule -- it creates a list of the extended attributes, together with their
> values.
> 
> Given the following IDL extended attribute block:
> 
> [Foo,
>  Bar=Baz(a, b),
>  Cond=(A,B),]
> 
> In the previous hack, the following would happen:
>     len=5, having ("Foo", "Bar=Baz(a", "b)", "Cond=(A", "B)")
> 
> In the new hack, the following will happen:
>     len=4, having ("Foo", "Bar=Baz(a", "b)", "Cond=(A,B)")
> 
> Since \s* means zero or more spaces, the expression would be identical to the
> comma-split we previously did.
> 
> It's a hack, but it's compatible with the Blink IDL style. The real solution
> here would be to have a smarter parsing mechanism, ideally using the normal
IDL
> parser, but I suspect that there will be performance worries.

Makes sense. Since we don't care about performance of the IDL compiler, it's
better to integrate the logic to the normal IDL parser.

[Peter Beverloo, 2014-08-07 15:54:01]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2014-08-07 15:55:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/peter@chromium.org/434133002/40001

[commit-bot: I haz the power, 2014-08-07 17:18:10]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[commit-bot: I haz the power, 2014-08-07 18:34:27]

Change committed as 179736