Bug 13976

Summary: getPresentationAttribute not implemented
Product: WebKit Reporter: Rob Buis <rwlbuis>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: eric
Priority: P4    
Version: 523.x (Safari 3)   
Hardware: Mac   
OS: OS X 10.4   
Attachments:
Description Flags
Work in progress
none
Patch including layout test
none
Improved patch eric: review+

Description Rob Buis 2007-06-02 01:20:08 PDT
SVGStylable::getPresentationAttribute is not implemented either in code or bindings.
Comment 1 Rob Buis 2007-06-02 01:43:38 PDT
Created attachment 14838 [details]
Work in progress

The patch works fine except the objC bindings fail. Do not apply the patch yet or make sure only js bindings for getPresentationAttribute. Once the bindings are fixed this should be a simple review.
Cheers,

Rob.
Comment 2 Sam Weinig 2007-06-02 12:56:44 PDT
To get the bindings to work you need to touch both CodeGeneratorObjC.pm and CodeGeneratorJS.pm to make sure all of the bindings are rebuilt.  This is needed because we are changing an idl that effects only its subclasses and we don't have any dependancy analysis in the system to notify the correct classes to regenerate in this case.
Comment 3 Rob Buis 2007-06-03 03:31:16 PDT
Created attachment 14846 [details]
Patch including layout test

Given Sam's comments, I think it is valid to make this reviewable.
Cheers,

Rob.
Comment 4 Eric Seidel (no email) 2007-06-04 11:19:35 PDT
Comment on attachment 14846 [details]
Patch including layout test

This check seems unnecessary:
+    if (SVGStyledElement::cssPropertyIdForSVGAttributeName(cssSVGAttr->name()) <= 0)
+        return 0;

It's very possible that SVG elements could be carrying style from non-SVG attributes.  I don't see why we would want to intentionally disable access to these properties via this call.

Using CDATA blocks is probably easier to read than &amp;amp; ;)

The test would also be clearer if it was 3 checks instead of one check for 3 things.

You also don't test mutability as the spec requires.

I think the code looks OK, but the test should be improved before landing.
Comment 5 Rob Buis 2007-06-06 01:48:59 PDT
Created attachment 14877 [details]
Improved patch

This one should be better.
Cheers,

Rob.
Comment 6 Eric Seidel (no email) 2007-06-06 01:55:40 PDT
Comment on attachment 14877 [details]
Improved patch

Looks good.
Comment 7 Sam Weinig 2007-06-08 16:37:30 PDT
Landed in feature branch in r22025.