Closed Bug 999964 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: shigeyuki.tsukihashi, Assigned: shigeyuki.tsukihashi)

References

(Blocks 2 open bugs, )

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 13 obsolete files)

18.32 KB, patch
shigeyuki.tsukihashi
: review+
bzbarsky
: review+
Details | Diff | Splinter Review
34.83 KB, patch
shigeyuki.tsukihashi
: review+
Details | Diff | Splinter Review
Attached file Patch for SVG 2 getBBox method. (obsolete) —
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.
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #8410808 - Attachment is obsolete: true
Attachment #8410808 - Attachment is patch: false
(In reply to Brian Birtles (:birtles) from comment #1)
Hi Brian-san, thank you for comment. I will submit the modified patch.
Attached patch Patch for SVG 2 getBBox method. (obsolete) — Splinter Review
Hi Tsukihashi-san, is this patch ready for review?
Do you have any tests for this code?
Flags: needinfo?(shigeyuki.tsukihashi)
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.
(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.
Flags: needinfo?(shigeyuki.tsukihashi)
Attached patch mochitest for Bug 999964. (obsolete) — Splinter Review
Attachment #8414991 - Flags: review?(longsonr)
Attachment #8410824 - Flags: review?(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.
Attachment #8410824 - Flags: review?(longsonr) → review-
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.
Attachment #8414991 - Flags: review?(longsonr) → review-
(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.
> 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.
(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.
Attachment #8410824 - Attachment is obsolete: true
Attachment #8416331 - Flags: review?(longsonr)
Attachment #8414991 - Attachment is obsolete: true
Attachment #8416332 - Flags: review?(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.
Attachment #8416331 - Flags: review?(longsonr) → review+
Attachment #8416332 - Flags: review?(longsonr) → review+
(In reply to Robert Longson from comment #15)

Thanks for comment and advise, Robert.
I will modify the patch based on your comment.
Attachment #8416331 - Attachment is obsolete: true
Attachment #8416332 - Attachment is obsolete: true
Attachment #8419143 - Flags: review+
Attachment #8419144 - Flags: review+
(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 ?
Depends on: 997735
(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)
Assignee: nobody → shigeyuki.tsukihashi
Status: NEW → ASSIGNED
Flags: needinfo?(shigeyuki.tsukihashi)
(In reply to Brian Birtles (:birtles) from comment #20)
> On the following kinds of patches need super-review
  ^ "Only"
(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.
Flags: needinfo?(shigeyuki.tsukihashi)
(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.
(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.
Flags: needinfo?(shigeyuki.tsukihashi)
(In reply to Brian Birtles (:birtles) from comment #24)

Brian-san, Thank you for your comment.
I will modify the patch.
Flags: needinfo?(shigeyuki.tsukihashi)
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/.
(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.
Attachment #8419143 - Attachment is obsolete: true
Attachment #8419956 - Flags: review+
Attachment #8419144 - Attachment is obsolete: true
Attachment #8419958 - Flags: review+
Attachment #8419956 - Attachment is obsolete: true
Attachment #8420785 - Flags: review+
Attachment #8419958 - Attachment is obsolete: true
Attachment #8420786 - Flags: review+
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.
(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.
(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!
(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.
Attachment #8420785 - Attachment is obsolete: true
Attachment #8420786 - Attachment is obsolete: true
(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.
Flags: needinfo?(birtles)
(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.
Flags: needinfo?(birtles) → needinfo?(longsonr)
(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.
(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.
(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.
Attachment #8421513 - Attachment is obsolete: true
(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.
(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.
Flags: needinfo?(longsonr)
(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.
(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.
(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.
(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.
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
Attachment #8422101 - Attachment is obsolete: true
Flags: needinfo?(birtles)
(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.
Flags: needinfo?(birtles)
(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.
(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!
Attachment #8421511 - Flags: review+
Attachment #8423601 - Flags: review+
Keywords: checkin-needed
(In reply to Brian Birtles (:birtles) from comment #53)
> Looks good. Yes please! Thank you!

I appreciate your help.
Thank you so much.
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
Attachment #8421511 - Flags: review?(bzbarsky)
Clearing checkin-needed. This needs review from a DOM peer before it can land.
Keywords: checkin-needed
Comment on attachment 8421511 [details] [diff] [review]
Patch for SVG 2 getBBox method. (2014-05-13)

r=me on the webidl bits
Attachment #8421511 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/e56e0f49a088
https://hg.mozilla.org/mozilla-central/rev/dcbcf7497627
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Blocks: 1019326
This has currently been disabled in Nightly due to bug 1019326.
Depends on: 1189042
Blocks: svg2
You need to log in before you can comment on or make changes to this bug.