Last Comment Bug 413960 - viewport changes should only affect transforms if we have a viewBox attribute
: viewport changes should only affect transforms if we have a viewBox attribute
Status: RESOLVED FIXED
: perf
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Jonathan Watt [:jwatt]
:
Mentors:
Depends on: 374111
Blocks:
  Show dependency treegraph
 
Reported: 2008-01-25 01:16 PST by Robert Longson
Modified: 2012-05-04 13:36 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Clean up and document misleading viewBox code (14.96 KB, patch)
2012-05-03 06:36 PDT, Jonathan Watt [:jwatt]
longsonr: review-
Details | Diff | Splinter Review
Only send the modification flags that are needed (7.50 KB, patch)
2012-05-03 06:37 PDT, Jonathan Watt [:jwatt]
longsonr: review-
Details | Diff | Splinter Review
Clean up and document misleading viewBox code (14.96 KB, patch)
2012-05-03 08:09 PDT, Jonathan Watt [:jwatt]
longsonr: review+
Details | Diff | Splinter Review
Only send the change notification flags that are required (8.02 KB, patch)
2012-05-03 08:33 PDT, Jonathan Watt [:jwatt]
longsonr: review+
Details | Diff | Splinter Review

Description Robert Longson 2008-01-25 01:16:17 PST
Follow-up from bug 411555 comment 26.
Comment 1 Robert Longson 2009-12-23 13:39:00 PST
In nsSVGSVGElement::GetViewBoxTransform if the viewPortWidth or viewportHeight is zero then the transform returned is singular, otherwise it is an identity matrix. So in actual fact the transform can change.

So to fix this we'd have to detect when the svg width/height changed from 0 to non-zero and vice versa. Is that worth doing Jonathan?
Comment 2 Jonathan Watt [:jwatt] 2012-04-30 14:51:42 PDT
This is unnecessarily hurting us during window/viewport resize of expensive SVGs that do not actually have a valid viewBox rect. We have at least one bug open that would benefit from this being fixed, although I can't currently remember where I saw it.
Comment 3 Jonathan Watt [:jwatt] 2012-05-03 06:36:37 PDT
Created attachment 620679 [details] [diff] [review]
Clean up and document misleading viewBox code
Comment 4 Jonathan Watt [:jwatt] 2012-05-03 06:37:40 PDT
Created attachment 620680 [details] [diff] [review]
Only send the modification flags that are needed
Comment 5 Robert Longson 2012-05-03 06:47:58 PDT
Comment on attachment 620679 [details] [diff] [review]
Clean up and document misleading viewBox code

>+  /**
>+   * Returns true if this element has a base/anim value for its "viewBox"
>+   * attribute that defines a viewBox rectangle with finite values.
>+   *
>+   * Note that this does not check whether we need to synthesize a viewBox,
>+   * so you must call ShouldSynthesizeViewBox() if you need to check that too.
>+   *
>+   * Note also that this method does not pay attention to whether the width or
>+   * height values of the viewBox rect are positive!
>+   */
>+  bool HasViewBoxRect() const {
>+    return mViewBox.HasRect();
>+  }

Don't really see the point of this. Callers can just call mViewBox.HasRect() directly.

> 
> class nsSVGViewBox
> {
> 
>+  /**
>+   * Returns true if the corresponding "viewBox" attribute defined a rectangle
>+   * with finite values. Returns false if the viewBox was set to "none" or an
>+   * invalid string, or if any of the four rect values were too big to store in
>+   * a float.
>+   *
>+   * This method does not check whether the width or height values are positive,
>+   * so callers must check whether the viewBox rect is valid if necessary!
>+   */
>+  bool HasRect() const
>     { return (mHasBaseVal || mAnimVal); }

Every other type uses IsExplicitlySet so that's what viewBox should use rather than HasRect or IsValid.
Comment 6 Jonathan Watt [:jwatt] 2012-05-03 06:51:34 PDT
(In reply to Robert Longson from comment #5)
> Don't really see the point of this. Callers can just call mViewBox.HasRect()
> directly.

It's cleaner not to have other classes reaching into private members IMO.

> Every other type uses IsExplicitlySet so that's what viewBox should use
> rather than HasRect or IsValid.

I disagree in this case, since IsExplicitlySet would sound like it should return true for viewBox="none", when actually what the callers are interested in is whether there is a rect defined.
Comment 7 Robert Longson 2012-05-03 06:52:19 PDT
Does part 2 pass try? I would be surprised if it did because you've not handled the issue in comment 1.
Comment 8 Robert Longson 2012-05-03 06:55:11 PDT
(In reply to Jonathan Watt [:jwatt] from comment #6)
> It's cleaner not to have other classes reaching into private members IMO.

The frame classes do that throughout to get element attribute values.

> 
> > Every other type uses IsExplicitlySet so that's what viewBox should use
> > rather than HasRect or IsValid.
> 
> I disagree in this case, since IsExplicitlySet would sound like it should
> return true for viewBox="none", when actually what the callers are
> interested in is whether there is a rect defined.

viewBox="none" you're making that up! Or maybe just confusing viewBox with preserveAspectRatio.
Comment 9 Robert Longson 2012-05-03 07:31:35 PDT
Comment on attachment 620680 [details] [diff] [review]
Only send the modification flags that are needed

It did indeed fail on try.
Comment 10 Jonathan Watt [:jwatt] 2012-05-03 08:09:32 PDT
Created attachment 620704 [details] [diff] [review]
Clean up and document misleading viewBox code

I've gone with IsExplicitlySet as you want, but kept the nsSVGSVGElement method (shortened to just "HasViewBox" not that I've been corrected about the "none" value ;)) since I'd like to move away from reaching in to private members and it makes the code look cleaner.
Comment 11 Jonathan Watt [:jwatt] 2012-05-03 08:11:20 PDT
Err, forget the nsSVGSVGElement::ShouldSynthesizeViewBox XXX comment, I've removed that locally.
Comment 12 Robert Longson 2012-05-03 08:28:41 PDT
Comment on attachment 620704 [details] [diff] [review]
Clean up and document misleading viewBox code

r=longsonr assuming the crazy comment is removed as stated in comment 11
Comment 13 Jonathan Watt [:jwatt] 2012-05-03 08:33:53 PDT
Created attachment 620714 [details] [diff] [review]
Only send the change notification flags that are required

Note You need to log in before you can comment on or make changes to this bug.