Bug 125527 – Clean up SVGScriptElement

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

[Rob Buis]

There seems to be no need for m_type or the type getter/setters to be there.

[Rob Buis]

Created attachment 218888 [details]
Patch

[Sam Weinig]

Comment on attachment 218888 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=218888&action=review

> Source/WebCore/svg/SVGScriptElement.idl:29
> +    [TreatNullAs=NullString, ImplementedAs=typeAttributeValue] attribute DOMString type;

Can this use Reflect?

[Rob Buis]

(In reply to comment #2)
> (From update of attachment 218888 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218888&action=review
> 
> > Source/WebCore/svg/SVGScriptElement.idl:29
> > +    [TreatNullAs=NullString, ImplementedAs=typeAttributeValue] attribute DOMString type;
> 
> Can this use Reflect?

Hi Sam, I was concerned that Reflect would not be able to tell it is the svg type attribute, not the html type attribute, but it is. Will make a new patch.

[Rob Buis]

Created attachment 218904 [details]
Patch

[Sam Weinig]

Comment on attachment 218904 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 218904 [details]
Patch

Clearing flags on attachment: 218904

Committed r160389: <http://trac.webkit.org/changeset/160389>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 218904 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=218904&action=review

> Source/WebCore/svg/SVGScriptElement.idl:29
> +    [TreatNullAs=NullString, Reflect] attribute DOMString type;

This is more than a cleanup. It’s a bug fix.

The old setter code would not set the content attribute when you set the type attribute from JavaScript. The reverse would work, setting the content attribute would affect what you see from JavaScript. We should have a regression test demonstrating this improvement in behavior.

[Rob Buis]

Reopening to attach new patch.

[Rob Buis]

Created attachment 219119 [details]
Patch

[Rob Buis]

(In reply to comment #8)
> (From update of attachment 218904 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218904&action=review
> 
> > Source/WebCore/svg/SVGScriptElement.idl:29
> > +    [TreatNullAs=NullString, Reflect] attribute DOMString type;
> 
> This is more than a cleanup. It’s a bug fix.
> 
> The old setter code would not set the content attribute when you set the type attribute from JavaScript. The reverse would work, setting the content attribute would affect what you see from JavaScript. We should have a regression test demonstrating this improvement in behavior.

Makes sense, I'll work on adding a test.

[Rob Buis]

Comment on attachment 219119 [details]
Patch

Clear review flag since I want to add the test as well.

[Rob Buis]

Created attachment 219131 [details]
Patch

[Rob Buis]

A further datapoint, FireFox syncs the type JS property and the content attribute too.

[Brent Fulgham]

I'd love to get this landed; I think it will clear up four build bot test failures on our Debug machines.

[Rob Buis]

(In reply to comment #15)
> I'd love to get this landed; I think it will clear up four build bot test failures on our Debug machines.

It landed in r160544, sorry about the test failures.

[Brent Fulgham]

(In reply to comment #16)
> (In reply to comment #15)
> > I'd love to get this landed; I think it will clear up four build bot test failures on our Debug machines.
> 
> It landed in r160544, sorry about the test failures.

Excellent!  And it did clear those failures.  Nice and green again. :-)