Closed Bug 1108887 Opened 5 years ago Closed 5 years ago

Back out initial SVG iframe implementation

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: birtles, Assigned: longsonr)

References

Details

Attachments

(2 files, 1 obsolete file)

In bug 949435 we started implemented a native SVG iframe element. After further discussion with the SVG WG and HTML people this no longer seems like a good idea and instead we want to pursue allowing HTML iframe in SVG.

We should backout the parts of bug 949435 that landed since that no longer seems to be the way ahead and adds unnecessary complexity to the code base.
Attached patch parts3and4.txtSplinter Review
Looks like this originally landed in 4 parts per bug 949435 comment 104

Here's a backout for parts 3 and 4.

Parts 1 and 2 seem rather more difficult to backout as the code has changed.
Attachment #8560619 - Flags: review?(bbirtles)
Attached patch part2.txtSplinter Review
Attachment #8560853 - Flags: review?(bzbarsky)
Comment on attachment 8560853 [details] [diff] [review]
part2.txt

>+NS_IMETHODIMP
>+nsGenericHTMLFrameElement::SetIsPrerendered()
>+{
>+  MOZ_ASSERT(!mFrameLoader, "Please call SetIsPrerendered before frameLoader is created");
>+  mIsPrerendered = true;
>+  return NS_OK;
>+  return NS_OK;
>+}

Repeated line here.
Attachment #8560619 - Flags: review?(bbirtles) → review+
Keywords: leave-open
Comment on attachment 8560619 [details] [diff] [review]
parts3and4.txt

Needs a DOM peer review as it contains a webidl change (the complete backout of SVGIFrameElement.webidl)
Attachment #8560619 - Flags: superreview?(bzbarsky)
Comment on attachment 8560853 [details] [diff] [review]
part2.txt

>+nsGenericHTMLFrameElement::SetIsPrerendered()
>+  return NS_OK;
>+  return NS_OK;

Remove one of those, please.

r=me
Attachment #8560853 - Flags: review?(bzbarsky) → review+
Comment on attachment 8560619 [details] [diff] [review]
parts3and4.txt

sr=me
Attachment #8560619 - Flags: superreview?(bzbarsky) → superreview+
Attachment #8560619 - Flags: checkin+
Flags: in-testsuite-
Attachment #8560853 - Flags: checkin+
Attached patch part1.txt (obsolete) — Splinter Review
Assignee: nobody → longsonr
Attachment #8562810 - Flags: review?(bzbarsky)
Comment on attachment 8562810 [details] [diff] [review]
part1.txt

Are we very sure that no SVG element will ever grow anything reflected as DOMSettableTokenList?
Flags: needinfo?(longsonr)
e.g. are we sure we'll never want .dropzone on SVG elements?
SVG 2 doesn't seem to have anything like this at the moment (i.e. nothing about drag/drop is in the SVG specification) and we could always yoyo it back again if necessary.

On the other hand if you'd rather leave things as they are, I'm happy to call this bug fixed at this point. The dead code for SVG iframes is all gone.
Flags: needinfo?(longsonr)
I think I'd rather leave the code in Element.
Attachment #8562810 - Attachment is obsolete: true
Attachment #8562810 - Flags: review?(bzbarsky)
I guess we're done then.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.