Bug 13976 – getPresentationAttribute not implemented

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Rob Buis]

SVGStylable::getPresentationAttribute is not implemented either in code or bindings.

[Rob Buis]

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.

[Sam Weinig]

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.

[Rob Buis]

Created attachment 14846 [details]
Patch including layout test

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

Rob.

[Eric Seidel (no email)]

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.

[Rob Buis]

Created attachment 14877 [details]
Improved patch

This one should be better.
Cheers,

Rob.

[Eric Seidel (no email)]

Comment on attachment 14877 [details]
Improved patch

Looks good.

[Sam Weinig]

Landed in feature branch in r22025.