Bug 13976 - getPresentationAttribute not implemented
Summary: getPresentationAttribute not implemented
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P4 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-06-02 01:20 PDT by Rob Buis
Modified: 2007-06-08 16:37 PDT (History)
1 user (show)

See Also:


Attachments
Work in progress (5.70 KB, patch)
2007-06-02 01:43 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch including layout test (11.43 KB, patch)
2007-06-03 03:31 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Improved patch (29.93 KB, patch)
2007-06-06 01:48 PDT, Rob Buis
eric: review+
Details | Formatted Diff | Diff

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