Bug 28449 – [v8] Use atomic string caching for createElement

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Christian Plesner Hansen]

Extend atomic string caching to createElement.  Fix a bug in idl parser that caused incorrect parsing if a function argument attribute list contained a comma.

[Christian Plesner Hansen]

Created attachment 35105 [details]
initial

[Christian Plesner Hansen]

Comment on attachment 35105 [details]
initial

Something unrelated was rolled into this patch.  Will upload a new one.

[Christian Plesner Hansen]

Created attachment 35106 [details]
Unrelated changes removed

[David Levin]

Comment on attachment 35106 [details]
Unrelated changes removed


> Index: WebCore/bindings/scripts/CodeGeneratorV8.pm

Why did this attribute change?


> Index: WebCore/bindings/scripts/IDLParser.pm
> +                # Split arguments at commas but only if the comma
> +                # is not within attribute brackets, expressed here
> +                # as being followed by a ']' without a preceding '['.
> +                # Note that this assumes that attributes don't nest.
> +                my @params = split(/,(?![^[]*])/, $methodSignature);

I don't understand how this regex does what the comment says.

It looks like it splits on commas that are followed by anything except these three characters: '[', ']', '*'
(using a negative width lookahead assertion).

[Christian Plesner Hansen]

> > Index: WebCore/bindings/scripts/CodeGeneratorV8.pm
> 
> Why did this attribute change?

It's cleanup -- using V8Custom was a hack.  V8Custom is really there to indicate that we have to use a customized implementation rather than a generated accessor or function and using it for specifying atomic hints was a bad idea in the first place.

> > Index: WebCore/bindings/scripts/IDLParser.pm
> > +                # Split arguments at commas but only if the comma
> > +                # is not within attribute brackets, expressed here
> > +                # as being followed by a ']' without a preceding '['.
> > +                # Note that this assumes that attributes don't nest.
> > +                my @params = split(/,(?![^[]*])/, $methodSignature);
> 
> I don't understand how this regex does what the comment says.
> 
> It looks like it splits on commas that are followed by anything except these
> three characters: '[', ']', '*'
> (using a negative width lookahead assertion).

The character class is closed by the first ']' so, using parens to indicate precedence, it gets parsed as ((([^[])*)]) rather than ([^[]*]).

[David Levin]

Comment on attachment 35106 [details]
Unrelated changes removed


> +++ WebCore/bindings/scripts/IDLParser.pm	(working copy)
> +                my @params = split(/,(?![^[]*])/, $methodSignature);

Although apparently valid without this, it would be nice to escape the last ] to clue in the reader that it isn't a closing ] in the regex.

[Christian Plesner Hansen]

Created attachment 35191 [details]
Escaped ']' in regexp

[Christian Plesner Hansen]

Comment on attachment 35191 [details]
Escaped ']' in regexp

Gah!  The ChangeLog disappeared.  Will upload a new patch.

[Christian Plesner Hansen]

Created attachment 35192 [details]
Included ChangeLog

[Eric Seidel (no email)]

Comment on attachment 35192 [details]
Included ChangeLog

Rejecting patch 35192 from commit-queue.  This patch will require manual commit.

Found no modified ChangeLogs, cannot create a commit message.
All changes require a ChangeLog.  See:
http://webkit.org/coding/contributing.html

[Christian Plesner Hansen]

(In reply to comment #10)
> (From update of attachment 35192 [details])
> Rejecting patch 35192 from commit-queue.  This patch will require manual
> commit.
> 
> Found no modified ChangeLogs, cannot create a commit message.
> All changes require a ChangeLog.  See:
> http://webkit.org/coding/contributing.html

I accidentally left out the ChangeLog from the previous patch but the latest one does have one.

[David Levin]

I think the commit tool is having issues which are fixed in  https://bugs.webkit.org/show_bug.cgi?id=28485

[Adam Barth]

Comment on attachment 35192 [details]
Included ChangeLog

Eric commit-queue+ this patch during the blackout.

[David Levin]

Committed as http://trac.webkit.org/changeset/47562

[Eric Seidel (no email)]

Comment on attachment 35192 [details]
Included ChangeLog

Rejecting patch 35192 from commit-queue.  This patch will require manual commit.

Patch https://bugs.webkit.org/attachment.cgi?id=35192 from bug 28449 failed to download and apply.

[Eric Seidel (no email)]

The queue (correctly) rejected this because it was already landed... bugzilla just forgot.