| Summary: | Clean up SVGScriptElement | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Rob Buis <rwlbuis> | ||||||||||
| Component: | SVG | Assignee: | Rob Buis <rwlbuis> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | bfulgham, cdumez, commit-queue, d-r, esprehn+autocc, fmalita, gyuyoung.kim, kondapallykalyan, pdr, schenney, zimmermann | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Bug Depends on: | |||||||||||||
| Bug Blocks: | 125670 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Rob Buis
2013-12-10 11:49:10 PST
Created attachment 218888 [details]
Patch
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? (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. Created attachment 218904 [details]
Patch
Comment on attachment 218904 [details]
Patch
r=me
Comment on attachment 218904 [details] Patch Clearing flags on attachment: 218904 Committed r160389: <http://trac.webkit.org/changeset/160389> All reviewed patches have been landed. Closing bug. 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. Reopening to attach new patch. Created attachment 219119 [details]
Patch
(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. Comment on attachment 219119 [details]
Patch
Clear review flag since I want to add the test as well.
Created attachment 219131 [details]
Patch
A further datapoint, FireFox syncs the type JS property and the content attribute too. I'd love to get this landed; I think it will clear up four build bot test failures on our Debug machines. (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. (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. :-) |