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 treegraph
 
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

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>