nsSVGPatternFrame::GetPatternWithAttr and its callers should take account of SMIL animation.

We have a bug with the way we convert lengths to percentages: bug 621651. Until that bug is fixed we can't properly test the pattern x,y,width,height attributes since they all default to 0% and cause us to run into that bug.

This patch basically fixes it. Things still to do: * Work out if there's a less hacky interim solution for detecting if a transform has been set (while we wait for bug 602759 to be fixed) * The x,y,width,height test cases currently fail due to bug 621651 -- need to see if there's any possible way of testing them without waiting for that bug. If not, perhaps we need to split the test case into the failing part and succeeding part and just mark the failing part as "fails".

Here's an initial attempt at fixing this. One question I'd appreciate some feedback on is the naming of these "IsAnimValSet" methods. That seems like a confusing name. What we need is a way of testing if an attribute is explicitly set whether that be by markup, a DOM call, or animation. For viewboxes we have IsValid which is fine for that case since a viewbox is either set or it's not and if it's not set, you shouldn't be trying to query its value. But for other types there's a default value so it's perfectly legitimate to query the attribute value even if it hasn't been explicitly set. That's why we currently have IsAnimValSet which means "if I use the anim value am I getting something that's been explicitly set one way or another?" I wonder if either of the following is any better: IsSet() IsExplicitlySet() The last one makes most sense to me. Is that ok?

Comment on attachment 500155 [details] [diff] [review] Patch v1a > > const SVGPreserveAspectRatio &GetBaseValue() const > { return mBaseVal; } > const SVGPreserveAspectRatio &GetAnimValue() const > { return mAnimVal; } > PRBool IsAnimated() const > { return mIsAnimated; } >+ PRBool IsAnimValSet() const >+ { return mIsAnimated || mIsBaseSet; } > IsSet would be my choice. I could live with IsExplicitlySet but I don't like IsAnimValSet at all ;-), please change to one or the other alternatives. > SVGPreserveAspectRatio mAnimVal; > SVGPreserveAspectRatio mBaseVal; >- PRPackedBool mIsAnimated; >+ PRPackedBool mIsAnimated:1; >+ PRPackedBool mIsBaseSet:1; You're not saving anything here. Default packing on all our platforms is 4 bytes so you can have up to 4 PRPackedBool variables before you'd compress them to individual bits. Please revert the mIsAnimated change and make msIsBaseSet just an ordinary PRPackedBool. > nsSVGEnumValue mAnimVal; > nsSVGEnumValue mBaseVal; > PRUint8 mAttrEnum; // element specified tracking for attribute >- PRPackedBool mIsAnimated; >+ PRPackedBool mIsAnimated:1; >+ PRPackedBool mIsBaseSet:1; same here, although you can only have 3 PRPackedBools since mAttrEnum takes one of the 4 bytes available in the block already. >--- /dev/null >+++ b/layout/reftests/svg/smil/anim-pattern-attr-presence-02.svg >@@ -0,0 +1,52 @@ >+<svg xmlns="http://www.w3.org/2000/svg" >+ xmlns:xlink="http://www.w3.org/1999/xlink" viewBox="0 0 200 200"> >+ <!-- Bug 544809 - nsSVGPatternFrame::GetPatternWithAttr and callers should >+ take account of SMIL animation. >+ >+ This test is a continuation of anim-pattern-attr-presence-01.svg but is >+ separated because it currently fails due to bug 621651. Once that bug is >+ resolved the tests in this file should be merged into >+ anim-pattern-attr-presence-01.svg >+ --> Perhaps you should just merge it all now but comment out all the elements with <!-- --> rather than leaving this as an exercise for the implementor of bug 621651. > nsSVGPatternElement * > nsSVGPatternFrame::GetPatternWithAttr(nsIAtom *aAttrName, nsIContent *aDefault) > { I think splitting this into GetEnumValue, GetTransformValue, GetLengthValue etc and passing the index for enums and lengths (nsSVGPatternElement::PATTERNUNITS etc) would be simpler wouldn't it? You could then return the animated value directly from these methods rather than just returning a pointer to the patternElement.

