949435 - Implementation Proposal for SVG iframe element

Status: RESOLVED WONTFIX

(Core :: SVG, enhancement)

Description

[Ryo HIKOSAKA]

We are planning to implement rendering SVG iframe element, 
which we are currently proposing to SVG Working group.
http://www.w3.org/Graphics/SVG/WG/wiki/Proposals/IFrame_Like_Syntax#5.8_The_.27iframe.27_element

SVG iframe element introduces the function that is almost equivalent to 'iframe' element of html5.0 into svg.
Except for any SVG-specific rules explicitly mentioned in this specification.
It allows dynamic loading and embedding of browsing contexts into svg.
Because 'iframe' can generate the browsing context with the dynamic SVG document, it can constitute cascading documents.Therefore it can express such as the tiling and layering.

We believe the iframe element is useful, and ask considerations for
including this to future releases of Firefox.

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

You are most welcome to attach patches here. The patches should probably also create a pref to disable this feature until it is in the SVG2 spec and has consensus from other vendors (as per https://wiki.mozilla.org/WebAPI/ExposureGuidelines).

Comment 2

[Ryo HIKOSAKA]

Thank you for advice. I will create the pref in the patch.

Comment 3

[Ryo HIKOSAKA]

Attachment: Patch for rendering SVG iframe element.

The patch consists following features:
1. Add model, atom and tags.
1-1. Add model classes which represent SVG iframe element.
1-2. Add idl and webidl which represent SVG iframe element and related elements/ attributes.
1-3. Add atoms which are required for recognizing SVG iframe element and related elements/ attributes.
1-4. Add SVG tags which are required for recognizing SVG iframe element and related attributes.
2. Add functions for rendering SVG iframe.
2-1. Add classes for rendering SVG.
2-2. Add method for showing svg to nsFrameLoader.
3. Add preference for disable thie feature. (preference key: 'svg.svg-iframe.enabled')

Comment 4

[Tomoaki Konno]

Brian and Cameron, could you suggest a suitable reviewer for this patch?

Comment 5

[Robert Longson [:longsonr]]

A few comments in passing.

Is any other UA implementing this? 
Where is this mentioned in the SVG 2 draft specification?
Where's the code for media handling?

And more specifically...

You can't add new .idl files. They are all supposed to be going away. webidl is the future.

Adding back nsSVGUtils::InvalidateBounds is a non-starter too. Remove it and fix your code not to require it.

rename nsGenericSVGFrameElement.h to SVGFrameElement.h (same for .cpp)

Needs tests.

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Tomoaki Konno from comment #4)
> Brian and Cameron, could you suggest a suitable reviewer for this patch?

Hi Konno-san, thank you and Hikosaka-san for your hard work. I think the first step is to address Robert's suggestions in comment 5.

If possible, it would be helpful to split the patch into several smaller patches. For example, one patch for each part in comment 3 would be good. Then we might ask different people to review different patches.

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Robert Longson from comment #5)
> Where is this mentioned in the SVG 2 draft specification?

This feature (along with native <video>, <audio>, <canvas>, <track> etc.) have been approved for SVG2. See discussion here:

http://www.w3.org/2013/02/04-svg-minutes.html#item06

Takagi-san has an action to add this to SVG2 (https://www.w3.org/Graphics/SVG/WG/track/actions/3432) which I think he is in the process of doing.

Comment 8

[Ryo HIKOSAKA]

(In reply to Robert Longson from comment #5)

Thanks for comment, Robert.
I will modify the patch based on your comment.
But, please let me ask a question.

> rename nsGenericSVGFrameElement.h to SVGFrameElement.h (same for .cpp)

We named nsGenericSVGFrameElement accordingly to name of nsGenericHTMLFrameElement class.
Could you tell me why rename is required?

Comment 9

[Ryo HIKOSAKA]

(In reply to Brian Birtles (:birtles) from comment #6)

Thanks for your supports, Brian.
I will split the patch.

Comment 10

[Robert Longson [:longsonr]]

(In reply to Ryo HIKOSAKA from comment #8)
> Could you tell me why rename is required?

We have a new naming convention now. Many files predate that.

Comment 11

[Ryo HIKOSAKA]

(In reply to Robert Longson from comment #10)

> We have a new naming convention now. Many files predate that.

Thanks for information. I will rename the class.

Comment 12

[Ryo HIKOSAKA]

Attachment: Patch for rendering SVG iframe element: patch1

The patch consists following features:
1. Add model, atom and tags.
1-1. Add model classes which represent SVG iframe element.
1-2. Add idl and webidl which represent SVG iframe element and related elements/ attributes.
1-3. Add atoms which are required for recognizing SVG iframe element and related elements/ attributes.
1-4. Add SVG tags which are required for recognizing SVG iframe element and related attributes.

Comment 13

[Ryo HIKOSAKA]

Attachment: Patch for rendering SVG iframe element: patch2

This patch consists following features:
2. Add functions for rendering SVG iframe.
2-1. Add classes for rendering SVG.
2-2. Add method for showing svg to nsFrameLoader.
3. Add preference for disable thie feature. (preference key: 'svg.svg-iframe.enabled')

Comment 14

[Ryo HIKOSAKA]

I split a first patch into following 2 parts.
Patch1 consists part1 of the first patch.
Patch2 consists part2 and part3 of the first patch.
(I didn't split part2 and part3, because these patch contains modification to same file.)

I also modified the patch based on Robert's comment.

Comment 15

[Robert Longson [:longsonr]]

Comment on attachment 8350521 [details] [diff] [review]
Patch for rendering SVG iframe element: patch1

>diff -r 50f784620694 .hgtags
>--- a/.hgtags	Thu Dec 19 23:30:38 2013 -0500
>+++ b/.hgtags	Fri Dec 20 16:33:18 2013 +0900
>@@ -97,3 +97,5 @@
> ad0ae007aa9e03cd74e9005cd6652e544139b3b5 FIREFOX_AURORA_25_BASE
> 2520866d58740851d862c7c59246a4e3f8b4a176 FIREFOX_AURORA_26_BASE
> 05025f4889a0bf4dc99ce0c244c750adc002f015 FIREFOX_AURORA_27_BASE
>+d110c456d363c2e134530c01b01ab1e9577125c2 a1
>+a06e530f36e7deec05a082ce0cd1214b0ac5e90a a2

I don't think this is right. Please remove it unless you can explain what it is and why it's required.

>diff -r 50f784620694 content/base/src/moz.build
>--- a/content/base/src/moz.build	Thu Dec 19 23:30:38 2013 -0500
>+++ b/content/base/src/moz.build	Fri Dec 20 16:33:18 2013 +0900
>@@ -197,6 +197,7 @@
>     '/content/events/src',
>     '/content/html/content/src',
>     '/content/html/document/src',
>+    '/content/svg/content/src',
>     '/content/xbl/src',
>     '/content/xml/content/src',
>     '/content/xml/document/src',

Why is this required? It doesn't seem right. svg should depend on base, not the other way round.

>diff -r 50f784620694 content/events/src/nsEventStateManager.cpp
>--- a/content/events/src/nsEventStateManager.cpp	Thu Dec 19 23:30:38 2013 -0500
>+++ b/content/events/src/nsEventStateManager.cpp	Fri Dec 20 16:33:18 2013 +0900
>@@ -1630,6 +1630,11 @@
>     return !!TabParent::GetFrom(target);
>   }
> 
>+  // <iframe> from SVG
>+  if (browserFrame && (target->Tag() == nsGkAtoms::iframe) && target->IsSVG()) {

Write this as

 if (browserFrame && target->IsSVG(nsGkAtoms::iframe)) {

>diff -r 50f784620694 content/html/content/src/HTMLIFrameElement.cpp
>--- a/content/html/content/src/HTMLIFrameElement.cpp	Thu Dec 19 23:30:38 2013 -0500
>+++ b/content/html/content/src/HTMLIFrameElement.cpp	Fri Dec 20 16:33:18 2013 +0900
>@@ -5,6 +5,7 @@
> 
> #include "mozilla/dom/HTMLIFrameElement.h"
> #include "mozilla/dom/HTMLIFrameElementBinding.h"
>+#include "mozilla/dom/SVGDocumentBinding.h"

Why is this required?

>diff -r 50f784620694 content/html/content/src/HTMLObjectElement.cpp
>--- a/content/html/content/src/HTMLObjectElement.cpp	Thu Dec 19 23:30:38 2013 -0500
>+++ b/content/html/content/src/HTMLObjectElement.cpp	Fri Dec 20 16:33:18 2013 +0900
>@@ -7,6 +7,7 @@
> #include "mozilla/dom/HTMLObjectElement.h"
> #include "mozilla/dom/HTMLObjectElementBinding.h"
> #include "mozilla/dom/ElementInlines.h"
>+#include "mozilla/dom/SVGDocumentBinding.h"

Why is this required?

>diff -r 50f784620694 content/html/content/src/HTMLSharedObjectElement.cpp
>--- a/content/html/content/src/HTMLSharedObjectElement.cpp	Thu Dec 19 23:30:38 2013 -0500
>+++ b/content/html/content/src/HTMLSharedObjectElement.cpp	Fri Dec 20 16:33:18 2013 +0900
>@@ -8,7 +8,7 @@
> #include "mozilla/dom/HTMLEmbedElementBinding.h"
> #include "mozilla/dom/HTMLAppletElementBinding.h"
> #include "mozilla/dom/ElementInlines.h"
>-
>+#include "mozilla/dom/SVGDocumentBinding.h"

Why is this required?

>diff -r 50f784620694 content/svg/content/src/SVGContentUtils.cpp
>--- a/content/svg/content/src/SVGContentUtils.cpp	Thu Dec 19 23:30:38 2013 -0500
>+++ b/content/svg/content/src/SVGContentUtils.cpp	Fri Dec 20 16:33:18 2013 +0900
>@@ -30,7 +30,8 @@
>   nsIContent *ancestor = aSVGElement->GetFlattenedTreeParent();
> 
>   while (ancestor && ancestor->IsSVG() &&
>-                     ancestor->Tag() != nsGkAtoms::foreignObject) {
>+                    (ancestor->Tag() != nsGkAtoms::foreignObject || 
>+                     ancestor->Tag() != nsGkAtoms::iframe)) {
>     element = ancestor;
>     ancestor = element->GetFlattenedTreeParent();
>   }
>@@ -157,7 +158,8 @@
>   return aContent && aContent->IsSVG() &&
>            (aContent->Tag() == nsGkAtoms::svg ||
>             aContent->Tag() == nsGkAtoms::foreignObject ||
>-            aContent->Tag() == nsGkAtoms::symbol);
>+            aContent->Tag() == nsGkAtoms::symbol ||
>+            aContent->Tag() == nsGkAtoms::iframe );

Remove space before )

>diff -r 50f784620694 content/svg/content/src/SVGIFrameElement.cpp
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/content/svg/content/src/SVGIFrameElement.cpp	Fri Dec 20 16:33:18 2013 +0900
>@@ -0,0 +1,420 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#include "mozilla/ArrayUtils.h"
>+
>+#include "mozilla/dom/SVGIFrameElement.h"

This should be the first include line.

>+#include "nsCOMPtr.h"
>+#include "nsComputedDOMStyle.h"
>+#include "nsGkAtoms.h"
>+#include "mozilla/dom/SVGDocumentBinding.h"
>+#include "mozilla/dom/SVGIFrameElementBinding.h"
>+#include "mozilla/dom/SVGMatrix.h"
>+#include "mozilla/dom/SVGSVGElement.h"
>+#include "mozilla/Preferences.h"
>+#include "nsStyleConsts.h"
>+#include "nsIFrame.h"

Are these required. Why should SVGIFrame require nsIFrame or nsComputedDOMStyle?

>+
>+//DOMCI_NODE_DATA(SVGIFrameElement, mozilla::dom::SVGIFrameElement)

Please don't write commented out code.

>+
>+NS_IMPL_NS_NEW_NAMESPACED_SVG_ELEMENT_CHECK_PARSER(IFrame)

IFrame? Shouldn't that be SVGIFrame?

>+SVGIFrameElement::SVGIFrameElement(already_AddRefed<nsINodeInfo> aNodeInfo,
>+                                       FromParser aFromParser)

Align arguments so that FromParser starts just after (

>+  : SVGIFrameElementBase(aNodeInfo, aFromParser)
>+	, mRatioReplaced(false)
>+{
>+	mOrgSrc.Truncate();

Too many spaces of indent.

>+}
>+
>+SVGIFrameElement::~SVGIFrameElement()
>+{
>+}

Not required.

>+//NS_IMPL_ELEMENT_CLONE_WITH_INIT(SVGIFrameElement)

Please don't write commented out code.

>+//----------------------------------------------------------------------
>+// nsIDOMSVGIFrameElement methods:
>+
>+already_AddRefed<nsIDocument>
>+SVGIFrameElement::GetContentDocument()
>+{
>+  // return SVGIFrameElementBase::GetContentDocument();

Ditto.

>+  nsCOMPtr<nsIDocument> doc = SVGIFrameElementBase::GetContentDocument();
>+  return doc.forget();
>+}
>+
>+already_AddRefed<SVGAnimatedString>
>+SVGIFrameElement::Media()
>+{
>+  //nsCOMPtr<SVGAnimatedString> media;
>+  //mStringAttributes[MEDIA].ToDOMAnimatedString(getter_AddRefs(media), this);
>+  //return media.forget();

etc. In fact let's just assume I'm going to say this everytime it occurs and I'll stop at this
point but you need to carry on removing the commented out code.

>+
>+void
>+SVGIFrameElement::SetSandbox(const nsAString& aSandbox, ErrorResult& rv)
>+{
>+  rv = SetAttr(kNameSpaceID_None, nsGkAtoms::sandbox, aSandbox, true);
>+}

This is really in the specification? It's unusual to do this for SVG elements, after all
if someone wants to do this they can just call setAttribute can't they?

>+
>+bool
>+SVGIFrameElement::ParseAttribute(int32_t aNamespaceID,
>+                                    nsIAtom* aAttribute,
>+                                    const nsAString& aValue,
>+                                    nsAttrValue& aResult)

Indenting of arguments is incorrect.

>+{
>+  return nsSVGElement::ParseAttribute(aNamespaceID, 
>+		                                  aAttribute, aValue, aResult);

Here too. Why are we wrapping ParseAttributer here anyway this seems pointless and should
therefore be removed.

>+bool SVGIFrameElement::postPoneValue()

Methods must begin with a capital letter.

>+{
>+  bool ret = mBooleanAttributes[POSTPONE].GetAnimValue();
>+  if (!ret) {
>+    nsRefPtr<nsStyleContext> styleContext;
>+    styleContext = 
>+      nsComputedDOMStyle::GetStyleContextForElementNoFlush(this, nullptr, nullptr);
>+    if (styleContext) {
>+      ret = styleContext->StyleVisibility()->IsPostPone();
>+    }

This is weird. If we don't have an attribute get a style. If this is in the specification, the
specification should change.

>+nsresult
>+SVGIFrameElement::BindToTree(nsIDocument* aDocument,
>+                              nsIContent* aParent,
>+                              nsIContent* aBindingParent,
>+                                     bool aCompileEventHandlers)
>+{
>+  nsresult rv = nsSVGElement::BindToTree(aDocument, aParent,
>+                                                 aBindingParent,
>+                                                 aCompileEventHandlers);

Bad indenting again, I'm going to stop commenting on this now but you should check it everywhere and fix it.

>+    // We're in a document now.  Kick off the frame load.
>+    LoadSrc( true );

No spaces after ( or before )

>+
>+    if (this->HasAttribute(NS_LITERAL_STRING("sandbox"))) {

This is almost certainly wrong. What happens if sandbox only exists because it's animated?

>+      //EnsureFrameLoader();

commented out code again.

>+    LoadSrc( true );

incorrect whitespace again.

>+SVGIFrameElement::checkPreserveAspectRatioOfChild( nsIContent* node )

ditto.

>+{
>+  if (!mRatioReplaced) {
>+    if (node && node->IsElement()) {
>+      SVGSVGElement* childRoot = static_cast<SVGSVGElement*>(node);
>+
>+      SVGPreserveAspectRatio ratio = mPreserveAspectRatio.GetBaseValue();
>+      bool flag = false;
>+      if (childRoot->HasAttribute(NS_LITERAL_STRING("preserveAspectRatio"))) {

Pretty much any use of HasAttribute by SVG code is wrong as an attribute may be set by animating it
HasAttribute would be false but the attribute would still exist.

>+          if (!ratio.GetDefer() && !mRatioReplaced) {
>+            flag = true;

A variable called flag doesn't tell us much about what it's for.

>+          } else if (ratio.GetDefer()) {
>+            mRatioReplaced = true;
>+          }
>+      } else {
>+          flag = true;
>+      }
>+      if (flag) {
>+          mRatioReplaced = true;
>+          nsAutoString astr;
>+          mPreserveAspectRatio.GetBaseValueString(astr);
>+          childRoot->SetAttr(kNameSpaceID_None, nsGkAtoms::preserveAspectRatio, nullptr, astr, false);
>+      }
>+    }
>+  }

Looking at this funcation. How can mRatioReplaced ever become false again? Is that not a possible scenario? Why not?

>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#ifndef mozilla_dom_SVGIFrameElement_h
>+#define mozilla_dom_SVGIFrameElement_h
>+
>+#include "nsSVGElement.h"

Is this include required?

>+  SVGIFrameElement(already_AddRefed<nsINodeInfo> aNodeInfo,
>+	                 mozilla::dom::FromParser aFromParser);

indenting.

>+  nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
>+                   const nsAString& aValue, bool aNotify)
>+  {
>+    return SetAttr(aNameSpaceID, aName, nullptr, aValue, aNotify);
>+  }

Is this required? If so why?

>+  uint32_t GetSandboxFlags();
>+  void checkPreserveAspectRatioOfChild( nsIContent* childRoot );

Add comments about what these do. And checkPreserveAspectRatioOfChild must begin with a capital letter.

>diff -r 50f784620694 content/svg/content/src/SVGSVGElement.h
>--- a/content/svg/content/src/SVGSVGElement.h	Thu Dec 19 23:30:38 2013 -0500
>+++ b/content/svg/content/src/SVGSVGElement.h	Fri Dec 20 16:33:18 2013 +0900

>-    return parent && parent->IsSVG() &&
>-           parent->Tag() != nsGkAtoms::foreignObject;
>+    return parent && parent->IsSVG() &&
>+           (parent->Tag() != nsGkAtoms::foreignObject || 
>+		   parent->Tag() != nsGkAtoms::iframe);

indenting.

>diff -r 50f784620694 content/svg/content/src/moz.build
>--- a/content/svg/content/src/moz.build	Thu Dec 19 23:30:38 2013 -0500
>+++ b/content/svg/content/src/moz.build	Fri Dec 20 16:33:18 2013 +0900
>@@ -9,6 +9,7 @@
>     'nsSVGElement.h',
>     'nsSVGFeatures.h',
>     'SVGAttrValueWrapper.h',
>+    'SVGFrameElement.h',

There's no SVGFrameElement.h in this patch. Is this a typo? Does this patch build?

>     'SVGFilterElement.cpp',
>     'SVGForeignObjectElement.cpp',
>     'SVGFragmentIdentifier.cpp',
>+    'SVGFrameElement.cpp',

What's this? The source is not present in this patch? Why is there a separate SVGFrameElement and SVGIFrameElement?

>+
>+  // not implement yet

not implemented yet

>diff -r 50f784620694 dom/webidl/moz.build
>--- a/dom/webidl/moz.build	Thu Dec 19 23:30:38 2013 -0500
>+++ b/dom/webidl/moz.build	Fri Dec 20 16:33:18 2013 +0900
>@@ -344,6 +344,7 @@
>     'SVGGElement.webidl',
>     'SVGGradientElement.webidl',
>     'SVGGraphicsElement.webidl',
>+	'SVGIFrameElement.webidl',

Spaces rather than tabs.

Comment 16

[Robert Longson [:longsonr]]

Please apply some of the more general comments such as indenting, whitespace, no commented code etc to the other patch and resubmit that too.

Comment 17

[Ryo HIKOSAKA]

Thanks for comment, and sorry to take up your time for pointing basic mistake such as indenting, whitespace, etc.
I will modify the patch1 and patch2 based on your comment.

Comment 18

[Robert Longson [:longsonr]]

Dont worry about it, you're new at this, we've all been where you are now.

Comment 19

[Robert Longson [:longsonr]]

One other thing about the postPoneValue function while I remember. Postpone is a single word in English so I would not expect the middle p to be capitalised, just the initial one.

Comment 20

[Tomoaki Konno]

Hikosaka-san, could you separate the patch for "postpone" from the patch for "iframe"?
The spec of "postpone" is basically independent of "iframe" spec.
https://dvcs.w3.org/hg/webperf/raw-file/tip/specs/ResourcePriorities/Overview.html#attr-postpone

Comment 21

[Ryo HIKOSAKA]

Attachment: Patch for rendering SVG iframe element: patch1

address comment #15 and #17

> >diff -r 50f784620694 content/base/src/moz.build
> >--- a/content/base/src/moz.build	Thu Dec 19 23:30:38 2013 -0500
> >+++ b/content/base/src/moz.build	Fri Dec 20 16:33:18 2013 +0900
> >@@ -197,6 +197,7 @@
> >     '/content/events/src',
> >     '/content/html/content/src',
> >     '/content/html/document/src',
> >+    '/content/svg/content/src',
> >     '/content/xbl/src',
> >     '/content/xml/content/src',
> >     '/content/xml/document/src',
> Why is this required? It doesn't seem right. svg should depend on base, not the other way round.

I didn't delete the import.
Because without the import compile error is occured.
(mozilla\dom\nsSVGAnimatedTransformList.h(13) : fatal error C1083: Cannot open include file: 'SVGTransformList.h')

> >+
> >+NS_IMPL_NS_NEW_NAMESPACED_SVG_ELEMENT_CHECK_PARSER(IFrame)
> IFrame? Shouldn't that be SVGIFrame?

I think 'IFrame' is right.
Because NS_IMPL_NS_NEW_NAMESPACED_SVG_ELEMENT_CHECK_PARSER adds prefix 'SVG' to argument.
http://dxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGElement.h#667


> >+          } else if (ratio.GetDefer()) {
> >+            mRatioReplaced = true;
> >+          }
> >+      } else {
> >+          flag = true;
> >+      }
> >+      if (flag) {
> >+          mRatioReplaced = true;
> >+          nsAutoString astr;
> >+          mPreserveAspectRatio.GetBaseValueString(astr);
> >+          childRoot->SetAttr(kNameSpaceID_None, nsGkAtoms::preserveAspectRatio, nullptr, astr, false);
> >+      }
> >+    }
> >+  }
> 
> Looking at this funcation. How can mRatioReplaced ever become false again? Is that not a possible scenario? Why not?

The purpose of CheckPreserveAspectRatioOfChild method is to apply preserveAspectRatio attribute to nested element of the iframe.
When src attribute is changed, mRatioReplaced shold become false again.
I deleted this method, and will rewrite in a separate patch. Because this method has other problems (for example, this method doesn't work if nested element is html).

> Hikosaka-san, could you separate the patch for "postpone" from the patch for "iframe"?
> The spec of "postpone" is basically independent of "iframe" spec.
> https://dvcs.w3.org/hg/webperf/raw-file/tip/specs/ResourcePriorities/Overview.html#attr-postpone

Thanks for comment. I separated the patch for "postpone".

Comment 22

[Ryo HIKOSAKA]

Attachment: Patch for rendering SVG iframe element: patch2

address comment #16 and #17

Comment 23

[Ryo HIKOSAKA]

(In reply to Ryo HIKOSAKA from comment #21)
> Created attachment 8351422 [details] [diff] [review]
> Patch for rendering SVG iframe element: patch1
> 
> address comment #15 and #17
> 

Sorry, I wrote wrong comment number.
"address comment #15 and #20" is right.

Comment 24

[Ryo HIKOSAKA]

Attachment: testcase

testcase

Comment 25

[Robert Longson [:longsonr]]

Comment on attachment 8351423 [details] [diff] [review]
Patch for rendering SVG iframe element: patch2

>+  , mOwnerIsSVGIFrame(false)

Why is mOwnerIsSVGIFrame required? Won't the content just be an SVGIFrameElement which we can test for?

>+  if (!mOwnerIsSVGIFrame) {
>+    HTMLIFrameElement* iframe = HTMLIFrameElement::FromContent(mOwnerContent);
>+    if (iframe) {
>+      sandboxFlags = iframe->GetSandboxFlags();
>+    }
>+  } else {
>+    SVGIFrameElement* iframe = static_cast<SVGIFrameElement*>(mOwnerContent);

You should test for an SVGIFrameElement by using mOwnerContent->IsSVG(nsGkAtoms::SVGIFrameElement)
Don't need a mOwnerIsSVGIFrame then.

>   /**
>+   * Called from the layout frame associated with this frame loader;
>+   * this notifies us to hook up with the widget and view.
>+   */
>+  bool ShowSVG(int32_t marginWidth, int32_t marginHeight, nsSVGIFrameFrame* frame);
>+
>+  /**

Why do we need this? Can't we use the existing Show function?

> 
>+  void SetOwnerIsSVGIFrame(bool flag){ mOwnerIsSVGIFrame = flag; }
>+

I think this can and should be removed.

>+
>+  bool mOwnerIsSVGIFrame;

ditto.

> FRAME_ID(nsSVGGFrame)
> FRAME_ID(nsSVGGradientFrame)
>+FRAME_ID(nsSVGIFrameForeignObjectFrame)

What's this? Why is it required?

>+
>+static nsRect
>+ToCanvasBounds(const gfxRect &aUserspaceRect,
>+               const gfxMatrix &aToCanvas,
>+               const nsPresContext *presContext);

There's on of these in nsSVGUtils or maybe nsSVGForeignObject frame. Can't we have a single instance? If it's in nsSVGForeignObjectFrame copy it to nsSVGUtils and make both things use a single function.

>+  // OuterFrame but we wait for the normal view creation path in
>+  // nsCSSFrameConstructor, then we will lose because the inner view's
>+  // parent will already have been set to some outer view (e.g., the
>+  // canvas) when it really needs to have this frame's view as its
>+  // parent. So, create this frame's view right away, whether we
>+  // really need it or not, and the inner view will get it as the
>+  // parent.
>+  if (!HasView()) {
>+    nsContainerFrame::CreateViewForFrame(this, true);
>+  }
>+  EnsureInnerView();
>+
>+  // Set the primary frame now so that nsDocumentViewer::FindContainerView
>+  // called from within EndSwapDocShellsForViews below can find it if needed.
>+  aContent->SetPrimaryFrame(this);
>+
>+  // If we have a detached subdoc's root view on our frame loader, re-insert
>+  // it into the view tree. This happens when we've been reframed, and
>+  // ensures the presentation persists across reframes. If the frame element
>+  // has changed documents however, we blow away the presentation.
>+  nsRefPtr<nsFrameLoader> frameloader = FrameLoader();
>+  if (frameloader) {
>+    nsCOMPtr<nsIDocument> oldContainerDoc;
>+    nsView* detachedViews =
>+      frameloader->GetDetachedSubdocView(getter_AddRefs(oldContainerDoc));
>+    if (detachedViews) {
>+      if (oldContainerDoc == aContent->OwnerDoc()) {
>+        // Restore stashed presentation.
>+        ::InsertViewsInReverseOrder(detachedViews, mInnerView);
>+        ::EndSwapDocShellsForViews(mInnerView->GetFirstChild());
>+      } else {
>+        // Presentation is for a different document, don't restore it.
>+        frameloader->Hide();
>+      }
>+    }
>+    frameloader->SetDetachedSubdocView(nullptr, nullptr);
>+  }
>+
>+  AddStateBits(NS_FRAME_FONT_INFLATION_CONTAINER |
>+               NS_FRAME_FONT_INFLATION_FLOW_ROOT);
>+  nsContentUtils::AddScriptRunner(new AsyncSVGFrameInit(this));
>+
>+  return;

Is this all copied from somewhere? If so perhaps we should abstract it into some common method.

>+nsIntSize
>+nsSVGIFrameFrame::GetSubdocumentSize()
>+{
>+  float x, y, w, h;
>+  if (mContent) {
>+    static_cast<SVGIFrameElement*>(mContent)->
>+    GetAnimatedLengthValues(&x, &y, &w, &h, nullptr);
>+    if (w < 0.0f) w = 0.0f;
>+    if (h < 0.0f) h = 0.0f;
>+    // GetCanvasTM includes the x,y translation
>+    return nsIntSize(w, h);
>+  } else {
>+    // contents not loaded ?!

I don't see how that can happen. Nix the if (mContent) test.

>+static void
>+InsertViewsInReverseOrder(nsView* aSibling, nsView* aParent)
>+{

Is this copied from somewhere? We should have this in a common location if so e.g. nsLayoutUtils

>+
>+static bool
>+EndSwapDocShellsForDocument(nsIDocument* aDocument, void*)
>+{

ditto.

>+
>+static void
>+EndSwapDocShellsForViews(nsView* aSibling)
>+{

ditto.

Alternatively perhaps HTML IFrame's and SVG IFrames could share a common base class so we don't have everything copied into two locations.

>+NS_IMETHODIMP
>+nsSVGIFrameFrame::AttributeChanged(int32_t  aNameSpaceID,
>+                                   nsIAtom *aAttribute,
>+                                   int32_t  aModType)
>+{
>+      NS_WARNING("Attribute changed");

a) wrong indenting b) shouldn't be a warning anyway, this is a valid code path.

>+    } else if (aAttribute == nsGkAtoms::viewBox ||
>+               aAttribute == nsGkAtoms::preserveAspectRatio) {
>+      nsSVGEffects::InvalidateRenderingObservers(this);
>+      if (aAttribute == nsGkAtoms::preserveAspectRatio) {
>+        nsSVGUtils::ScheduleReflowSVG(this);
>+      }
>+    } else if (aAttribute == nsGkAtoms::src) {
>+      NS_WARNING("src changed");

Erm, no this shouldn't be a warning. Should this do something? Reflow perhaps?

Quite a lot of the rest of this code seems to be common to ForeignObject. Can some of the duplication be abstracted into somewhere common if so (nsLayoutUtils if common with HTML Iframes or nsSVGUtils if common to foreignObject or a common base class).

>+{
>+  NS_PRECONDITION(aReflowState.frame == aKidFrame, "bad reflow state");
>+
>+  nsresult  result;

Define the variable where it's used. But note that the standard name for an nsresult variable is rv

...
>+
>+  // Reflow the child frame
>+  result = aKidFrame->Reflow(aPresContext, aDesiredSize, aReflowState,
>+                             aStatus);

i.e. here.

Comment 26

[Robert Longson [:longsonr]]

I can complete reviews of patch 1 and part of patch 2 but the content/base code is outside my purview so will need to be split into another patch for someone else to review really.

In future please select a reviewer when you submit your patches so we know to review them ;-)

Generally looks OK in structure though, apart from all the duplication.

Comment 27

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Robert Longson from comment #26)
> Generally looks OK in structure though, apart from all the duplication.

I met up with Hikosaka-san and the end of last year to discuss these patches. Hikosaka-san agreed to split out the patches further (in particular, the postpone attribute we will make into a separate bug).

There is a lot of duplication from nsGenericHTMLFrameElement.cpp and nsContainerFrame.cpp. We talked briefly about ways to address this and if SVGElement ends up inheriting from HTMLElement (as was brought up in Cameron's proposal: http://dev.w3.org/SVG/proposals/improving-svg-dom/) then this might be easier to achieve.

I think Hikosaka-san has a number of changes he will make before asking for a full review.

Comment 28

[Robert Longson [:longsonr]]

There are a few common ways of reducing duplication.

a) Make SVGIFrame derive from HTMLIFrame
b) Make SVGIFrame a container for a HTMLIFrame (like marker and outer svg frames check out nsSVGOuterSVGAnonChildFrame and  nsSVGMarkerAnonChildFrame)
c) Make a common base class for both

Not sure which is best. c) may be easiest though given the different underlying elements in each case.

Comment 29

[Ryo HIKOSAKA]

Attachment: Part1:Webidl of SVGIFrameElement

Comment 30

[Ryo HIKOSAKA]

Attachment: Part2:Add pref for disable SVGIFrameElement

Comment 31

[Ryo HIKOSAKA]

Attachment: Part3:Move nsGenericHTMLElement::GetTokenList implementation to nsContentUtils

Comment 32

[Ryo HIKOSAKA]

Attachment: Part4:Create base class of nsGenericHTMLFrameElement and SVGIFrameElement

Comment 33

[Ryo HIKOSAKA]

Attachment: Part5:Create SVGIFrameELement class

Comment 34

[Ryo HIKOSAKA]

I split the patch and address review comment.
It is not full implementation.
I will modify and split the remaining.

Comment 35

[Robert Longson [:longsonr]]

Comment on attachment 8359640 [details] [diff] [review]
Part4:Create base class of nsGenericHTMLFrameElement and SVGIFrameElement

>+  virtual mozilla::dom::Element* GetThisFrameElement() MOZ_OVERRIDE
>+  {
>+    return this;
>+  }

What's the point of this function? Why not call the methods directly.

Comment 36

[Ryo HIKOSAKA]

(In reply to Robert Longson from comment #35)
> Comment on attachment 8359640 [details] [diff] [review]
> Part4:Create base class of nsGenericHTMLFrameElement and SVGIFrameElement
> 
> >+  virtual mozilla::dom::Element* GetThisFrameElement() MOZ_OVERRIDE
> >+  {
> >+    return this;
> >+  }
> 
> What's the point of this function? Why not call the methods directly.

I made nsGenericFrameElement not inherit Element class to avoid diamond inherit problem.
GetThisFrameElement method is used when a pointer of Element is required in nsGenericFrameElement class.
The method is defined as pure virtual function in nsGenericFrameElement, and derived class implements the method by returning this pointer.
(Maybe there is more simple approach. I would appreciate any advice.)

Comment 37

[Robert Longson [:longsonr]]

Can you just sketch out the inheritence tree you have to save me going though all the classes please?

Comment 38

[Ryo HIKOSAKA]

Attachment: class_tree_svg_iframe.png

Comment 39

[Ryo HIKOSAKA]

(In reply to Robert Longson from comment #37)
> Can you just sketch out the inheritence tree you have to save me going
> though all the classes please?

I attached inheritance tree diagrams. (before and after the patch)

Comment 40

[Ryo HIKOSAKA]

Attachment: Part6: Add SVG_FROM_PARSER_TAG of SVGIframe

Comment 41

[Ryo HIKOSAKA]

Attachment: Add codes for handling nsGkAtoms::iframe (content/svg/content/src/*)

Comment 42

[Ryo HIKOSAKA]

Attachment: Part7: Add codes for handling nsGkAtoms::iframe (content/svg/content/src/*)

Comment 43

[Ryo HIKOSAKA]

Attachment: Part8: Add codes for handling nsGkAtoms::iframe (content/events/src/*)

Comment 44

[Ryo HIKOSAKA]

Attachment: Part9: Add codes for handling nsGkAtoms::iframe (content/base/src/*)

Comment 45

[Ryo HIKOSAKA]

Attachment: Part10: add nsSVGIFrameFrame to nsFrameIdList

Comment 46

[Ryo HIKOSAKA]

Attachment: Part11: Add svgIFrameFrame to nsGkAtomList

Comment 47

[Ryo HIKOSAKA]

Attachment: Part12: Define helper methods on nsContainerFrame

Comment 48

[Ryo HIKOSAKA]

Attachment: Part13: Create nsSVGIFrameFrame and common base class of nsSVGForeignObjectFrame and nsSVGIFrameFrame.

Comment 49

[Ryo HIKOSAKA]

Attachment: Part14: Add frame constructor for SVGIFrameFrame

Comment 50

[Ryo HIKOSAKA]

Attachment: Part15: Add code for handle svgIFrameFrame to nsDocumentViewer

Comment 51

[Ryo HIKOSAKA]

I split the remaining.
In the patch 12, I defined static helper methods on nsContainerFrame, which are shared with nsContainerFrame and nsSVGIFrameFrame class.
Because nsContainerFrame has some static helper methods, I followed suit.
But it's better to define on nsLayoutUtil, perhaps.

In patch 13, I created nsSVGEmbeddedFrame class as a common base class of nsSVGForeignObjectFrame and nsSVGIFrameFrame.
I will attach inheritance tree diagrams of the nsSVGEmbeddedFramens, SVGIFrameFrame and related classes.

Comment 52

[Ryo HIKOSAKA]

Attachment: class_tree_nsSVGIFrameFrame.png

Comment 53

[Ryo HIKOSAKA]

I think the paches are ready to a review.
Brian, if possible, could you help to select a suitable reviewer?
If I have something to do before review, please let me know.

Comment 54

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Ryo HIKOSAKA from comment #53)
> I think the paches are ready to a review.
> Brian, if possible, could you help to select a suitable reviewer?
> If I have something to do before review, please let me know.

Hi Hikosaka-san. Thank you very much for your work on these patches and the class diagrams.

I think you can ask Robert Longson and Jonathan Watt to review the patches. Perhaps they can review half of the patches each.

After that, some of the patches will probably require an additional review called superreview. For that Boris Zbarsky would be a good reviewer, or otherwise Robert O'Callahan. Robert Longson and Jonathan Watt can suggest which patches require superreview.

Note to reviewers, the SVG2 spec text is still awaiting review and may change. For example, I'm not sure yet if we really need frameWidth/frameHeight.

Comment 55

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Ryo HIKOSAKA from comment #53)
> If I have something to do before review, please let me know.

For each patch, you need to follow the "Details" link, set review to "?", then type in the reviewer's name.

Comment 56

[Ryo HIKOSAKA]

Thanks for your support, Brian.
I set flags.

Comment 57

[Robert Longson [:longsonr]]

Comment on attachment 8359636 [details] [diff] [review]
Part1:Webidl of SVGIFrameElement

I'm not a suitable reviewer for this. Trying Boris

Comment 58

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8359636 [details] [diff] [review]
Part1:Webidl of SVGIFrameElement

>+++ b/dom/bindings/Bindings.conf	Tue Jan 14 14:26:25 2014 +0900

You don't need the change to this file, do you?  That's the default headerFile value you're setting.

>+++ b/dom/webidl/SVGIFrameElement.webidl	Tue Jan 14 14:26:25 2014 +0900

The right spec link here is probably to http://www.w3.org/Graphics/SVG/WG/wiki/Proposals/IFrame_Like_Syntax#5.12.14_Interface_SVGIFrameElement not to where you're linking?

>+  [Constant]
>+  readonly attribute SVGAnimatedPreserveAspectRatio preserveAspectRatio;

It's not clear to me this attribute makes any sense for iframe, but ok.

>+  readonly attribute SVGAnimatedString media;

This doesn't seem to be in the IDL in the wiki.  Issue raised?

More generally, why do we want all these things that are supposed to match the API <html:iframe> provides to be SVGAnimated*?  What are the use cases for animating src or srcdoc, for example, as compared to actually just setting them like people will expect to do?  Or name, of that matter?

I can see the desire to have SVGAnimatedLength for x and y, and maybe width/height.  And no opinion on the type for preserveAspectRatio.  The three I listed above seem like they should not be animated.  No idea about "media".

This interface should be conditional on the pref being added in part 2, I would think.  It should also not be added until mozilla/dom/SVGIFrameElement.h actually exists, unless you plan to fold all the changesets together before pushing...

Most importantly here, would like to get the DOMString vs SVGAnimatedString bits sorted out.

Comment 59

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8359639 [details] [diff] [review]
Part3:Move nsGenericHTMLElement::GetTokenList implementation to nsContentUtils

Why nsContentUtils and not Element?  The latter makes a lot more sense to me if we just want to share it across SVG and HTML elements.

Comment 60

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8359640 [details] [diff] [review]
Part4:Create base class of nsGenericHTMLFrameElement and SVGIFrameElement

This base class shouldn't have its own refcounting.

It also doesn't need a QI impl (which is never being called anyway).  In fact, it shouldn't NS_DECL_ISUPPORTS at all.

I'm not entirely convinced on the naming: it sounds like an element class, when it actually isn't.  Maybe nsElementFrameLoaderOwner instead?

>+  virtual mozilla::dom::Element* GetThisFrameElement() = 0;

This can never return null, so drop the "Get"?

>+  // This doesn't really ensure a frame loade in all cases, only when

Please fix the preexisting typo; I assume it meant "ensure a frame loader".

>+  nsGenericFrameElement(already_AddRefed<nsINodeInfo> aNodeInfo,

aNodeInfo is unused.  Just don't ask the caller to pass it.

>+    '/content/svg/content/src',

I'm not sure why this part needed a LOCAL_INCLUDES change... why did it?

Comment 61

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8359642 [details] [diff] [review]
Part5:Create SVGIFrameELement class

Just as a note, you shouldn't need your own SVGIFrameElement::GetContentDocument and whatnot method for things you already inherit from nsGenericFrameElement.  "using" is your friend here.

Also, this doesn't seem to handle removal of @srcdoc correctly.  Please add a test for that!

Comment 62

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8364889 [details] [diff] [review]
Part8: Add codes for handling nsGkAtoms::iframe (content/events/src/*)

I don't understand this change.  Are you trying to support <svg:iframe mozbrowser>?  Or something else?

But your element doesn't QI to nsIMozBrowserFrame, so this is dead code anyway.  And if it did, presumably this code would work already, since it would implement GetReallyIsBrowserOrApp() correctly.

Comment 63

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8364887 [details] [diff] [review]
Part7: Add codes for handling nsGkAtoms::iframe (content/svg/content/src/*)

Why stop at ::iframe in GetOuterSVGElement?  It doesn't stop for svg:image, which is what iframe should behave like wrt its descendants.  It should _not_ act like foreignObject, which does something quite different from the replaced element cases.

Similar in GetNearestViewportElement, etc.  In fact, most of this patch...

Comment 64

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8364890 [details] [diff] [review]
Part9: Add codes for handling nsGkAtoms::iframe (content/base/src/*)

Why do you need to include SVGDocumentBinding.h in nsFrameLoader.cpp?

>+    SVGIFrameElement* iframe = static_cast<SVGIFrameElement*>(mOwnerContent);
>+    if (iframe) {

That null-check makes no sense.

>+++ b/content/base/src/nsTreeSanitizer.cpp	Tue Jan 14 16:41:42 2014 +0900

Why is this whitelisting svg:iframe?  I would think we want it to be flattened here.

And given that, you don't need to add the src attr to kAttributesSVG either.

Comment 65

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8364894 [details] [diff] [review]
Part13: Create nsSVGIFrameFrame and common base class of nsSVGForeignObjectFrame and nsSVGIFrameFrame.

Shouldn't this frame be IsLeaf()?

In fact, I'd expect it should have more to do with nsSubDocumentFrame than with foreignobject....

Comment 66

[Boris Zbarsky [:bzbarsky]]

Oh, it does inherit from nsSubDocumentFrame, so nevermind comment 65.  ;)

Comment 67

[Robert Longson [:longsonr]]

Comment on attachment 8364887 [details] [diff] [review]
Part7: Add codes for handling nsGkAtoms::iframe (content/svg/content/src/*)

Boris already commented on this (comment 63)

Comment 68

[Robert Longson [:longsonr]]

Comment on attachment 8364885 [details] [diff] [review]
Part6: Add SVG_FROM_PARSER_TAG of SVGIframe

This should really be folded into part 5 patch.

Comment 69

[Robert Longson [:longsonr]]

Comment on attachment 8359642 [details] [diff] [review]
Part5:Create SVGIFrameELement class

>+{
>+  { &nsGkAtoms::x, 0, nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER, SVGContentUtils::X },
>+  { &nsGkAtoms::y, 0, nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER, SVGContentUtils::Y },
>+  { &nsGkAtoms::width, 0, nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER, SVGContentUtils::X },
>+  { &nsGkAtoms::height, 0, nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER, SVGContentUtils::Y },

remove trailing comma

>+};
>+
>+nsSVGElement::StringInfo SVGIFrameElement::sStringInfo[4] =
>+{
>+  { &nsGkAtoms::media, 0, true },

media is not animatable is it? I don't think it should be.

>+  { &nsGkAtoms::name, 0, true },
>+  { &nsGkAtoms::src, 0, true },
>+  { &nsGkAtoms::srcdoc, 0, true },

remove trailing comma

Have you thought about the performance implications of allowing src and srcdoc to be animatable? We'd have to unload the existing content and load another, that's probably not something we want to encourage doing frequently is it?

>+SVGIFrameElement::~SVGIFrameElement()
>+{
>+}

This does nothing so it's not needed.

>+SVGAnimatedPreserveAspectRatio *
>+SVGIFrameElement::GetPreserveAspectRatio()
>+{
>+  return &mPreserveAspectRatio;

Does it make any sense to have preserveAspectRatio without viewBox?

>+already_AddRefed<nsDOMSettableTokenList>
>+SVGIFrameElement::Sandbox()
>+{
>+  nsDOMSettableTokenList* pRet = nsContentUtils::GetTokenList(this, nsGkAtoms::sandbox);
>+  return pRet;

Don't need a temporary.

>+nsresult
>+SVGIFrameElement::SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
>+                          nsIAtom* aPrefix, const nsAString& aValue,
>+                          bool aNotify)
>+{
>+  nsresult rv = nsSVGElement::SetAttr(aNameSpaceID, aName, aPrefix,
>+                                      aValue, aNotify);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (aNameSpaceID == kNameSpaceID_None && aName == nsGkAtoms::src &&
>+      !HasAttr(kNameSpaceID_None,nsGkAtoms::srcdoc)) {
>+    // Don't propagate error here. The attribute was successfully set, that's
>+    // what we should reflect.
>+    LoadSrc();
>+  }
>+  if (aNameSpaceID == kNameSpaceID_None && aName == nsGkAtoms::srcdoc) {
>+    // Don't propagate error here. The attribute was successfully set, that's
>+    // what we should reflect.
>+    LoadSrc();
>+  }
>+  return NS_OK;
>+}

This is incorrect. If srcdoc or src is animated then HasAttr may return false but the attribute
will exist. Perhaps another reason that src and srcdoc should not be animated. If you're going
to have these attributes animated then you need to write tests for them animating properly.

>diff -r 60d53dab1988 -r 77b14cfc6085 content/svg/content/src/SVGIFrameElement.h
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/content/svg/content/src/SVGIFrameElement.h	Tue Jan 14 14:50:24 2014 +0900
>@@ -0,0 +1,121 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+//#include "SVGFrameElement.h"

No commented out code please.

>+#include "nsContentUtils.h"
>+#include "nsDOMSettableTokenList.h"
>+#include "nsFrameLoader.h"
>+#include "nsGenericFrameElement.h"
>+#include "nsIDocument.h"
>+#include "nsIDOMDocument.h"
>+#include "nsIDOMEventListener.h"
>+#include "nsIFrameLoader.h"
>+#include "nsIWebNavigation.h"
>+#include "nsSVGElement.h"
>+#include "nsSVGLength2.h"
>+#include "nsSVGString.h"
>+#include "SVGAnimatedPreserveAspectRatio.h"
>+#include "SVGAnimatedNumberList.h"

SVGAnimatedNumberList isn't used is it?

Comment 70

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Boris Zbarsky [:bz] from comment #58)
> Comment on attachment 8359636 [details] [diff] [review]
> Part1:Webidl of SVGIFrameElement
> >+  readonly attribute SVGAnimatedString media;
> 
> This doesn't seem to be in the IDL in the wiki.  Issue raised?

Hikosaka-san, please remove 'media' from these patches. The media feature should be tracked in a separate bug.

> More generally, why do we want all these things that are supposed to match
> the API <html:iframe> provides to be SVGAnimated*?  What are the use cases
> for animating src or srcdoc, for example, as compared to actually just
> setting them like people will expect to do?  Or name, of that matter?

The strings, at least, should be just DOMString. This part of the spec has yet to be reviewed so Hikosaka-san was just following the draft spec text. The final shape of the API probably depends on Cameron's reworking of the SVG DOM.

For now I think we can make the strings DOMString. The others can probably stay as SVGAnimated* until we know how the DOM is going to look in SVG2.

Comment 71

[Robert Longson [:longsonr]]

Oh and we really really need tests for this.

Comment 72

[Ryo HIKOSAKA]

Attachment: Part1:Webidl of SVGIFrameElement v2

Comment 73

[Ryo HIKOSAKA]

Attachment: Part3:Move nsGenericHTMLElement::GetTokenList implementation to Element v2

Comment 74

[Ryo HIKOSAKA]

Attachment: Part4:Create base class of nsGenericHTMLFrameElement and SVGIFrameElement v2

Comment 75

[Ryo HIKOSAKA]

Attachment: class_tree_svg_iframe.png v2

Comment 76

[Ryo HIKOSAKA]

Attachment: Part5:Create SVGIFrameELement class v2

Comment 77

[Ryo HIKOSAKA]

Attachment: Part9: Modify nsFrameLoader to handle SVG iframe v2

Comment 78

[Ryo HIKOSAKA]

Attachment: Part16: reftests (layout/reftests/svg/iframe)

Comment 79

[Ryo HIKOSAKA]

Attachment: Part17: mochitests - iframe sandbox general

Comment 80

[Ryo HIKOSAKA]

Attachment: Part18: mochitests - iframe sandbox inheritance

Comment 81

[Ryo HIKOSAKA]

Attachment: Part19: mochitests - iframe sandbox workers

Comment 82

[Ryo HIKOSAKA]

Attachment: Part20: mochitests - iframe sandbox same origin

Comment 83

[Ryo HIKOSAKA]

Attachment: Part21: mochitests - iframe sandbox navigation

Comment 84

[Ryo HIKOSAKA]

Attachment: Part22: mochitests - iframe sandbox popups

Comment 85

[Ryo HIKOSAKA]

Attachment: Part23: mochitests - iframe sandbox plugins

Comment 86

[Ryo HIKOSAKA]

Attachment: Part24: mochitest.ini(content/svg/test)

Comment 87

[Ryo HIKOSAKA]

Thanks for review and advice. I addressed review comment.

In patch part4, I removed nsImageLoadingContent.cpp from UNIFIED_SOURCES in moz.build.
Because I got following errors when I renamed nsGenericFrameElement to nsElementFrameLoaderOwner.
I'm not sure that it's right way to fix this problem...

 content/base/src/nsImageLoadingContent.cpp(611) : error C3861: 'LoadImage': identifier not found
 content/base/src/nsImageLoadingContent.cpp(703) : error C2039: 'LoadImage' : is not a member of 'nsImageLoadingContent'
 content/base/src/nsImageLoadingContent.cpp(708) : error C3861: 'GetOurOwnerDoc': identifier not found
 ...

I added reftests and mochi tests.
In the tests for sandbox, I used the test cases of HTML iframes as reference.
I copied a lot of support-files (file_sandbox_*.html) from content/html/content/test to content/svg/content/test to use from the test cases of SVG iframe sandbox (content/svg/content/test/test_iframe_sandbox_*.html).
Perhaps, is it good for adding these test cases in content/html/content/test to reuse file_sandbox_*.html?

Comment 88

[Ryo HIKOSAKA]

(In reply to Boris Zbarsky [:bz] from comment #63)
> Comment on attachment 8364887 [details] [diff] [review]
> Part7: Add codes for handling nsGkAtoms::iframe (content/svg/content/src/*)
> 
> Why stop at ::iframe in GetOuterSVGElement?  It doesn't stop for svg:image,
> which is what iframe should behave like wrt its descendants.  It should
> _not_ act like foreignObject, which does something quite different from the
> replaced element cases.
> 
> Similar in GetNearestViewportElement, etc.  In fact, most of this patch...

I changed iframe to act like svg:image.
As a result, I discard all of the codes in this part.
Do I understand correctly?

Comment 89

[Boris Zbarsky [:bzbarsky]]

> Do I understand correctly?

I didn't read all the code in the patch there, but seems plausible, yes.

Comment 90

[Boris Zbarsky [:bzbarsky]]

> I'm not sure that it's right way to fix this problem...

The right way is to find whoever is redefining LoadImage (e.g. if it's windows.h, whoever is including that) and make them stop doing that?

Comment 91

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8377966 [details] [diff] [review]
Part3:Move nsGenericHTMLElement::GetTokenList implementation to Element v2

> +++ b/content/base/public/Element.h	Tue Feb 18 10:12:54 2014 +0900

The new bits shouldn't be in the middle of the attribute-handling functions.  Please put them after GetLinkTarget instead or something?

r=me with that

Comment 92

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8377969 [details] [diff] [review]
Part4:Create base class of nsGenericHTMLFrameElement and SVGIFrameElement v2

>+++ b/content/base/src/nsElementFrameLoaderOwner.cpp	Tue Feb 18 10:19:52 2014 +0900
>+  mozilla::dom::Element* pThisElement = ThisFrameElement();

Don't need the "mozilla::dom::" bit here.  And just call the variable "thisElement", not "pThisElement", please.

>+++ b/content/html/content/src/nsGenericHTMLFrameElement.h	Tue Feb 18 10:19:52 2014 +0900
>+    : nsElementFrameLoaderOwner(aFromParser)
>+    , nsGenericHTMLElement(aNodeInfo)

These two should come in the opposite order.

r=me with those nits fixed.

Comment 93

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8377974 [details] [diff] [review]
Part9: Modify nsFrameLoader to handle SVG iframe v2

r=me

Comment 94

[Ryo HIKOSAKA]

Attachment: Part13: Create nsSVGIFrameFrame and common base class of nsSVGForeignObjectFrame and nsSVGIFrameFrame v2

I fixed a bug, which is detected by test case.

Comment 95

[Robert Longson [:longsonr]]

Comment on attachment 8377971 [details] [diff] [review]
Part5:Create SVGIFrameELement class v2

>+
>+  // nsSVGElement specializations:
>+  virtual gfxMatrix PrependLocalTransformsTo(const gfxMatrix &aMatrix,
>+                                             TransformTypes aWhich = eAllTransforms) const;
>+  
>+  // nsIContent
>+  virtual bool ParseAttribute(int32_t aNamespaceID,
>+                              nsIAtom* aAttribute,
>+                              const nsAString& aValue,
>+                              nsAttrValue& aResult) MOZ_OVERRIDE;
>+  virtual nsresult BindToTree(nsIDocument* aDocument, nsIContent* aParent,
>+                              nsIContent* aBindingParent,
>+                              bool aCompileEventHandlers);
>+  virtual void UnbindFromTree(bool aDeep = true,
>+                              bool aNullParent = true);
>+  virtual nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
>+                           nsIAtom* aPrefix, const nsAString& aValue,
>+                           bool aNotify);
>+  virtual nsresult AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
>+                                const nsAttrValue* aValue,
>+                                bool aNotify);
>+  virtual nsresult UnsetAttr(int32_t aNameSpaceID, nsIAtom* aAttribute,
>+                             bool aNotify) MOZ_OVERRIDE;
>+
>+  virtual void DestroyContent();
>+  nsresult CopyInnerTo(mozilla::dom::Element* aDest);
>+  virtual nsresult Clone(nsINodeInfo *aNodeInfo, nsINode **aResult) const;

Make them all MOZ_OVERRIDE rather than just some of them.

>--- a/content/svg/content/src/moz.build	Tue Feb 18 10:19:52 2014 +0900
>+++ b/content/svg/content/src/moz.build	Tue Feb 18 10:24:40 2014 +0900
>@@ -8,7 +8,11 @@
>     'nsSVGClass.h',
>     'nsSVGElement.h',
>     'nsSVGFeatures.h',
>+    'nsSVGLength2.h',
>+    'nsSVGTransform.h',
>+    'SVGAnimatedPreserveAspectRatio.h',

I don't see any reason for these changes in this patch.

>     'SVGAttrValueWrapper.h',
>+    'SVGContentUtils.h',

or this

>     'SVGPreserveAspectRatio.h',
>     'SVGStringList.h',
> ]
>     'SVGTitleElement.h',
>     'SVGTransform.h',
>     'SVGTransformableElement.h',
>+    'SVGTransformList.h',

or this.

If you need them they should be in a different patch where you can justify adding them.

r=longsonr with comments addressed.

Comment 96

[Robert Longson [:longsonr]]

The mochitests and reftests need to either check and skip running or set the pref before running, otherwise when we get to a release build the tree will turn orange (see 1st patch).

Comment 97

[Ryo HIKOSAKA]

I chatted with Jonathan in last week. He suggested splitting logically the patches in the bug into patches on several bugs. Because it is hard to have overlapping discussions about 24 patches all in one bug.
I agreed the suggestion. My idea is as follows:

Bug 1. Implementation Proposal for SVG iframe element - Create common implementation of nsIFrameLoaderOwner.
In this bug, we split off the implementation of nsIFrameLoaderOwner from nsGenericHTMLFrameElement for resusing from SVGIFrameElement class.
The bug includes the patches part 3 and 4.

Bug 2. Implementation Proposal for SVG iframe element - Create content class of SVG ifame.
In this bug. we create content class of SVGIFrameElement.
The bug includes the patches part 1, 2 and 5.
The bug depends on Bug 1.

Bug 3. Implementation Proposal for SVG iframe element - Create common implementation of SVG embedded frame.
In this bug, we split off the implementation of SVG embedded frame from nsSVGForeignObjectFrame and nsContainerFrame for resusing from nsSVGIFrameFrame class.
This bug includes the patches 12 and part of 13.

Bug 4. Implementation Proposal for SVG iframe element - Create frame class for SVG iframe.
In this bug. we create frame class of SVGIFrameElement.
This bug includes the patches 9, 10, 11, part of 13, 14, 15 and all tests.
The bug depends on Bug 1 and 3.

Perhaps, bug 1 and 2 can be in same bug, because the patches in bug 1 and 2 are already discussed and reviewed.

Jonathan, thanks for your time in busy schedule.
What do you think about the idea?

Comment 98

[Tomoaki Konno]

How is the status of your review for some patches?

Comment 99

[Jonathan Watt [:jwatt]]

I've unbitrotted some of these patches, addressed their outstanding review comments and addressed a few issues that I spotted:

https://hg.mozilla.org/integration/mozilla-inbound/rev/263812990c50
Move nsGenericHTMLElement::GetTokenList to Element
Was Part3

https://hg.mozilla.org/integration/mozilla-inbound/rev/53932844d047
Create a base class of nsGenericHTMLFrameElement and SVGIFrameElement
Was Part4

https://hg.mozilla.org/integration/mozilla-inbound/rev/85fa12537b41
Implement an SVGIFrameELement class
Was Part1, Part2 and Part5

https://hg.mozilla.org/integration/mozilla-inbound/rev/f360a73433c7
Update nsFrameLoader to handle SVG iframe
Was Part9

Comment 100

[Jonathan Watt [:jwatt]]

Comment on attachment 8364893 [details] [diff] [review]
Part12: Define helper methods on nsContainerFrame

Is this all because of the diamond inheritance?

Comment 101

[Jonathan Watt [:jwatt]]

Oh, and I meant to say in comment 99 that I changed:

SVGIFrameElement::AfterSetAttr:

-      nsCOMPtr<nsIDocShell> docshell = mFrameLoader->GetExistingDocShell();
-      if (docshell) {
-        uint32_t newFlags = nsContentUtils::ParseSandboxAttributeToFlags(aValue);
-        docshell->SetSandboxFlags(newFlags);
-      }
+      mFrameLoader->ApplySandboxFlags(GetSandboxFlags());


SVGIFrameElement::BindToTree:

-        nsCOMPtr<nsIDocShell> docshell = mFrameLoader->GetExistingDocShell();
-        if (docshell) {
-          uint32_t newFlags = GetSandboxFlags();
-          docshell->SetSandboxFlags(newFlags);
-        }
+        mFrameLoader->ApplySandboxFlags(GetSandboxFlags());


Because that looked like a gaping security hole. Why did you write it this way instead of the way it is in HTMLIFrameElement.cpp? Please request separate review for that explaining why in code comments.

Comment 102

[Ryo HIKOSAKA]

Thanks for updating the patches, Jonathan.

(In reply to Jonathan Watt [:jwatt] from comment #100)
> Comment on attachment 8364893 [details] [diff] [review]
> Part12: Define helper methods on nsContainerFrame
> 
> Is this all because of the diamond inheritance?

Yes, and I wrote supplementary explanation about the patch in comment #51

Comment 103

[Ryo HIKOSAKA]

(In reply to Jonathan Watt [:jwatt] from comment #101)

Thanks for modify the patch.

> Because that looked like a gaping security hole. Why did you write it this
> way instead of the way it is in HTMLIFrameElement.cpp? Please request
> separate review for that explaining why in code comments.

I did not mean much.
I can't recall about detail, but I think that I wrote this code following old version of HTMLIFrameElement.cpp.

Comment 104

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/263812990c50
https://hg.mozilla.org/mozilla-central/rev/53932844d047
https://hg.mozilla.org/mozilla-central/rev/85fa12537b41
https://hg.mozilla.org/mozilla-central/rev/f360a73433c7

Comment 105

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why do we need a separate IFRAME element? Is there a technical reason why we can't add the needed features to the HTML IFRAME element?

Comment 106

[Jonathan Watt [:jwatt]]

It would be nice if we could do that.

Here's a summary of how the HTML and SVG IDL compares:

HTML:
  attribute DOMString        width;
  attribute DOMString        height;
vs SVG:
  [Constant]
  readonly attribute SVGAnimatedLength width;
  [Constant]
  readonly attribute SVGAnimatedLength height;
(very different)

HTML:
  attribute DOMString        name;
  attribute DOMString        src;
  attribute DOMString        srcdoc;
vs SVG:
  [Constant]
  readonly attribute DOMString name;
  [Constant]
  readonly attribute DOMString src;
  [Constant]
  readonly attribute DOMString srcdoc;
(easy enough to share implementation)

HTML:
  readonly attribute nsIDOMDocument   contentDocument;
  readonly attribute nsIDOMWindow     contentWindow;
vs SVG:
  readonly attribute Document? contentDocument;
  readonly attribute WindowProxy? contentWindow;
(same)

SVG only:
  [Constant]
  readonly attribute SVGAnimatedLength x;
  [Constant]
  readonly attribute SVGAnimatedLength y;
  [Constant]
  readonly attribute SVGAnimatedPreserveAspectRatio preserveAspectRatio;
  [PutForwards=value]
  readonly attribute DOMSettableTokenList sandbox;
  [Constant]
  readonly attribute SVGAnimatedBoolean seamless;
  readonly attribute SVGAnimatedBoolean postpone;

HTML only:
  attribute DOMString        align;
  attribute DOMString        frameBorder;
  attribute DOMString        longDesc;
  attribute DOMString        marginHeight;
  attribute DOMString        marginWidth;
  attribute DOMString        scrolling;
  attribute boolean          allowFullscreen;

The main interface issue would be the width/height properties being different. There are other interface complications like whether we can make things appear as they should from the JS side. That is, no x, y, etc. if the element is HTML, no align, frameBorder etc. for SVG, and making instanceof work correctly.

Comment 107

[Boris Zbarsky [:bzbarsky]]

>  [Constant]
>  readonly attribute DOMString name;

Wait, why readonly and [Constant]?  And same for src/srcdoc?  That makes absolutely no sense to me.

Comment 108

[Jonathan Watt [:jwatt]]

I'm not active in the SVG WG so I have no more idea about the iframe stuff than anyone else. But that part does seem like something that could/should change.

Comment 109

[Boris Zbarsky [:bzbarsky]]

The [Constant] bit is not something to do with the WG, right?  And I really doubt our impl is actually returning a constant there...

Comment 110

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #105)
> Why do we need a separate IFRAME element? Is there a technical reason why we
> can't add the needed features to the HTML IFRAME element?

The intent of this change was simply to allow iframe directly in SVG without having to wrap in <foreignObject> but that also suggests adding x/y.

The WG has sometimes talked about allowing all HTML elements directly in SVG but no-one's ever really followed it through so currently SVG2 has a subset: image/video/audio/iframe/canvas/source/track (https://svgwg.org/svg2-draft/embedded.html).

(In reply to Jonathan Watt [:jwatt] from comment #106)
> It would be nice if we could do that.
> 
> Here's a summary of how the HTML and SVG IDL compares:

This IDL is actually quite different to what's in the spec--and that spec text hasn't been reviewed by the WG so basically we're quite free to change this.

>   readonly attribute SVGAnimatedBoolean seamless;
>   readonly attribute SVGAnimatedBoolean postpone;

I'm not sure it makes much sense to animate these (and once we fix the SVG DOM, we'll probably allow animating stuff without having to use these SVGAnimated* types).

Comment 111

[Ryo HIKOSAKA]

(In reply to Brian Birtles (:birtles) from comment #110)
> This IDL is actually quite different to what's in the spec--and that spec
> text hasn't been reviewed by the WG so basically we're quite free to change
> this.

Brian-san
Is it better to change the IDL, or should I wait until the review of the spec in the WG?

Comment 112

[Jonathan Watt [:jwatt]]

We're going to need review of the webidl again before turning this on, so I wouldn't bother for the moment.

Comment 113

[Tomoaki Konno]

x,y,width and height attributes are not constant. These attributes can be changed with setAttribute(*,*). I think that [Constant] should be removed from the webidl in that patch (Part1).

Comment 114

[Tomoaki Konno]

Attributes such as "src" are copied from HTMLIFrameElement to the SVGIFrameElement in this patch(Part1).
On the other hand, SVG spec says that these attributes implemented from HTMLIFrameElement (SVGIFrameElement implements HTMLIFrameElement;).
However, this spec notes that this implementation might cause multiple inheritance of SVGElement and HTMLElement[1].
And also, it is related to the work of improving SVG DOM, which will mix SVG and HTML DOM[2].

I think that the current interfaces in this patch would be almost acceptable for application developers.
Because, the exposed interfaces (src etc.) would not consequently change (if it is copied from HTMLIFrameElement or implemented from HTMLIFrameElement).

So, could you push the review process of this current idl?

[1] http://www.w3.org/TR/SVG2/single-page.html#embedded-DOMInterfaces
[2] http://dev.w3.org/SVG/proposals/improving-svg-dom/#interface-hierarchy

Comment 115

[Boris Zbarsky [:bzbarsky]]

> SVGIFrameElement implements HTMLIFrameElement;

You really don't want that.  It would make SVGIFrameElement have all sorts of weird behavior....

Comment 116

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Tomoaki Konno from comment #114)
> So, could you push the review process of this current idl?

I'm not sure what you're asking for? To take something to the SVG WG?

I'm also not sure which version of the IDL you want to use. The IDL in these patches is quite different to the (unreviewed, and, as Boris points out, undesirable) spec text.

As for the patch review process Jonathan is currently working on it and seeing if there is a way to avoid the inheritance problems there.

Comment 117

[Tomoaki Konno]

(In reply to Brian Birtles (:birtles) from comment #116)
> I'm not sure what you're asking for? To take something to the SVG WG?
No. I just wanted to make sure whether the review of these patches is progressed without waiting the review result of the spec in the WG.
 
> As for the patch review process Jonathan is currently working on it and
> seeing if there is a way to avoid the inheritance problems there.
That's good enough for me.

Comment 118

[Jonathan Watt [:jwatt]]

Comment on attachment 8377976 [details] [diff] [review]
Part16: reftests (layout/reftests/svg/iframe)

The width and height on the red rect in layout/reftests/svg/iframe/iframe-srcdoc.svg is broken, leading that test to always pass. (It's also missing the comment that you've added to the other tests.)

I'd also like to have some tests that compare the rendering of <iframe> in SVG directly to the rendering of <iframe> in an HTML file, and with more complex content than colored rectangles (an embedded page containing text and images, say).

Otherwise this looks good.

Comment 119

[Ryo HIKOSAKA]

Attachment: Part5: Modify SVG iframe webidl

Comment 120

[Ryo HIKOSAKA]

Attachment: Part6: Add nsSVGIFrameFrame to nsFrameIdList

Comment 121

[Ryo HIKOSAKA]

Attachment: Part7: Add svgIFrameFrame to nsGkAtomList

Comment 122

[Ryo HIKOSAKA]

Attachment: Part8: Create nsSVGIFrameFrame and common base class of nsSVGForeignObjectFrame and nsSVGIFrameFrame v3

Comment 123

[Ryo HIKOSAKA]

Attachment: Part9: Add frame constructor for SVGIFrameFrame

Comment 124

[Ryo HIKOSAKA]

Attachment: Part10: Add code for handle svgIFrameFrame to nsDocumentViewer

Comment 125

[Ryo HIKOSAKA]

(In reply to Jonathan Watt [:jwatt] from comment #118)
> Comment on attachment 8377976 [details] [diff] [review]
> Part16: reftests (layout/reftests/svg/iframe)
> 

Thanks for review. I will modify the test.

Comment 126

[Ryo HIKOSAKA]

I have unbitrotted the rest of patches.
And I have changed idl as follows:

-Before:
[Constant]
readonly attribute DOMString name;
[Constant]
readonly attribute DOMString src;
[Constant]
readonly attribute DOMString srcdoc;

-After:
attribute DOMString        name;
attribute DOMString        src;
attribute DOMString        srcdoc;

The review of the idl have not been finished, but it seems to be better to remove constants and readonly at the moment.

And I removed the patch part 12(Define helper methods on nsContainerFrame).
Because I noticed that nsSVGIFrameFrame class doesn't need the methods in the patch.  (SetInitialChildList, AppendFrames, InsertFrames and RemoveFrame)
Jonathan, sorry to waste your time for review of this patch.

Comment 127

[Jesse Ruderman]

Added <svg:iframe> to DOM fuzzer (0a674ed659a0).

Comment 128

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I haven't seen an Intent to Implement <https://wiki.mozilla.org/WebAPI/ExposureGuidelines> before implementing this, so I think I want to see one now.

Comment 129

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Brian Birtles (:birtles) from comment #110)
> The intent of this change was simply to allow iframe directly in SVG without
> having to wrap in <foreignObject> but that also suggests adding x/y.
> 
> The WG has sometimes talked about allowing all HTML elements directly in SVG
> but no-one's ever really followed it through so currently SVG2 has a subset:
> image/video/audio/iframe/canvas/source/track
> (https://svgwg.org/svg2-draft/embedded.html).

Hmm, I had not seen that before.

I think this approach to importing elements in SVG is very dangerous. See comment #115. It's also going to be a mess to keep in sync with HTML5 as those elements evolve. It will constrain HTML5 evolution too. Already there are bugs; e.g. note that, as written, the spec says that SVGIframeElement has two 'width' attributes with different types. I hope we can find a much better approach, but this bug is not really the right place to have that discussion. I'll open a thread in www-svg.

Sorry for being behind in my bugmail.

Comment 130

[Jonathan Watt [:jwatt]]

http://lists.w3.org/Archives/Public/www-svg/2014Jun/thread.html#msg63
http://lists.w3.org/Archives/Public/public-whatwg-archive/2014Jun/thread.html#msg51

Comment 131

[Ryo HIKOSAKA]

(In reply to :Ms2ger from comment #128)
> I haven't seen an Intent to Implement
> <https://wiki.mozilla.org/WebAPI/ExposureGuidelines> before implementing
> this, so I think I want to see one now.

Sorry, I sent the mail to [e-mail].
https://groups.google.com/forum/#!topic/mozilla.dev.platform/Ux_s1AC4NKA

Comment 132

[Jonathan Watt [:jwatt]]

From the list discussion it seems we're heading in a different direction to the one pursued by the patches here. I'll remove the review requests for now while that is sorted out (including the requests for the tests, even though they may be quite close to what we eventually want the behavior to be).

Comment 133

[Ryo HIKOSAKA]

Attachment: Part8: Create nsSVGIFrameFrame and common base class of nsSVGForeignObjectFrame and nsSVGIFrameFrame v4

Maybe there is no point to update the patches at the moment, but I fixed a following bug in the patch part8:
- Subdocument size will be incorrect when devicePixelRatio is not 1.0.

Comment 134

[Robert Longson [:longsonr]]

I don't think so. It's a huge amount of code to get us from

<foreignObject><iframe/></foreignObject>

to

<iframe/>

so it doesn't seem to offer enough bang for the buck at the moment in my opinion.

Comment 135

[Brian Birtles (:birtles, Feb 26~ parental leave)]

I filed bug 1108887 for backing out the parts of this that have landed.