Bug 28449 - [v8] Use atomic string caching for createElement
Summary: [v8] Use atomic string caching for createElement
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-19 00:34 PDT by Christian Plesner Hansen
Modified: 2009-08-21 09:15 PDT (History)
2 users (show)

See Also:


Attachments
initial (6.83 KB, patch)
2009-08-19 00:38 PDT, Christian Plesner Hansen
no flags Details | Formatted Diff | Diff
Unrelated changes removed (5.26 KB, patch)
2009-08-19 00:46 PDT, Christian Plesner Hansen
levin: review+
levin: commit-queue-
Details | Formatted Diff | Diff
Escaped ']' in regexp (4.37 KB, patch)
2009-08-20 01:07 PDT, Christian Plesner Hansen
no flags Details | Formatted Diff | Diff
Included ChangeLog (5.26 KB, patch)
2009-08-20 01:13 PDT, Christian Plesner Hansen
levin: review+
eric: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Christian Plesner Hansen 2009-08-19 00:34:01 PDT
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.
Comment 1 Christian Plesner Hansen 2009-08-19 00:38:33 PDT
Created attachment 35105 [details]
initial
Comment 2 Christian Plesner Hansen 2009-08-19 00:42:36 PDT
Comment on attachment 35105 [details]
initial

Something unrelated was rolled into this patch.  Will upload a new one.
Comment 3 Christian Plesner Hansen 2009-08-19 00:46:45 PDT
Created attachment 35106 [details]
Unrelated changes removed
Comment 4 David Levin 2009-08-19 15:26:00 PDT
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).
Comment 5 Christian Plesner Hansen 2009-08-19 20:46:09 PDT
> > 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 ([^[]*]).
Comment 6 David Levin 2009-08-19 21:28:49 PDT
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.
Comment 7 Christian Plesner Hansen 2009-08-20 01:07:57 PDT
Created attachment 35191 [details]
Escaped ']' in regexp
Comment 8 Christian Plesner Hansen 2009-08-20 01:09:01 PDT
Comment on attachment 35191 [details]
Escaped ']' in regexp

Gah!  The ChangeLog disappeared.  Will upload a new patch.
Comment 9 Christian Plesner Hansen 2009-08-20 01:13:57 PDT
Created attachment 35192 [details]
Included ChangeLog
Comment 10 Eric Seidel (no email) 2009-08-20 01:38:58 PDT
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
Comment 11 Christian Plesner Hansen 2009-08-20 01:48:55 PDT
(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.
Comment 12 David Levin 2009-08-20 01:58:57 PDT
I think the commit tool is having issues which are fixed in  https://bugs.webkit.org/show_bug.cgi?id=28485
Comment 13 Adam Barth 2009-08-20 23:06:39 PDT
Comment on attachment 35192 [details]
Included ChangeLog

Eric commit-queue+ this patch during the blackout.
Comment 14 David Levin 2009-08-20 23:13:20 PDT
Committed as http://trac.webkit.org/changeset/47562
Comment 15 Eric Seidel (no email) 2009-08-20 23:13:25 PDT
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.
Comment 16 Eric Seidel (no email) 2009-08-21 09:15:14 PDT
The queue (correctly) rejected this because it was already landed... bugzilla just forgot.