Bug 41949 - Implement SVG attribute case mapping for HTMLTreeBuilder
Summary: Implement SVG attribute case mapping for HTMLTreeBuilder
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks: 41123
  Show dependency tree / graph
 
Reported: 2010-07-09 04:10 PDT by Eric Seidel (no email)
Modified: 2010-07-09 14:22 PDT (History)
3 users (show)

See Also:

Attachments
Patch (5.25 KB, patch)
2010-07-09 04:14 PDT, Eric Seidel (no email) 		no flags Details | Formatted Diff | Diff
Patch (21.85 KB, patch)
2010-07-09 12:14 PDT, Eric Seidel (no email) 		abarth: review+
eric: commit-queue+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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>