Bug 41949

Summary:

Implement SVG attribute case mapping for HTMLTreeBuilder

Product:

WebKit

Reporter:

Eric Seidel (no email) <eric>

Component:

New Bugs

Assignee:

Eric Seidel (no email) <eric>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, darin, hyatt

Priority:

Version:

528+ (Nightly build)

Hardware:

Other

OS:

OS X 10.5

Bug Depends on:

Bug Blocks:

41123

Attachments:

Description	Flags
Patch	none
Patch	abarth: review+, eric: commit-queue+

Description Eric Seidel (no email) 2010-07-09 04:10:45 PDT

Implement SVG attribute case mapping for HTMLTreeBuilder

Comment 1 Eric Seidel (no email) 2010-07-09 04:14:25 PDT

Created attachment 61032 [details]
Patch

Comment 2 Eric Seidel (no email) 2010-07-09 12:14:53 PDT

Created attachment 61073 [details]
Patch

Comment 3 Adam Barth 2010-07-09 12:42:51 PDT

Comment on attachment 61073 [details]
Patch

WebCore/html/HTMLTreeBuilder.cpp:742
 +      for (unsigned x = 0; x < attributes->length(); ++x) {
I'd prefer "i" here, but whatever.

WebCore/html/HTMLTreeBuilder.cpp:743
 +          Attribute* attribute = attributes->attributeItem(x);
I'm not sure what the internal structure of the attributes data structure is.  Is it efficient to index each time, or is there an iterator API we should be using.

Comment 4 Adam Barth 2010-07-09 12:44:07 PDT

+darin in case he has feedback in the pattern of introducing parser* APIs that might not be safe for general use by the rest of the code.

Comment 5 Eric Seidel (no email) 2010-07-09 13:22:44 PDT

Comment on attachment 61073 [details]
Patch

Its a vector. :)

Comment 6 Darin Adler 2010-07-09 13:56:04 PDT

(In reply to comment #4)
> +darin in case he has feedback in the pattern of introducing parser* APIs that might not be safe for general use by the rest of the code.

Thanks for cc'ing me.

I think it’s good to give the function a clear name the way you have, Eric. But also it would be best if you could state in some clear way what is "unsafe" about using this in other code. A more concrete comment would help people understand what the real issue is.

Comment 7 Eric Seidel (no email) 2010-07-09 13:59:15 PDT

Certainly.  I'm actually not 100% sure it's unsafe, but it seems possibly unsafe if others have a hash of Attributes.  I don't really know enough about the attribute system to know if this is safe or not.  I presumed it was not since QualifiedName has no setLocalName accessor, and Attribute seemed to have explicitly avoided it in the past (or at least forgotten to add one).

Comment 8 Eric Seidel (no email) 2010-07-09 14:13:14 PDT

Talked to hyatt.  parserSetLocalName() is only ssafe up until parseMappedAttributes.

Comment 9 Darin Adler 2010-07-09 14:15:07 PDT

(In reply to comment #8)
> Talked to hyatt.  parserSetLocalName() is only ssafe up until parseMappedAttributes.

One possible way to reflect that in the code is to devote a debug-only boolean to this and assert that parseMappedAttributes didn't happen yet.

Comment 10 Eric Seidel (no email) 2010-07-09 14:22:22 PDT

Committed r62994: <http://trac.webkit.org/changeset/62994>