999964 - Implementation Proposal for 'clipped' option of SVG 2 getBBox method.

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Shigeyuki Tsukihashi]

Attachment: Patch for SVG 2 getBBox method.

We are planning to implement control of dynamic loading using 'postpone' attribute of Resource Priorities.
(http://www.w3.org/Graphics/SVG/WG/wiki/Proposals/ResourcePriorities_for_SVG#Loading_condition_of_the_element)
(http://www.w3.org/Graphics/SVG/WG/wiki/Proposals/globalView#Tiling)
In this case, it becomes important to obtain the bounding box of the clipped domain.
Because it will be more efficient cases to be loaded only when the display part of the clipped object appears in viewport.

In the SVG 2, getBBox method will be extended, and we can get the bounding box of the clipping area.
(https://svgwg.org/svg2-draft/single-page.html#types-SVGBoundingBoxOptions)

So, before the implementation of dynamic loading, we have decided to implement 'clipped' option in getBBox method of the SVG 2.
In addition, FireFox already has the procedure for calculation of 'fill','stroke' and 'markers' option, so I didn't modify GetBBoxContribution function.

Comment 1

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

Hi Tsukihashi-san, thanks for the patch. The whitespace in the patch needs to be fixed before it is ready for review (2 space indent, not tabs). See: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Whitespace
Thanks

Comment 2

[Shigeyuki Tsukihashi]

(In reply to Brian Birtles (:birtles) from comment #1)
Hi Brian-san, thank you for comment. I will submit the modified patch.

Comment 3

[Shigeyuki Tsukihashi]

Attachment: Patch for SVG 2 getBBox method.

Comment 4

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

Hi Tsukihashi-san, is this patch ready for review?
Do you have any tests for this code?

Comment 5

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

If the patch is ready for review, Robert Longson would be a good reviewer but I think you should add tests first.

You should probably use mochitests (https://developer.mozilla.org/en/docs/Mochitest) since this is a Javascript API. Please let me know if you need help (birtles on ircs://irc.mozilla.org:6697/#svg). Otherwise Hikosaka-san can probably assist.

Comment 6

[Shigeyuki Tsukihashi]

(In reply to Brian Birtles (:birtles) from comment #5)
Brian-san, thank you for your advice and supports.
I am creating mochitests and I will submit them next week.

Comment 7

[Shigeyuki Tsukihashi]

Attachment: mochitest for Bug 999964.

Comment 8

[Robert Longson [:longsonr]]

Comment on attachment 8410824 [details] [diff] [review]
Patch for SVG 2 getBBox method.

>+  if (!Preferences::GetBool("svg.new-getBBox.enabled", false)) {

Should probably have this as a function in nsSVGUtils like the other SVG preferences.

>+  } else {
>+    if (!frame) {
>+      rv.Throw(NS_ERROR_FAILURE);
>+      return nullptr;
>+    }
>+    if (frame->GetStateBits() & NS_FRAME_IS_NONDISPLAY) {
>+      return NS_NewSVGRect(this,0,0,0,0);

Why this change in behaviour. It seems wrong.

>+    }
>+    nsISVGChildFrame* svgframe = do_QueryFrame(frame);
>+    if (!svgframe) {
>+      rv.Throw(NS_ERROR_NOT_IMPLEMENTED); // XXX: outer svg
>+      return nullptr;
>+    }
> 


>     gfxMatrix matrix;
>-    if (aFrame->GetType() == nsGkAtoms::svgForeignObjectFrame) {
>+    if (aFrame->GetType() == nsGkAtoms::svgForeignObjectFrame ||
>+        (Preferences::GetBool("svg.new-getBBox.enabled", false) &&
>+         aFrame->GetType() == nsGkAtoms::svgUseFrame)) {

Why do this? Is this a pre-existing bug i.e. do we do the wrong thing without the pref set? If so
we shouldn't just fix things if the pref is enabled.

>+    // Account for 'clipped'.
>+    if (aFlags & nsSVGUtils::eBBoxIncludeClipped) {
>+      gfxRect clipRect(0,0,0,0);
>+      float x, y, width, height;
>+      gfxMatrix wmat;

wmat doesn't really mean anything to me. Can you pick a better name please?

>+      gfxRect fillBBox = 
>+        svg->GetBBoxContribution(ToMatrix(wmat), 
>+                                 nsSVGUtils::eBBoxIncludeFill).ToThebesRect();
>+      x = fillBBox.x;
>+      y = fillBBox.y;
>+      width = fillBBox.width;
>+      height = fillBBox.height;
>+      bool hasClip = 
>+        static_cast<const nsSVGElement*>(content)->HasAttr(
>+          kNameSpaceID_None, nsGkAtoms::clip);

This is wrong, clips may exist due to SMIL animation so calling HasAttr in any SVG code is pretty much always a mistake. You'd need to call (StyleDisplay()->IsScrollableOverflow()) to check whether there's a clip.

>+      nsSVGEffects::EffectProperties effectProperties =
>+        nsSVGEffects::GetEffectProperties(aFrame);
>+      bool isOK = true;
>+      nsSVGClipPathFrame *clipPathFrame = 
>+        effectProperties.GetClipPathFrame(&isOK);
>+      if (clipPathFrame && isOK) {

if isOK is false then nothing is rendered so you should probably return a 0,0,0,0 rect. Should have a test for this. i.e. a clip-path that points to a something that isn't a clipPath.

>+        SVGClipPathElement *clipContent = 
>+          static_cast<SVGClipPathElement*>(clipPathFrame->GetContent());
>+        nsRefPtr<SVGAnimatedEnumeration> units = clipContent->ClipPathUnits();
>+        if (units->AnimVal() == SVG_UNIT_TYPE_OBJECTBOUNDINGBOX) {
>+          matrix = gfxMatrix().Scale(width, height) *
>+                      gfxMatrix().Translate(gfxPoint(x, y)) *
>+                      matrix;
>+        }
>+        nsIContent* node = clipPathFrame->GetContent()->GetFirstChild();
>+        gfxRect bbox1(0,0,0,0), bbox2;
>+        if (node) {
>+          for (; node; node = node->GetNextSibling()) {
>+            nsIFrame *wframe = 
>+              static_cast<nsSVGElement*>(node)->GetPrimaryFrame();
>+            if (wframe) {
>+              nsISVGChildFrame *svg2 = do_QueryFrame(wframe);
>+              if (svg2) {
>+                bbox2 = svg2->GetBBoxContribution(ToMatrix(matrix), 
>+                              nsSVGUtils::eBBoxIncludeFill).ToThebesRect();
>+                bbox2 = bbox.Intersect(bbox2);
>+                if (bbox2.width > 0 && bbox2.height > 0) {

!IsEmpty()

>+                  if (bbox1.width == 0 && bbox1.height == 0) {
>+                    bbox1 = bbox2;

Not sure what this is doing, can you use SVGBBox instead?

>+                  } else {
>+                    bbox1 = bbox1.UnionEdges(bbox2);
>+                  }

This doesn't seem to cope with nested clipPaths as far as I can tell i.e.

•The ‘clipPath’ element or any of its children can specify property ‘clip-path’.
If a valid ‘clip-path’ reference is placed on a ‘clipPath’ element, the resulting clipping path is the intersection of the contents of the ‘clipPath’ element with the referenced clipping path.
 If a valid ‘clip-path’ reference is placed on one of the children of a ‘clipPath’ element, then the given child element is clipped by the referenced clipping path before OR'ing the silhouette of the child element with the silhouettes of the other child elements.

Comment 9

[Robert Longson [:longsonr]]

Comment on attachment 8414991 [details] [diff] [review]
mochitest for Bug 999964.

Needs test for clipPath intersections and unions i.e.

•The ‘clipPath’ element or any of its children can specify property ‘clip-path’.
If a valid ‘clip-path’ reference is placed on a ‘clipPath’ element, the resulting clipping path is the intersection of the contents of the ‘clipPath’ element with the referenced clipping path.
 If a valid ‘clip-path’ reference is placed on one of the children of a ‘clipPath’ element, then the given child element is clipped by the referenced clipping path before OR'ing the silhouette of the child element with the silhouettes of the other child elements.


See http://www.w3.org/Graphics/SVG/Test/20110816/harness/htmlObjectApproved/masking-path-07-b.html for an example of this.

Comment 10

[Shigeyuki Tsukihashi]

(In reply to Robert Longson from comment #8)

Thanks for comment, Robert.
I will modify the patch and the mochitest based on your comment.

> Why this change in behaviour. It seems wrong.

Please see "7.11. Bounding boxes" of SVG 2 W3C Editor's Draft.
( https://svgwg.org/svg2-draft/single-page.html#coords-BoundingBoxes )
EXAMPLE 25 says that "defs-1" returns {0, 0, 0, 0}.
Without this change, FireFox occurs Exception.(nsresult:"0x80004005 (NS_ERROR_FAILURE)")

> Why do this? Is this a pre-existing bug i.e. do we do the wrong thing
> without the pref set? If so
> we shouldn't just fix things if the pref is enabled.

I don't know about svgForeignObjectFrame.
According to the above EXAMPLE 25, "use-1" return {30, 30, 40, 40};
But without this change, "use-1" returns {20, 20, 40, 40};

> Not sure what this is doing, can you use SVGBBox instead?

In the following case, if bbox1 is {0,0,0,0} and bbox2 is {50,50,100,100},
the result of bbox1.UnionEdges(bbox2) is {0,0,100,100};
But expected result is {50,50,100,100}.

  <defs>
    <clipPath id="circle" clip-rule="evenodd">
      <rect x="50" y="50" width="100" height="100"/>
      <rect x="250" y="250" width="100" height="100"/>
    </clipPath>
  </defs>

SVGBBox doesn't have Intersect() function. So, I would like to use gfxRect.

Comment 11

[Robert Longson [:longsonr]]

> Please see "7.11. Bounding boxes" of SVG 2 W3C Editor's Draft.
> ( https://svgwg.org/svg2-draft/single-page.html#coords-BoundingBoxes )
> EXAMPLE 25 says that "defs-1" returns {0, 0, 0, 0}.
> Without this change, FireFox occurs Exception.(nsresult:"0x80004005
> (NS_ERROR_FAILURE)")

Please put this in a separate patch/bug.

> 
> I don't know about svgForeignObjectFrame.
> According to the above EXAMPLE 25, "use-1" return {30, 30, 40, 40};
> But without this change, "use-1" returns {20, 20, 40, 40};
> 
> > Not sure what this is doing, can you use SVGBBox instead?
> 
> In the following case, if bbox1 is {0,0,0,0} and bbox2 is {50,50,100,100},
> the result of bbox1.UnionEdges(bbox2) is {0,0,100,100};
> But expected result is {50,50,100,100}.

That's what the nsSVGUtils::SVGBBox class handles. Please rewrite using that class as it handles empty
boxes. (http://mxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGUtils.h#84)

> 
>   <defs>
>     <clipPath id="circle" clip-rule="evenodd">
>       <rect x="50" y="50" width="100" height="100"/>
>       <rect x="250" y="250" width="100" height="100"/>
>     </clipPath>
>   </defs>
> 
> SVGBBox doesn't have Intersect() function. So, I would like to use gfxRect.

Add Intersect() to SVGBBox.

Comment 12

[Shigeyuki Tsukihashi]

(In reply to Robert Longson from comment #11)

Thanks for comment.

> Please put this in a separate patch/bug.

I understand.

> That's what the nsSVGUtils::SVGBBox class handles. Please rewrite using that
> class as it handles empty
> boxes.
> (http://mxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGUtils.h#84)
> 
> Add Intersect() to SVGBBox.

OK, I will add Intersect() to nsSVGUtils::SVGBBox.

Comment 13

[Shigeyuki Tsukihashi]

Attachment: Patch for SVG 2 getBBox method. (2014-05-02)

Comment 14

[Shigeyuki Tsukihashi]

Attachment: mochitest for Bug 999964. (2014-05-02)

Comment 15

[Robert Longson [:longsonr]]

Comment on attachment 8416331 [details] [diff] [review]
Patch for SVG 2 getBBox method. (2014-05-02)

>     gfxMatrix matrix;
>-    if (aFrame->GetType() == nsGkAtoms::svgForeignObjectFrame) {
>+    if (aFrame->GetType() == nsGkAtoms::svgForeignObjectFrame ||
>+          (NS_SVGNewGetBBoxEnabled() &&
>+           aFrame->GetType() == nsGkAtoms::svgUseFrame)) {

I don't think the NS_SVGNewGetBBoxEnabled test should be there. If the implementation is wrong it's almost certainly wrong without the flag set.

>+    // Account for 'clipped'.
>+    if (aFlags & nsSVGUtils::eBBoxIncludeClipped) {
>+      gfxRect clipRect(0,0,0,0);

single space after each comma please.

>+          clipRect = matrix.TransformBounds( clipRect );

No spaces after ( or before )

>+      nsSVGClipPathFrame *clipPathFrame = 
>+        effectProperties.GetClipPathFrame(&isOK);
>+      if (clipPathFrame && isOK) {


>+        SVGClipPathElement *clipContent = 
>+          static_cast<SVGClipPathElement*>(clipPathFrame->GetContent());
>+        nsRefPtr<SVGAnimatedEnumeration> units = clipContent->ClipPathUnits();
>+        if (units->AnimVal() == SVG_UNIT_TYPE_OBJECTBOUNDINGBOX) {
>+          matrix = gfxMatrix().Scale(width, height) *
>+                      gfxMatrix().Translate(gfxPoint(x, y)) *
>+                      matrix;
>+        }
>+        gfxRect bbox1 = 
>+          GetBBoxForClipPathFrame(clipPathFrame, bbox, matrix).ToThebesRect();

This bit above would be better off in nsSVGClipPathFrame as a member function that gets called.

>+        if (!isOK) {
>+          bbox = gfxRect(0,0,0,0);

single space after each comma please.

> 
>+SVGBBox
>+nsSVGUtils::GetBBoxForClipPathFrame(nsSVGClipPathFrame* aFrame, 
>+                               const SVGBBox &aBBox, const gfxMatrix &aMatrix)

I think this code should be a member function of nsSVGClipPathFrame instead of living in nsSVGUtils

>+{
>+  nsIContent* node = aFrame->GetContent()->GetFirstChild();
>+  SVGBBox bbox1, bbox2;

These are rather meaningless names. It wouldn't matter if there was only one of them, but
there's two which makes the code hard to follow. Can you think of any better names?

r=longsonr with comments addressed.

Comment 16

[Shigeyuki Tsukihashi]

(In reply to Robert Longson from comment #15)

Thanks for comment and advise, Robert.
I will modify the patch based on your comment.

Comment 17

[Shigeyuki Tsukihashi]

Attachment: Patch for SVG 2 getBBox method. (2014-05-08)

Comment 18

[Shigeyuki Tsukihashi]

Attachment: mochitest for Bug 999964. (2014-05-08)

Comment 19

[Shigeyuki Tsukihashi]

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

Hi Brian-san, should I request superreview for these patches ?
Could you give me some advise ?

Comment 20

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

(In reply to Shigeyuki Tsukihashi from comment #19)
> Hi Brian-san, should I request superreview for these patches ?
> Could you give me some advise ?

Hi Tsukihashi-san, you don't need superreview for this (unless the reviewer says so). On the following kinds of patches need super-review:
"Patches which cross modules, change APIs, or have security-related changes must have an additional super-review."
(https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_reviewed)

If you have addressed all review feedback, this patch can be checked-in:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree

But first, have you tested this on try server? If you don't have access to try server I will do it for you.

(Also, one more tip, if you have a question for someone, you can normally get their attention more easily by setting the "Need more information from ..." field)

Comment 21

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

(In reply to Brian Birtles (:birtles) from comment #20)
> On the following kinds of patches need super-review
  ^ "Only"

Comment 22

[Shigeyuki Tsukihashi]

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

Hi Brian-san, Thank you for comment and advise. I understand about super-review.

> But first, have you tested this on try server? If you don't have access to
> try server I will do it for you.

I don't have tested on try server. I'm studying about how to use try server.
Could you do it for me? 
Thanks advance.

Comment 23

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

(In reply to Shigeyuki Tsukihashi from comment #22)
> I don't have tested on try server. I'm studying about how to use try server.
> Could you do it for me? 

Here is the try server run: https://tbpl.mozilla.org/?tree=Try&rev=7e423cf47742

It will take several hours before all the results are available. If everything is green or known-orange then you can add the checkin-needed keyword.

I will help you interpret the results when it has finished.

Comment 24

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

(In reply to Brian Birtles (:birtles) from comment #23)
> Here is the try server run:
> https://tbpl.mozilla.org/?tree=Try&rev=7e423cf47742

It fails to build on some platforms so I cancelled the rest of the jobs.

Can you fix the build failure and upload a new patch? If you have access to a Linux or OSX machine it will be easy to check if there are other similar build failures.

Comment 25

[Shigeyuki Tsukihashi]

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

Brian-san, Thank you for your comment.
I will modify the patch.

Comment 26

[Markus Stange [:mstange]]

Comment on attachment 8419143 [details] [diff] [review]
Patch for SVG 2 getBBox method. (2014-05-08)

>diff --git a/layout/svg/nsSVGUtils.cpp b/layout/svg/nsSVGUtils.cpp

>@@ -880,28 +892,94 @@ nsSVGUtils::GetBBox(nsIFrame *aFrame, ui

>-    if (aFrame->GetType() == nsGkAtoms::svgForeignObjectFrame) {
>-      // The spec says getBBox "Returns the tight bounding box in *current user
>-      // space*". So we should really be doing this for all elements, but that
>-      // needs investigation to check that we won't break too much content.
>-      // NOTE: When changing this to apply to other frame types, make sure to
>-      // also update nsSVGUtils::FrameSpaceInCSSPxToUserSpaceOffset.
>-      NS_ABORT_IF_FALSE(content->IsSVG(), "bad cast");
>+    if (!NS_SVGNewGetBBoxEnabled()) {
>+      if (aFrame->GetType() == nsGkAtoms::svgForeignObjectFrame) {
>+        // The spec says getBBox "Returns the tight bounding box in *current user
>+        // space*". So we should really be doing this for all elements, but that
>+        // needs investigation to check that we won't break too much content.

>+        // NOTE: When changing this to apply to other frame types, make sure to
>+        // also update nsSVGUtils::FrameSpaceInCSSPxToUserSpaceOffset.

You ignored this comment when...

>+        NS_ABORT_IF_FALSE(content->IsSVG(), "bad cast");
>+        nsSVGElement *element = static_cast<nsSVGElement*>(content);
>+        matrix = element->PrependLocalTransformsTo(matrix,
>+                            nsSVGElement::eChildToUserSpace);
>+      }
>+      return svg->GetBBoxContribution(ToMatrix(matrix), aFlags).ToThebesRect();
>+    }
>+    
>+    if (aFrame->GetType() == nsGkAtoms::svgForeignObjectFrame ||
>+        aFrame->GetType() == nsGkAtoms::svgUseFrame) {
                                          ^^^^^^^^^^^
...you made this change.

I expect this to result in failures of a few tests in layout/reftests/invalidation/.

Comment 27

[Shigeyuki Tsukihashi]

(In reply to Markus Stange [:mstange] from comment #26)
> >+        // NOTE: When changing this to apply to other frame types, make sure to
> >+        // also update nsSVGUtils::FrameSpaceInCSSPxToUserSpaceOffset.
> 
> I expect this to result in failures of a few tests in layout/reftests/invalidation/.

Thank you for your comment. 
I will modify the patch.

Comment 28

[Shigeyuki Tsukihashi]

Attachment: Patch for SVG 2 getBBox method. (2014-05-09)

Comment 29

[Shigeyuki Tsukihashi]

Attachment: mochitest for Bug 999964. (2014-05-09)

Comment 30

[Shigeyuki Tsukihashi]

Attachment: Patch for SVG 2 getBBox method. (2014-05-12)

Comment 31

[Shigeyuki Tsukihashi]

Attachment: mochitest for Bug 999964. (2014-05-12)

Comment 32

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

Comment on attachment 8420786 [details] [diff] [review]
mochitest for Bug 999964. (2014-05-12)

Review of attachment 8420786 [details] [diff] [review]:
-----------------------------------------------------------------

I think these test files should be more descriptively named, e.g. test_getBBox.html rather than text_bug999964

::: content/svg/content/test/test_bug999964.xhtml
@@ +1,2 @@
> +<!DOCTYPE HTML>
> +<html xmlns="http://www.w3.org/1999/xhtml">

Nit: It's not really necessary to use xhtml for test files anymore.

@@ +29,5 @@
> +  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> +  var prefService = Components.classes["@mozilla.org/preferences-service;1"]
> +                              .getService(Components.interfaces.nsIPrefService);
> +  var svgBranch = prefService.getBranch("svg.");
> +  var flag = svgBranch.getBoolPref("new-getBBox.enabled");

Why are we doing this? Why not just use SpecialPowers.getBoolPref("svg.new-getBBox.enabled") ?

@@ +124,5 @@
> +  checkBBox("image11", opt, 0,0,400,400, 0); 
> +  checkBBox("image12", opt, 25,43,768,768, 0); 
> +  checkBBox("image13", opt, 0,0,400,400, 0);
> +  
> +  // <path>

Nit: There is lots of trailing whitespace in this file.

::: layout/reftests/invalidation/reftest.list
@@ +21,5 @@
>  == filter-userspace-offset.svg?offsetContainer=innerSVG&filter=matrix-boundingBox filter-userspace-offset.svg
>  == filter-userspace-offset.svg?offsetContainer=foreignObject&filter=matrix-boundingBox filter-userspace-offset.svg
>  == filter-userspace-offset.svg?offsetContainer=rect&filter=flood-userSpace-at100 filter-userspace-offset.svg
> +pref(svg.new-getBBox.enabled,true) == filter-userspace-offset.svg?offsetContainer=use&filter=flood-userSpace-at100 filter-userspace-offset.svg
> +pref(svg.new-getBBox.enabled,false) == filter-userspace-offset.svg?offsetContainer=use&filter=flood-userSpace-atZero filter-userspace-offset.svg

Can you explain why this is needed? Thanks.

Comment 33

[Shigeyuki Tsukihashi]

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

Brian-san, Thank you for your comment.

> Comment on attachment 8420786 [details] [diff] [review]
> mochitest for Bug 999964. (2014-05-12)
> 
> Why are we doing this? Why not just use
> SpecialPowers.getBoolPref("svg.new-getBBox.enabled") ?

Because I didn't know about SpecialPowers APIs.
I understand. I will modify the patch.

> Can you explain why this is needed? Thanks.

Because of the following change of nsSVGUtils::GetBBox, the bounding box of <use> element is equal to 
the bounding box of <foreignObject> element in this reftest.

>    if (aFrame->GetType() == nsGkAtoms::svgForeignObjectFrame ||
>        aFrame->GetType() == nsGkAtoms::svgUseFrame) {
>      nsSVGElement *element = static_cast<nsSVGElement*>(content);
>      matrix = element->PrependLocalTransformsTo(matrix,
>                          nsSVGElement::eChildToUserSpace);
>    }

Thanks.

Comment 34

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

(In reply to Robert Longson from comment #15)
> Comment on attachment 8416331 [details] [diff] [review]
> Patch for SVG 2 getBBox method. (2014-05-02)
> 
> >     gfxMatrix matrix;
> >-    if (aFrame->GetType() == nsGkAtoms::svgForeignObjectFrame) {
> >+    if (aFrame->GetType() == nsGkAtoms::svgForeignObjectFrame ||
> >+          (NS_SVGNewGetBBoxEnabled() &&
> >+           aFrame->GetType() == nsGkAtoms::svgUseFrame)) {
> 
> I don't think the NS_SVGNewGetBBoxEnabled test should be there. If the
> implementation is wrong it's almost certainly wrong without the flag set.

Tsukihashi-san, I believe Robert is saying that we shouldn't be fixing the behavior for svgUseFrame under the flag (pref).

In your current patch, under nsSVGUtils::GetBBox we treat use elements different depending on whether the flag is set or not. I agree with Robert that we should treat use elements the same regardless of whether or not the flag is set. The flag is for determining if the extra parameters to getBbox are allowed.

The fixes to useFrame are fine (I assume you have tests that fail without them), but please make them apply even when the flag is not set.

This will mean you don't need to add pref checks to the invalidation reftest.list. It will also allow you to fix the code duplication in nsSVGUtils::FrameSpaceInCSSPxToUserSpace.

Thanks!

Comment 35

[Shigeyuki Tsukihashi]

(In reply to Brian Birtles (:birtles) from comment #34)
> (In reply to Robert Longson from comment #15)
> 
> Tsukihashi-san, I believe Robert is saying that we shouldn't be fixing the
> behavior for svgUseFrame under the flag (pref).

Brian-san, Thank you for your comment 
I'm so sorry, I was not able to understand correctly what Robert said. 
 
> The fixes to useFrame are fine (I assume you have tests that fail without
> them), but please make them apply even when the flag is not set.
> 
> This will mean you don't need to add pref checks to the invalidation
> reftest.list. It will also allow you to fix the code duplication in
> nsSVGUtils::FrameSpaceInCSSPxToUserSpace.
> 
> Thanks!

I understand and I will modify the patch.
Thank you so much.

Comment 36

[Shigeyuki Tsukihashi]

Attachment: Patch for SVG 2 getBBox method. (2014-05-13)

Comment 37

[Shigeyuki Tsukihashi]

Attachment: mochitest for Bug 999964. (2014-05-13)

Comment 38

[Shigeyuki Tsukihashi]

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

Brian-san, I'm sorry to take up your time, but would you check patches of today?
Here is the try server run: 
  https://tbpl.mozilla.org/?tree=Try&rev=d39c3a1f50fe

Thanks.

Comment 39

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

(In reply to Shigeyuki Tsukihashi from comment #38)
> Brian-san, I'm sorry to take up your time, but would you check patches of
> today?
> Here is the try server run: 
>   https://tbpl.mozilla.org/?tree=Try&rev=d39c3a1f50fe

Hi Tuskihashi-san, it seems like there are quite a few test failures in that try run.

More concerning, however is we seem to be leaking attribute tear-offs:

  [1824] ###!!! ABORT: Tear-off objects remain in hashtable at shutdown.: '!mTable', file c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\content\svg\content\src\nsSVGAttrTearoffTable.h, line 28 

My guess is it's happening somewhere around the following line:

  nsRefPtr<SVGAnimatedEnumeration> units = clipContent->ClipPathUnits();

Looking into the definition of DOMAnimatedEnum it looks like we have two definitions of the dtor:

  http://dxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGEnum.h#78
  http://dxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGEnum.cpp#126

So I'm guessing the original dtor isn't being called and we're failing to remove this object from the tear-off table. Robert, looks like this was introduced in bug 821960.

Comment 40

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

(In reply to Brian Birtles (:birtles) from comment #39)
> So I'm guessing the original dtor isn't being called and we're failing to
> remove this object from the tear-off table. Robert, looks like this was
> introduced in bug 821960.

Hmm... no, that's not it. I thought there was a definition of the dtor in the header but that's not it.

Comment 41

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

(In reply to Brian Birtles (:birtles) from comment #39)
> Hi Tuskihashi-san, it seems like there are quite a few test failures in that
> try run.

Sorry Tsukihashi-san. What I meant to say was that we need to investigate why test_getBBox-method.html is failing.

Comment 42

[Shigeyuki Tsukihashi]

(In reply to Brian Birtles (:birtles) from comment #41)
> Sorry Tsukihashi-san. What I meant to say was that we need to investigate
> why test_getBBox-method.html is failing.

Brian-san, Thank you for comments.
I found the following message in the results of Android and B2G.
> text1.getBBox().height - got 27.000001907348633, expected 20 (within 4)

I think this is the cause Probably.
I will modify test_getBBox-method.html and try again.

Comment 43

[Shigeyuki Tsukihashi]

Attachment: mochitest for Bug 999964. (2014-05-14)

Comment 44

[Shigeyuki Tsukihashi]

(In reply to Brian Birtles (:birtles) from comment #41)
> Sorry Tsukihashi-san. What I meant to say was that we need to investigate
> why test_getBBox-method.html is failing.

Hi Brian-san, I think that no error occurred by test_getBBox-method.html this time.
Here is the try server run: 
  https://tbpl.mozilla.org/?tree=Try&rev=58b7cf00d7aa

Attachment 8422101 [details] [diff] includes the new test_getBBox-method.html.

Comment 45

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

(In reply to Shigeyuki Tsukihashi from comment #44)
> Hi Brian-san, I think that no error occurred by test_getBBox-method.html
> this time.
> Here is the try server run: 
>   https://tbpl.mozilla.org/?tree=Try&rev=58b7cf00d7aa

This try run still has a lot of test failures due to bug 1004309 which was backed out. Can you update your tree and push to try again?

> Attachment 8422101 [details] [diff] includes the new
> test_getBBox-method.html.

The additional error here is too large. The clipped vs non-clipped versions only vary by about 6 user units but the error allowed is 8. That suggests these tests are useless. I suppose the variation is due to different platform fonts being used.

We could try using a specified font for this (we use Ahem.ttf in some mochitests). Alternatively, we could change the test to assert that the bbox when clipped is smaller than the bbox when not clipped.

Comment 46

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

(In reply to Brian Birtles (:birtles) from comment #45)
> (In reply to Shigeyuki Tsukihashi from comment #44)
> > Hi Brian-san, I think that no error occurred by test_getBBox-method.html
> > this time.
> > Here is the try server run: 
> >   https://tbpl.mozilla.org/?tree=Try&rev=58b7cf00d7aa
> 
> This try run still has a lot of test failures due to bug 1004309 which was
> backed out. Can you update your tree and push to try again?

Also, if you do your development against mozilla-central (not mozilla-inbound) you will face less problems like this since mozilla-central is more stable.

Comment 47

[Shigeyuki Tsukihashi]

(In reply to Brian Birtles (:birtles) from comment #45)
> This try run still has a lot of test failures due to bug 1004309 which was
> backed out. Can you update your tree and push to try again?
> 
> The additional error here is too large. The clipped vs non-clipped versions
> only vary by about 6 user units but the error allowed is 8. That suggests
> these tests are useless. I suppose the variation is due to different
> platform fonts being used.
> 
> We could try using a specified font for this (we use Ahem.ttf in some
> mochitests). Alternatively, we could change the test to assert that the bbox
> when clipped is smaller than the bbox when not clipped.

Thank you for comennts.
I understand and I will push to try again.

Comment 48

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

(In reply to Shigeyuki Tsukihashi from comment #47)
> Thank you for comennts.
> I understand and I will push to try again.

Thanks Tsukihashi-san! Please fix test_getBBox-method.html first too.

Comment 49

[Shigeyuki Tsukihashi]

(In reply to Brian Birtles (:birtles) from comment #48)
> (In reply to Shigeyuki Tsukihashi from comment #47)
> > Thank you for comennts.
> > I understand and I will push to try again.
> 
> Thanks Tsukihashi-san! Please fix test_getBBox-method.html first too.

I understand. I will try some fonts.
Thanks.

Comment 50

[Shigeyuki Tsukihashi]

Attachment: mochitest for Bug 999964. (2014-05-15)

I modified test_getBBox-method.html referring to test_bbox.xhtml.
Here is the try server run: 
  https://tbpl.mozilla.org/?tree=Try&rev=960f11bfcc79

Comment 51

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

(In reply to Shigeyuki Tsukihashi from comment #50)
> Created attachment 8423601 [details] [diff] [review]
> mochitest for Bug 999964. (2014-05-15)
> 
> I modified test_getBBox-method.html referring to test_bbox.xhtml.
> Here is the try server run: 
>   https://tbpl.mozilla.org/?tree=Try&rev=960f11bfcc79

Looks pretty good! The test_video_crossorigin.html failures appear to be bug 934814. Any idea what the test_integers.html failures are? They don't seem to be related.

Otherwise, it looks ready to land.

Comment 52

[Shigeyuki Tsukihashi]

(In reply to Brian Birtles (:birtles) from comment #51)
> Looks pretty good! The test_video_crossorigin.html failures appear to be bug
> 934814. Any idea what the test_integers.html failures are? They don't seem
> to be related.

Hi Brian-san, I'm sorry for my late response.

Previous time (https://tbpl.mozilla.org/?tree=Try&rev=960f11bfcc79), I used the patch for bug 1004309.
I think that failures of test_video_crossorigin.html and test_integers.html are not related to the patch for Bug 999964. This time, I executed try server run with the patch for bug 1004309 only.
Without the patch for Bug 999964, test_integers.html and test_video_crossorigin.html failed.
  https://tbpl.mozilla.org/?tree=Try&rev=52bbcc9a7a60 

Can I add the "checkin-needed" keyword ?
Thanks.

Comment 53

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

(In reply to Shigeyuki Tsukihashi from comment #52)
> (In reply to Brian Birtles (:birtles) from comment #51)
> > Looks pretty good! The test_video_crossorigin.html failures appear to be bug
> > 934814. Any idea what the test_integers.html failures are? They don't seem
> > to be related.
> 
> Hi Brian-san, I'm sorry for my late response.
> 
> Previous time (https://tbpl.mozilla.org/?tree=Try&rev=960f11bfcc79), I used
> the patch for bug 1004309.
> I think that failures of test_video_crossorigin.html and test_integers.html
> are not related to the patch for Bug 999964. This time, I executed try
> server run with the patch for bug 1004309 only.
> Without the patch for Bug 999964, test_integers.html and
> test_video_crossorigin.html failed.
>   https://tbpl.mozilla.org/?tree=Try&rev=52bbcc9a7a60 
> 
> Can I add the "checkin-needed" keyword ?

Looks good. Yes please! Thank you!

Comment 54

[Shigeyuki Tsukihashi]

(In reply to Brian Birtles (:birtles) from comment #53)
> Looks good. Yes please! Thank you!

I appreciate your help.
Thank you so much.

Comment 55

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

Comment on attachment 8421511 [details] [diff] [review]
Patch for SVG 2 getBBox method. (2014-05-13)

Boris, can you please review the WebIDL changes in this patch.
Spec link: https://svgwg.org/svg2-draft/types.html#SVGBoundingBoxOptions

Comment 56

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

Clearing checkin-needed. This needs review from a DOM peer before it can land.

Comment 57

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8421511 [details] [diff] [review]
Patch for SVG 2 getBBox method. (2014-05-13)

r=me on the webidl bits

Comment 58

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

Thanks Boris!

https://hg.mozilla.org/integration/mozilla-inbound/rev/e56e0f49a088
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcbcf7497627

Comment 59

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e56e0f49a088
https://hg.mozilla.org/mozilla-central/rev/dcbcf7497627

Comment 60

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

This has currently been disabled in Nightly due to bug 1019326.