1145195 - SVGFragmentIdentifier::ProcessSVGViewSpec() shouldn't actually let #svgView() affect attribute values

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

In our current svgView() implementation (from bug 512525), we make the overriding viewBox/preserveAspectRatio/etc. values take effect by actually *setting* them as the root element's attributes. So e.g. getAttribute behavior changes to reflect the override value.  This attribute-setting happens here in SVGFragmentIdentifier::ProcessSVGViewSpec():
http://mxr.mozilla.org/mozilla-central/source/dom/svg/SVGFragmentIdentifier.cpp?rev=c3f8fae88426#126

However, Chrome/Blink & Opera12.16/Presto don't seem to do this -- they make the overriding values affect *rendering*, but they don't impact e.g. getAttribute() on the root SVG element.

(The spec is vague on this, but I think it more closely matches what Chrome & Opera do than what we do -- enough to provide us justification for changing, at least. Specifically, the bulleted list at the end of http://www.w3.org/TR/SVG11/linking.html#SVGFragmentIdentifiers is says that <view> attributes "override" those on the SVG element, and that with svgView(), the document is "displayed ... using the svg view specification provided by the SVG fragment identifier." I take 'override' and 'displayed' here to mean that we don't actually set the attributes -- we just apply them for rendering purposes.)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: (SVG used in testcase; don't load directly. Use link in comment 2.)

Comment 2

[Daniel Holbert [:dholbert]]

Testcase (which alerts to indicate what getAttribute('viewBox') says, shortly after loading):
https://bug1145195.bugzilla.mozilla.org/attachment.cgi?id=8580091#svgView(viewBox(50,50,100,100))

In Opera & Chrome, the alert says "getAttribute(viewBox) returns: '9999 9999 200 200'"
(That's the underlying value -- so they're not modifying the attribute.)

In Firefox, the alert says "getAttribute(viewBox) returns: '50 50 100 100'
(That's the value provided in the URL, inside of #svgView().)

Comment 3

[Jonathan Watt [:jwatt]]

Spec text: https://svgwg.org/svg2-draft/linking.html#SVGFragmentIdentifiers

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> Testcase (which alerts to indicate what getAttribute('viewBox') says,
> shortly after loading):
> https://bug1145195.bugzilla.mozilla.org/attachment.cgi?id=8580091#svgView(viewBox(50,50,100,100))

Grr, bugzilla mis-linkified that URL. (It didn't include the last few parens in the linkification.)

To test this bug, you'll have to manually copy-paste, or just click on the "URL" field in this bug [which has the full test URL].

Comment 5

[Daniel Holbert [:dholbert]]

We need to make sure http://hoffmann.bplaced.net/svgtest/index.php?in=attributes#tr_view and http://hoffmann.bplaced.net/svgtest/viewviewbox01.svg#view1 continue to animate, after this bug is fixed.

We've got reftests that should ensure that, but worth doing a manual check on those too though, just in case.

Comment 6

[Robert Longson [:longsonr]]

Attachment: fix

Comment 7

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=88ef09c33f7e

Comment 8

[Robert Longson [:longsonr]]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> We need to make sure
> http://hoffmann.bplaced.net/svgtest/index.php?in=attributes#tr_view and
> http://hoffmann.bplaced.net/svgtest/viewviewbox01.svg#view1 continue to
> animate, after this bug is fixed.
> 
> We've got reftests that should ensure that, but worth doing a manual check
> on those too though, just in case.

Those animations still work with this patch.

Comment 9

[Daniel Holbert [:dholbert]]

Looks like a good fraction of the changes in this patch are really just about moving SVGFragmentIdentifier into the mozilla::dom namespace.  It'd be unfortunate to mix those in with the actual functional changes in the commit here -- that makes it easier for bugs to get overlooked, & it makes the review somewhat harder.

Would you be up for reposting the patch split into two pieces, with the namespace-related changes as "part 1" and the functional changes as "part 2"?

This should be doable by the following (from memory; untested but makes sense in my head):
   * qpushing the main patch here, with mercurial queues.
   * manually reverting all changes except for the namespace changes (e.g. using a merge tool)
   *  hg diff > ~/revertme-for-part2.patch
   *  hg qref

This should leave you with an applied MQ patch which is only the namespace changes, which would be your "part 1".  At that point, you can do:
   * patch -p1 -R < ~/revertme-for-part2.patch
   * hg qnew part2.patch
...which should give you a "part 2" that contains all the stuff that you reverted. (all the functional changes)

Comment 10

[Daniel Holbert [:dholbert]]

Also, stepping back: Are you sure this stuff wants to move to mozilla::dom?

I don't actually recall why some SVG things live in mozilla:: and others live in mozilla::dom, offhand.  From skimming across a few files, it looks like:
 - mozilla::dom:: is for types that are actually exposed via the DOM (e.g. SVGDocument, SVG elements, dom wrappers around types)
 - mozilla:: is for types that are only used internally (e.g. SVGPointList, SVGTransformList, SVGNumberList, etc.)

SVGFragmentIdentifier seems to belong to the latter category, which is probably why it's outside of the dom namespace in the first place.  So, I don't see a clear reason to bring it into the dom namespace.

Comment 11

[Robert Longson [:longsonr]]

Attachment: leave SVGFragmentIdentifier in the mozilla namespace

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 8691766 [details] [diff] [review]
leave SVGFragmentIdentifier in the mozilla namespace

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

::: dom/svg/SVGFragmentIdentifier.cpp
@@ +61,5 @@
>  
>    if (!tokenizer.hasMoreTokens()) {
>      return false;
>    }
> +  root->mSVGView = new SVGView();

It's a bit odd that this function sometimes modifies |root| ("sometimes" because it doesn't, if it takes the first early-return), and will likely end up leaving |root| with a partially-populated bogus mSVGView on failure (which the caller has to clean up for us).

Let's change this function as follows:
 (1) Work with a SVGView that's stored in a *local* nsAutoPtr variable, for brevity & clarity.
        So, e.g. s/root->mSVGView/newView/ throughout this function.

 (2) Make this function (ProcessSVGViewSpec) return an already_AddRefed<SVGView> instead of a bool.

 (3) Make all of the failure cases just "return nullptr" and the success case be "return newView.forget()"

That way, all of the checks on & modifications to root->mSVGView->mSomething will be more-clearly modifying a piece of local state that we're in the process of building up.  And our caller (further down in this file) can just do:
  rootElement->mSVGView = ProcessSVGViewSpec(...);

@@ +165,2 @@
>    if (wasOverridden) {
>      rootElement->InvalidateTransformNotifyFrame();

So, this section (around the ProcessSVGViewSpec callsite) would end up looking something like:

  rootElement->mSVGView = ProcessSVGViewSpec(...);
  if (!rootElement->mSVGView && wasOverridden) {
    rootElement->InvalidateTransformNotifyFrame();
  }

  return !!rootElement->mSVGView;

::: dom/svg/SVGSVGElement.cpp
@@ +965,5 @@
> +  // any transformations from the |transform| attribute. So since we're
> +  // PRE-multiplying, we need to apply the animateMotion transform *first*.
> +  if (mAnimateMotionTransform) {
> +    fromUserSpace.PreMultiply(ThebesMatrix(*mAnimateMotionTransform));
> +  }

Looks like this new code here is copypasted from the inherited version of this method (in SVGTransformableElement.cpp), because the inherited version applies mTransforms, but we may not want to do that here.

It seems unfortunate that we have to duplicate code here (and keep it in sync with the superclass method from now on, & know that we have to copypaste most changes from either method to the other one).  I have one idea for how to avoid this, though I'm not sure it's much better. In case you like it, though: I'm thinking we could just temporarily transfer svgView->mTransforms into |mTransforms|, *only* for the duration of the superclass call, and then put things back the way they were. Something like this (untested):

  // If we have an svgView() transforms attribute, temporarily swap it in
  // for our actual mTransforms, for our call to the superclass method:
  nsAutoPtr<nsSVGAnimatedTransformList> overriddenTransforms;
  bool didOverride = false;
  if (mSVGView && mSVGView->mTransforms) {
    overriddenTransforms = mTransforms.forget();
    mTransforms = mSVGView->mTransforms.forget();
  }

  gfxMatrix fromUserSpace =
    SVGSVGElementBase::PrependLocalTransformsTo(aMatrix, aWhich);	

  if (didOverride) {
    mSVGView->mTransforms = mTransforms.forget();
    mTransforms = overriddenTransforms.forget();
  }

(We could abstract this away into a custom autorestore-type RAII helper, too, if we wanted.)

I marginally prefer this over the code-duplication, but not enough to be sure about asking you to change it.  If you prefer what you've got (with the code duplication), please at least add a comment here, clearly saying that this must be kept in sync with the superclass method in SVGTransformableElement.

::: dom/svg/SVGSVGElement.h
@@ +90,5 @@
> +  friend class mozilla::dom::SVGSVGElement;
> +
> +private:
> +  enum { ZOOMANDPAN };
> +  nsSVGEnum mEnumAttributes[1];

Can we just make this a single nsSVGEnum (e.g. mZoomAndPan), instead of an enum-indexed array?

The enum-type & the array-indexing just seems like needless complication, and it introduces a possibility for out-of-bounds array reads, if we make a mistake with our array index somewhere.  On SVGElement subclasses, it makes more sense because it lets us share code nicely (I think?), but we're not sharing any of that code here, IIUC.

(On the other hand, this enum/array setup does make your new SVGView codepath in SVGSVGElement::ZoomAndPan() nicely analogous to the SVGViewElement codepath there. But that's a pretty marginal benefit, and I don't think that benefit is worth the complexity & the array-indexing footgun.)

@@ +297,5 @@
>    // particular) have access.
>    void SetImageOverridePreserveAspectRatio(const SVGPreserveAspectRatio& aPAR);
>    void ClearImageOverridePreserveAspectRatio();
>  
> +  // Set/Clear properties to hold old version of preserveAspectRatio

Add a second line here saying:
  // when it's being overridden by an <image> element that we are inside of.

Or something like that.

(The comment in the current patch, just saying "to hold old version", is a bit vague - why is it old? why are we holding it?)

Comment 13

[Robert Longson [:longsonr]]

(In reply to Daniel Holbert [:dholbert] from comment #12)
> Comment on attachment 8691766 [details] [diff] [review]
> leave SVGFragmentIdentifier in the mozilla namespace
> 
> 
> Let's change this function as follows:
>  (1) Work with a SVGView that's stored in a *local* nsAutoPtr variable, for
> brevity & clarity.
>         So, e.g. s/root->mSVGView/newView/ throughout this function.
> 
>  (2) Make this function (ProcessSVGViewSpec) return an
> already_AddRefed<SVGView> instead of a bool.
> 
>  (3) Make all of the failure cases just "return nullptr" and the success
> case be "return newView.forget()"
> 
> That way, all of the checks on & modifications to root->mSVGView->mSomething
> will be more-clearly modifying a piece of local state that we're in the
> process of building up.  And our caller (further down in this file) can just
> do:
>   rootElement->mSVGView = ProcessSVGViewSpec(...);

I'm not totally convinced that plan will work. It is assigned to root so that the notifications that get triggered when we change baseValue work properly.

It works like this

assign mViewSpec to root.
set baseValue on mViewSpec which then causes a notification which calls back into the nsSVGSVGElement and which expects things like SVGSVGElement::GetViewBoxWithSynthesis to return the modified value so that sizing works properly.


> It seems unfortunate that we have to duplicate code here (and keep it in
> sync with the superclass method from now on, & know that we have to
> copypaste most changes from either method to the other one).  I have one
> idea for how to avoid this, though I'm not sure it's much better. In case
> you like it, though: I'm thinking we could just temporarily transfer
> svgView->mTransforms into |mTransforms|, *only* for the duration of the
> superclass call, and then put things back the way they were. Something like
> this (untested):
> 
>   // If we have an svgView() transforms attribute, temporarily swap it in
>   // for our actual mTransforms, for our call to the superclass method:
>   nsAutoPtr<nsSVGAnimatedTransformList> overriddenTransforms;
>   bool didOverride = false;
>   if (mSVGView && mSVGView->mTransforms) {
>     overriddenTransforms = mTransforms.forget();
>     mTransforms = mSVGView->mTransforms.forget();
>   }
> 
>   gfxMatrix fromUserSpace =
>     SVGSVGElementBase::PrependLocalTransformsTo(aMatrix, aWhich);	
> 
>   if (didOverride) {
>     mSVGView->mTransforms = mTransforms.forget();
>     mTransforms = overriddenTransforms.forget();
>   }
> 
> (We could abstract this away into a custom autorestore-type RAII helper,
> too, if we wanted.)

Or maybe we could have an extra override matrix on the base class PrependLocalTransformsTo. We pass in the mSVGView->mTransforms consolidation matrix in this case and in other cases we pass in nullptr which means use mTransforms consolidation matrix. Does that sound better?

I'll sort the rest of the comments.

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to Robert Longson from comment #13)
> I'm not totally convinced that plan will work. It is assigned to root so
> that the notifications that get triggered when we change baseValue work
> properly.

OK, I suppose the current setup makes sense then.

Two thoughts:
 (1) Is it OK that the root->mSVGView = ... assignment won't trigger any notifications? (Do we need it to?)

 (2) Please add a comment alongside that assignment, indicating that we're doing it proactively (before parsing anything) so that notifications will work correctly when we parse & set values.

> Or maybe we could have an extra override matrix on the base class
> PrependLocalTransformsTo. We pass in the mSVGView->mTransforms consolidation
> matrix in this case and in other cases we pass in nullptr which means use
> mTransforms consolidation matrix. Does that sound better?

That sounds probably-better than my first suggestion, yeah. Though, it's unfortunate that we have to pollute all PrependLocalTransformsTo function-signatures, just so this one class can customize it.

Yet another option:
 - Leave the PrependLocalTransformsTo function-signature unmodified
 - Move the base-clsas code into a helper function which accepts an extra arg for the transforms attribute.
 - Make the base-class PrependLocalTransformsTo function call this helper-function & pass in mTransforms.
 - Make the SVGSVGElement version call this helper-function & pass in either mTransforms or svgView->mTransforms depending on svgView status.

I'd be happy with either this or with your suggestion, I think.

Comment 15

[Robert Longson [:longsonr]]

Attachment: viewspec.txt

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8693336 [details] [diff] [review]
viewspec.txt

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

Catching up on reviews; punting on this one for the moment, because (with the refactoring and includes noted below) it's doing a lot all at once.  Could you split it into two patches, as noted below?

::: dom/svg/SVGContentUtils.cpp
@@ +884,5 @@
> +  // PRE-multiplying, we need to apply the animateMotion transform *first*.
> +  if (aAnimateMotionTransform) {
> +    result.PreMultiply(ThebesMatrix(*aAnimateMotionTransform));
> +  }
> +  

whitespace on blank line

::: dom/svg/SVGTransformableElement.cpp
@@ +95,3 @@
>  {
> +   return SVGContentUtils::PrependLocalTransformsTo(
> +     aMatrix, aWhich, mAnimateMotionTransform, mTransforms); 

Whitespace at end-of-line here.

Also, could you split the existing-code-refactoring parts (this change & related changes) into its own patch, separate from the actual functional change here?

::: dom/svg/nsSVGPathGeometryElement.cpp
@@ -10,5 @@
>  #include "mozilla/gfx/2D.h"
>  #include "nsComputedDOMStyle.h"
>  #include "nsSVGUtils.h"
>  #include "nsSVGLength2.h"
> -#include "SVGContentUtils.h"

Can we split these #include-removals off, too (same patch as the code-refactoring is fine by me), so they don't obscure the main code-change here?

Comment 17

[Robert Longson [:longsonr]]

Attachment: just the non-functional changes

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 8693918 [details] [diff] [review]
just the non-functional changes

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

Thanks for splitting these non-functional changes out -- makes it much easier to see what's going on & why.

I think this looks good, from a high level perspective, though I think a lot of the churn here is probably avoidable & undesirable -- details below:

::: dom/svg/SVGContentUtils.h
@@ +354,5 @@
> +  enum TransformTypes {
> +     eAllTransforms
> +    ,eUserSpaceToParent
> +    ,eChildToUserSpace
> +  };

(1) Please add a space after the commas here.

(2) It looks like this is meant to replace the same enum in nsSVGElement.h (this is copypasted), but the enum still exists in the old location -- can we get rid of the old enum definition from nsSVGElement.h now?  (I don't think that one's used anymore.)

(3) It looks like these enum values are still documented in nsSVGElement.h (alongside PrependLocalTransformsTo). Probably worth adding a comment here, referencing that documentation to help people discover what these enum-values actually mean.

(4) Could we declare this type at the top of this file, outside of the SVGContentUtils {...} class definition?  (Maybe just put it in namespace mozilla {}?)  I think that would mean we can do away with all of the renames in this patch of the form of:
  s/TransformTypes/SVGContentUtils::TransformTypes/
  s/eUserSpaceToParent/SVGContentUtils::eUserSpaceToParent/
which seem to make up a lot of the churn in this patch, and which leave a lot of lines awkwardly-wrapped and/or awkwardly-long.

On balance, I don't think the SVGContentUtils:: scoping for the enum here is worth the awkwardness & churn that it causes in all of the implementations & callers...  (If we make the type un-qualified -- just mozilla-namespaced -- I think that'd mean we could avoid needing most of this patch's changes in dom/svg entirely.)

If you're worried about "TransformTypes" being too generic of a name, perhaps we could call this "mozilla::SVGTransformTypes"?  (Then we'd still have to do a bit of renaming, but we'd only be making the type-name 3 characters longer, and we wouldn't have to rename usages of the enum *values* in /dom at all.)

@@ +356,5 @@
> +    ,eUserSpaceToParent
> +    ,eChildToUserSpace
> +  };
> +  /**
> +   *  Prepends an element's local transforms to the transform matrix.

Probably worth adding a disclaimer here that this is really as a helper for nsSVGElement::PrependLocalTransformsTo implementations. (and that any callers probably really want to call that method instead of this one)

::: dom/svg/SVGMarkerElement.cpp
@@ -15,5 @@
>  #include "mozilla/dom/SVGMarkerElementBinding.h"
>  #include "mozilla/Preferences.h"
>  #include "mozilla/gfx/Matrix.h"
>  #include "mozilla/FloatingPoint.h"
> -#include "SVGContentUtils.h"

I'd lean against all of the SVGContentUtils.h #include-removals in this patch.

True, these #includes are technically unnecessary, because we get this header from a superclass #include.  But, it's still nice to be explicit about #including headers for APIs that we're explicitly using.  So -- unless there's a risk of a circular #include dependency somehow, I think it's worth erring on the side of being explicit about including headers for APIs that we use in a particular file.  So, I'd rather not take these #include removals (unless there are any cases where SVGContentUtils APIs are really not used at all in a given file -- but I don't think that's the case for these).

Comment 19

[Robert Longson [:longsonr]]

Attachment: non-functional changes review comments

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 8694413 [details] [diff] [review]
non-functional changes review comments

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

Looks good, thanks!

2 nits:

::: dom/svg/SVGFragmentIdentifier.cpp
@@ +31,5 @@
>  {
>    return false;
>  }
>  
> +static SVGViewElement*

Optional nit: I think the changes to this file (tweaking usage of the "dom::" namespace) are unrelated & orthogonal to the rest of this patch -- might make sense to split it out into another patch, but not a big deal if you want to leave it all together too.

(It makes this a bit more of a mishmash of different things; but I suppose that's not too bad for a cleanup/nonfunctional-refactoring-in-place patch.)

You have preemptive r=me on this additional helper-patch, if you'd like to spin it off.

::: layout/svg/nsSVGPatternFrame.cpp
@@ +406,5 @@
>        }
>        gfxMatrix tm = *(patternWithChildren->mCTM);
>        if (kid->GetContent()->IsSVGElement()) {
>          tm = static_cast<nsSVGElement*>(kid->GetContent())->
> +              PrependLocalTransformsTo(tm, eUserSpaceToParent);

While you're here, please add 1 more space of indent to this line, for consistency with other spots in this patch.  (Right now this call is indented by 1 space w.r.t. the object it's being called on, which is odd/awkward; usually we do 2-space indent.)

Comment 21

[Daniel Holbert [:dholbert]]

Comment on attachment 8693336 [details] [diff] [review]
viewspec.txt

Marking the original mega-patch r-.

I'll gladly review a new patch with the original functional changes on their own, though. (Thanks for being accommodating about splitting this up!)

Comment 22

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a41fb86ed5d

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dee213acd662

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dee213acd662

Comment 25

[Robert Longson [:longsonr]]

Attachment: part 2 functional change

Comment 26

[Robert Longson [:longsonr]]

Attachment: part 2 with test change

Comment 27

[Daniel Holbert [:dholbert]]

Comment on attachment 8695795 [details] [diff] [review]
part 2 with test change

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

Thanks! Review notes below -- this is basically r+ with the following addressed, but since the suggestions aren't quite trivial, I think this merits one more round of review on the final version -- so I'll mark this version r- for the moment. Nearly there, though.

::: dom/svg/SVGFragmentIdentifier.cpp
@@ +65,5 @@
> +  // We set mSVGView on the root element here because the assignments below
> +  // via SetBaseValueString will cause callbacks on the root element and
> +  // they expect to find updated values on the root element.
> +  // If something goes wrong the caller will return root->mSVGView to nullptr. 
> +  root->mSVGView = new SVGView();

Nit:
 - trailing whitespace at the end of the comment here.

Also, it still feels fragile that we can set up some broken data on the root element here, and then we have to depend on our caller to clean it up for us.  Feels like that should be more cleanly encapsulated.  We can't do it quite as cleanly as I was hoping for in comment 12, per your code-comment here, but we could still do better I think.

How about we just clean up after ourselves (nulling out root->mSVGView) at the end of this function, by:
 - adding a local variable "parsedSuccessfully" which is initially true
 - convert all of our "return false" statements in the do-while loop here to:
     parsedSuccessfully = false;
     break;
  - After this do-while loop, replace the "return true" with:
     if (!parsedSuccessfully) {
       root->mSVGView = nullptr;
     }
     return parsedSuccessfully;

That'll makes this function a little more complex, but this complexity is well worth it, IMO, to avoid having to spread the cleanup-code out to the caller for something that we might mess up internally here.

::: dom/svg/SVGSVGElement.cpp
@@ +951,5 @@
>    // 'transform' attribute:
>    gfxMatrix fromUserSpace =
> +    SVGContentUtils::PrependLocalTransformsTo(
> +      aMatrix, aWhich, mAnimateMotionTransform,
> +      mSVGView && mSVGView->mTransforms ? mSVGView->mTransforms : mTransforms); 

Drop end-of-line whitespace here.

Also: Please adjust the comment here to cover this new code, e.g.:
 // 'transform' attribute (or an override from an svgView()):

::: dom/svg/SVGSVGElement.h
@@ +84,5 @@
>    float width;
>    float height;
>  };
>  
> +class SVGView {

Please add a brief comment describing this class.

::: dom/svg/test/test_fragments.html
@@ -77,5 @@
> -    is(doc.rootElement.getAttribute("preserveAspectRatio"),
> -       test.preserveAspectRatioString, "unexpected preserveAspectRatio");
> -
> -    is(doc.rootElement.getAttribute("zoomAndPan"),
> -       test.zoomAndPanString, "unexpected zoomAndPan");

Could you add a test for the new behavior here?  (to replace these removed tests for the old behavior)

Comment 28

[Robert Longson [:longsonr]]

> Could you add a test for the new behavior here?  (to replace these removed
> tests for the old behavior)

How? The new behaviour is not very detectable.

Comment 29

[Daniel Holbert [:dholbert]]

Instead of removing all of these is(doc.rootElement.getAttribute("preserveAspectRatio"), ...) statements, can't you just update them to have the new expected value?  (which I think is just the values we've given in http://mxr.mozilla.org/mozilla-central/source/dom/svg/test/fragments-helper.svg, and maybe the empty string in some cases.)

Currently, the test is asserting that these attributes do change. But I think now, we want to be asserting that these attributes *don't* change.

Comment 30

[Daniel Holbert [:dholbert]]

Actually, it'd be the empty string in all cases, I guess, since fragments-helper.svg has none of these attributes set on its SVG element.

To really test this, it might be worth having this test run itself twice, with two different versions of fragments-helper.svg, one of which has these attributes unset on <svg> (as is the case currently), and one of which has all of these attributes set on its <svg>.  Say, fragments-helper-withAttrs.svg

Comment 31

[Robert Longson [:longsonr]]

Attachment: address review comments

Turns out you were right all along about everything and the extra tests you insisted on proved it :-)

The callbacks on the root element cause hasAttribute to be true (although the attribute value doesn't change, it returns the initial value). I've therefore had to prevent any callbacks, which means I don't set mSVGView on the root element till I know it's valid and then I have to force a refresh once the root element's mSVGView is set.

Comment 32

[Robert Longson [:longsonr]]

Attachment: address review comments

Comment 33

[Daniel Holbert [:dholbert]]

Comment on attachment 8696355 [details]
address review comments

Very nearly r+. Notes below, with only the very first point being substantial enough to hold up r+.

+++ b/dom/svg/SVGFragmentIdentifier.cpp
[...]
+  SVGView* svgView = new SVGView();
+  viewHandler.SetView(svgView);

I'm a bit uncomfortable with the code above -- we're allocating something using "new", and sticking the result in a raw local-variable pointer, and then sharing the pointer-value with something else.

This pattern smells like something that could cause subtle leaks or use-after-free bugs, either now or after some future tweaks.  So, it's worth avoiding, even though I think it's technically safe right now (after sanity-checking it manually). Also, one more reason to avoid this pattern: we're transitioning away from "new Whatever()" in favor of MakeUnique<Whatever> in cases like this, and MakeUnique only allows its result to be stored in a UniquePtr, not a raw pointer. (See e.g. bug 1220190 for some of these conversions.) I'm not suggesting that you use MakeUnique/UniquePtr now, because I think we probably want to convert files over in their entirety -- but I do want to avoid adding code that's *difficult to transition* to MakeUnique, and this falls into that category right now.

Good news, though -- I think we can structure this more cleanly & avoid this problem by doing the following:
 * Make |viewHandler| the unambiguous owner of the svgView object. It shouldn't even expose it publicly via an accessor.

 * Instantiate the SVGView object using a member-function on viewHandler, like so:
     viewHandler.CreateSVGView();
   This function should internally assert that mSVGView is null (to prevent accidental double-allocation), and then should do "mSVGView = new SVGView();" (where mSVGView is a nsAutoPtr, as you have it right now)

 * And to avoid handing out raw copies of this pointer, we should move the whole "if (IsMatchingParameter(...))" if/else cascade into a helper-method on viewHandler (called something like "ProcessAttr").

Then we can replace that whole if/else cascade (~40 lines) with just this one call:
 
  if (!viewHandler.ProcessAttr(token, params)) {
    return false;
  }

This makes the do{...}while loop much more compact, and lets us abstract the high level tokenization loop away from the low-level per-attribute-parsing/duplicate-attribute-rejecting/svgView-populating, which seems liie

Does that make sense?

+++ b/dom/svg/SVGSVGElement.h
[...]
+// Stores svgView arguments of SVG fragment identifiers.
+class SVGView {
+  friend class mozilla::SVGFragmentIdentifier;

I'm a bit confused by these two classes (SVGView & SVGFragmentIdentifier) -- they kind of represent the same thing. Seems like they might just want to be merged? (perhaps in a followup bug?)  One class is purely data-storage, and the other is purely static-methods to populate the other's data (relying on "friend" access).  Seems like there should just be a single class, perhaps with the exact same static functions (like "factory" functions, basically), whihc would return an instance of itself.

If you agree that these classes would benefit from being merged, could you file a followup on doing so?  (Or do that cleanup here if you like, though it's fine to punt on it IMO.)

>    nsAutoPtr<nsString>            mCurrentViewID;
> +  nsAutoPtr<SVGView>             mSVGView;

Could you add a comment indicating that at most one of these should be set (i.e. they're mutually exclusive), if that's the case?

(I think it is the case, from looking at how SVGFragmentIdentifier::ProcessFragmentIdentifier works.  But it's non-obvious from how these are used. We kind of act like one trumps the other -- always using the view element before the svgView spec -- but really they can't conflict, I think, since our #fragment can only specify one or the other.)

> +++ b/dom/svg/test/test_fragments.html
[...]
> +  for (var j = 0; j < 2; j++) {
> +    var initialViewBox = rootElement.getAttribute("viewBox");
> +    var initialPreserveAspectRatio =
> +      rootElement.getAttribute("preserveAspectRatio");
> +    var initialZoomAndPan = rootElement.getAttribute("zoomAndPan");

Please add a sanity-check ok()-test to verify that these properties are truly empty on our first time through this loop, to be sure we're actually testing this scenario as we intend to.  I think the following should work, though I haven't tested it:

    if (j == 0 && (initialViewBox != "" ||
                   initialPreserveAspectRatio != "" ||
                   initialZoomAndPan != "")) {
      ok(false, "Bug in helper SVG file -- needs to have attrs unset " +
                "so we can test that scenario.");
    }

This emptiness is worth verifying, since:
 (1) These attributes are specified in an entirely different file (fragments-helper.svg), which we might conceivably mess with down the line (e.g. add a viewBox) without realizing that we're reducing the effectiveness of this test.
  (2) We do dynamically mess with these attributes in this very mochitest, and it's good to be sure we're not accidentally setting them before we depend on them being empty here. (which could happen, in some reformulations of this test, depending on how the for-loops are nested)

Comment 34

[Robert Longson [:longsonr]]

Attachment: address review comments

> I'm a bit confused by these two classes (SVGView & SVGFragmentIdentifier) -- > they kind of represent the same thing. Seems like they might just want to be > merged? (perhaps in a followup bug?)

An SVGFragmentIdentifier can result in either the creation of an SVGView or view id so it makes sense to me to keep them separate.

Comment 35

[Daniel Holbert [:dholbert]]

(In reply to Robert Longson from comment #34)
> An SVGFragmentIdentifier can result in either the creation of an SVGView or
> view id so it makes sense to me to keep them separate.

Fair enough, ok.

Comment 36

[Daniel Holbert [:dholbert]]

Comment on attachment 8696578 [details] [diff] [review]
address review comments

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

Thanks, looks great! A few final nits below; r=me with these addressed.

Thanks for being so receptive to the multiple rounds of feedback & patch-split-requests here.  Much easier to review this stuff in smallish/focused pieces.

::: dom/svg/SVGFragmentIdentifier.cpp
@@ +40,5 @@
>              static_cast<SVGViewElement*>(element) : nullptr;
>  }
>  
> +// Handles setting/clearing the root's mSVGViewElement.
> +class MOZ_RAII AutoSVGViewHandler

s/mSVGViewElement/mSVGView pointer/, I think.

@@ +71,2 @@
>  
> +  bool ProcessAttr(const nsString& aToken, const nsAString &params) {

Might as well just make the first arg "const nsAString&", like the second one, since we're not doing anything nsString-specific.  (I think we generally default to using the abstract nsAString& for args, unless there's a reason not to.)

@@ +111,5 @@
> +    } else {
> +      // We don't support viewTarget currently
> +      return false;
> +    }
> +    return true;    

Drop whitespace at end of line here.

@@ +116,3 @@
>    }
> +  void SetValid() {
> +    mValid = true;

Add a blank line between the previous function & this one.

::: dom/svg/SVGSVGElement.h
@@ +375,5 @@
>    nsSVGViewBox                   mViewBox;
>    SVGAnimatedPreserveAspectRatio mPreserveAspectRatio;
>  
> +  // mCurrentViewID and mSVGView are mutually exclusive we can have
> +  // at most one non-null.

2 sentences smooshed together here -- please add semicolon after "exclusive".

Comment 37

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a0584dab5c8

Comment 38

[Robert Longson [:longsonr]]

SVGView needs a constructor.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=88a279423ce2

Comment 39

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/acbf0c453000

Comment 40

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/acbf0c453000