Comment on attachment 500155 [details] [diff] [review] Patch v1a >+ PRBool IsAnimValSet() const >+ { return mIsAnimated || mIsBaseSet; } I like both of your suggestions (IsSet() & IsExplicitlySet()) better than this one. (I lean slightly towards IsExplicitlySet, since many attributes are "set" by default but what we're wondering here is whether the author has explicitly set them somehow. I don't have strong feelings though.) > Perhaps you should just merge it all now but comment out all the elements with > <!-- --> rather than > leaving this as an exercise for the implementor of bug 621651. I think Brian's intention was that the implementor of bug 621651 needs only to remove the "fails" annotation from reftest.list -- and then *after* that point, the two tests can be merged (by anyone, at their leisure - not a high-priority item). The current split-up format has the benefit that it'll turn orange with UNEXPECTED-PASS when bug 621651 is fixed, and that will remind us to enable the extra tests and start testing those things.

(In reply to comment #5) > The current split-up format has the benefit that it'll turn orange with > UNEXPECTED-PASS when bug 621651 is fixed, and that will remind us to enable the > extra tests and start testing those things. OK.

This patch removes the bitfield packing and renames IsAnimValSet to IsExplicitlySet. Patch to address final comment regarding GetEnumVal etc. forthcoming.

(In reply to comment #4) > I think splitting this into GetEnumValue, GetTransformValue, GetLengthValue etc > and passing the index for enums and lengths (nsSVGPatternElement::PATTERNUNITS > etc) would be simpler wouldn't it? You could then return the animated value > directly from these methods rather than just returning a pointer to the > patternElement. What's the concern here? Is it just simplification or is it the fact that we end up looking up the values twice? If it's just the former then this patch may remedy that. The difficulty is that I really don't want to duplicate the second half of GetPatternWithAttr where we do the recursive lookup and detect loops and the like. That sort of code is error-prone and I'd rather it only appeared once. This patch reuses that code by using templates and functors. An alternative which wouldn't really simplify the code but might make it easier to read is just to take the first half of GetPatternWithAttr from the original patch and just stick it in its own method (extract method) such as IsAttrSet(nsSVGPatternElement* aElem, nsIAtom *aAttrName)

Sorry, I just noticed I've reordered some of the methods in that second patch (Possible refactoring of GetPatternWithAttr). I should fix that but I'll wait to hear first what you think of this approach before fiddling any more.

I've reordered the newly added methods to match their equivalent methods in the original file. Hopefully this makes it a little easier to see what's being changed.

The concern is the big switching "if" test which has to treat each type differently. Your template/helper solution does fix that but it seems complicated too. An AutoPatternReferencer class (c.f. AutoMaskReferencer) can hide the flag code. For Length's in my worldview, you'd end up with something like this... const nsSVGLength2 * nsSVGPatternFrame::GetLengthValue(PRUint32 aIndex, nsIContent *aDefault) { nsSVGPatternElement* patternElement = static_cast<nsSVGPatternElement *>(mContent); if (patternElement->mLengthAttributes[aIndex].IsExplicitlySet()) return &mContent->mLengthAttributes[aIndex]; AutoPatternReferencer patternRef(this); nsSVGPatternFrame *next = GetReferencedPattern(); return next && !next->mInUse ? next->GetLengthValue(aIndex, aDefault) : &static_cast<nsSVGPatternElement *>(aDefault)->mLengthAttributes[aIndex]; } This seems simpler to me than the helper classes you're proposing. Still not convinced?

(In reply to comment #11) > An AutoPatternReferencer class (c.f. AutoMaskReferencer) can hide the flag > code. Ok, can do. I've tried to match the previous behaviour with regarding to reporting warnings and the like so it differs a little from AutoMaskReferencer.

Comment on attachment 502715 [details] [diff] [review] Part 2 - Refactor GetPatternWithAttr v1b > PRUint16 >-nsSVGPatternFrame::GetPatternUnits() >+nsSVGPatternFrame::GetEnumValue(PRUint32 aIndex) > { >- // See if we need to get the value from another pattern >- nsSVGPatternElement *patternElement = >- GetPatternWithAttr(nsGkAtoms::patternUnits, mContent); >- return patternElement->mEnumAttributes[nsSVGPatternElement::PATTERNUNITS].GetAnimValue(); >+ return GetEnumValue(aIndex, mContent); > } Please make this one line method inline in nsSVGPatternFrame.h and do the same with the other one line methods for the other types. Otherwise looks great.

(In reply to comment #13) > Please make this one line method inline in nsSVGPatternFrame.h and do the same > with the other one line methods for the other types. Fixed.

Requesting approval to land. Fixes broken behaviour. It's not critical but fixes some bugs that will no doubt frustrate any web developers who stumble across them. Includes tests. Bug 608161 will probably build on the work done here. Incidentally, I'll also probably push this roll-up patch rather than the two individual patches since the second patch mostly just fixes problems with the first.

I'm getting a timeout on Try for OSX debug reftests with the test case that goes with this bug: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1294837728.1294839744.9545.gz&fulltext=1#err0 The timeout is with anim-pattern-attr-presence-01-ref.svg These test files give me *incredibly* slow performance on my local machine (e.g. 10 seconds for a repaint). See bug 465087. I'm going to guess that the machine in question was a little bogged down and so timed out. I'll try another push later and see if I can reproduce it. Otherwise the solution might just be to split the tests up into several files (or fix bug 465087--no idea why we're so slow at painting patterns).

(In reply to comment #16) > I'm getting a timeout on Try for OSX debug reftests Add to that OSX opt, OSX64 opt (but not OSX64 debug)

(In reply to comment #16) > These test files give me *incredibly* slow performance on my local machine > (e.g. 10 seconds for a repaint). I should also add that by local machine I'm referring to a Win7 machine and a WinXP machine. Perhaps performance is worse still on OSX.

What's happening is that the default for patternUnits is objectBoundingBox and in bug 465087 and your tests you have something like <pattern id="xy" width="100" height="100" x="0.1" y="-0.1"> and the rect referencing this is this... <rect x="50" width="50" height="50" fill="red"/> So the pattern surface is created is 50 x 100 x 50 x 100 x 4 bytes. Try making the width and height of your patterns 1 (or 100%). That's most likely what you really want rather than having them 100 times the object size. We should clip the surfaces to the object size but we don't.

(In reply to comment #19) > What's happening is that the default for patternUnits is objectBoundingBox and > in bug 465087 and your tests you have something like > > <pattern id="xy" width="100" height="100" x="0.1" y="-0.1"> > > and the rect referencing this is this... > > <rect x="50" width="50" height="50" fill="red"/> > > So the pattern surface is created is 50 x 100 x 50 x 100 x 4 bytes. That helps a lot. Thanks Robert! (I think actually the rect referencing it is <rect width="100" height="100" stroke="black" fill="url(#pattern)"/> -- the 50x50 rectangles are just part of the pattern content. It seems like we're making a massive pattern surface that's mostly empty except for the four squares at the top-left corner.) > Try making the width and height of your patterns 1 (or 100%). That's most > likely what you really want rather than having them 100 times the object size. Yep, exactly. > We should clip the surfaces to the object size but we don't. There's certainly some optimisation that would be helpful since these test cases render quickly on WebKit and Opera. Thanks again Robert!

Updated roll-up patch with fixed test cases. Currently running through Try once again.

Pushed: http://hg.mozilla.org/mozilla-central/rev/413eb50d4b81