Sebastian Zartner [:sebo] Reporter
	Description • 8 years ago

+++ This bug was initially created as a clone of Bug 619500 comment 85 +++

Testing this on Firefox 48.0a1 (2016-04-14) the results of the test case look like expected for the 50% slices now. I.e. you see a differently colored circle on each corner.

Though the ones using 25% slices look broken. It looks like the stretching is done preserving the aspect ratio of each of the 1/4 parts, which is wrong.
This gets even worse when zooming the page.
The expected display would stretch the two horizontal edge slices only horizontally and the vertical edge slices only vertically.

I have added a PNG file as reference border image.

Sebastian

	u459114
	Comment 7 • 8 years ago

Kevin, please check https://bugzilla.mozilla.org/show_bug.cgi?id=1151016#c5

	Kevin Chen[:kechen] (UTC + 8) Assignee
	Comment 8 • 8 years ago

The reason that border-image broken is due to the reset of preserveAspectRatio  for SVG with valid viewport size or aspect-ratio when drawing the image[1], this patch can prevent this reset.
But I am still studying about the spec to make sure if this behavior is correct.

[1] https://dxr.mozilla.org/mozilla-central/source/image/VectorImage.cpp#840

	Kevin Chen[:kechen] (UTC + 8) Assignee
	Comment 11 • 8 years ago

Hi Sebastian Zartner,

Though the current border-image with 25% seems broken, IMHO, it might be the correct behavior, the reason is described in following paragraph :

According the link of steps of drawing border-image in [1], taking the top and bottom edge for example, it will take two steps to scale the SVG image to target border's size.

Step 1 :
"The two images for the top and bottom edges are made as tall as the top and bottom border image area parts, respectively, and their width is scaled proportionally."

Step 2 :
"If the first keyword is ‘stretch’, the top, middle and bottom images are further scaled to be as wide as the middle part of the border image area. The height is not changed any further."

For the word "scale" in description above, I am not sure what it actually means, but if it means "change of viewport size"; Then, in the above SVG image(like attachment 8741555 [details]), it uses percentage as the value of circle's radius which will change dynamically while scaling according to SVG's spec[2] (the function will be "qrt((actual-width)**2 + (actual-height)**2))/sqrt(2)" ).

In that case, when resizing the SVG's subimage to fit the target border edge, we might expect it displays like attachment 8756720 [details], however it will actually displays like attachment 8756721 [details], and shows the broken border.

Is my description correct? Or are there anything I miss? Can you give me some advices?

Thanks!


[1] https://www.w3.org/TR/css3-background/#border-image-process
[2] https://www.w3.org/TR/SVG/coords.html#Units

	Sebastian Zartner [:sebo] Reporter
	Comment 12 • 8 years ago

(In reply to Kevin Chen[:kechen] from comment #11)
> Hi Sebastian Zartner,
> 
> Though the current border-image with 25% seems broken, IMHO, it might be the
> correct behavior, the reason is described in following paragraph :
> 
> According the link of steps of drawing border-image in [1], taking the top
> and bottom edge for example, it will take two steps to scale the SVG image
> to target border's size.

I had to reread the spec. a few times, but here is how I interpret the steps:

> Step 1 :
> "The two images for the top and bottom edges are made as tall as the top and
> bottom border image area parts, respectively, and their width is scaled
> proportionally."

This scales the width and height of the border image part proportionally until its height fills the border image area.

Applied to our 10px SVG with the 25% slices and the 30px border this means for the top border image area: Scale the 5px x 2.5px top border image part up to 60px x 30px (proportionally).

> Step 2 :
> "If the first keyword is ‘stretch’, the top, middle and bottom images are
> further scaled to be as wide as the middle part of the border image area.
> The height is not changed any further."

Applied to our example this means to resize the width of the 60px x 30px border image part, so that it fills the width of the top border image area.

> For the word "scale" in description above, I am not sure what it actually
> means, but if it means "change of viewport size"; Then, in the above SVG
> image(like attachment 8741555 [details]), it uses percentage as the value of
> circle's radius which will change dynamically while scaling according to
> SVG's spec[2] (the function will be "qrt((actual-width)**2 +
> (actual-height)**2))/sqrt(2)" ).

To my understanding 'scale' means resizing the image dimensions. There is no mention of the image's viewport and other image formats also don't have a viewport.

Sebastian

	Kevin Chen[:kechen] (UTC + 8) Assignee
	Comment 13 • 8 years ago

This patch can fix the problem, but it is more like a walk-around solution.

When using SVG image, which has viewBox attribute, as border-image, the preserveAspectRatio of SVG will be set to none for non-uniform scaling [1]; however, the reset of preserveAspectRatio somehow effect the rendering result of those border-images which use SVG without viewBox as image source.

I will continue inspecting the reason why the preserveAspectRatio effects the rendering result even if SVG source doesn't have viewBox attribute.


[1] https://dxr.mozilla.org/mozilla-central/source/image/VectorImage.cpp#840

	Kevin Chen[:kechen] (UTC + 8) Assignee
	Comment 14 • 8 years ago

Hi Daniel Holbert & CJ,
Can you give me some advices to this patch ?

When using SVG image as border-image, there are four possible settings in SVG image :
1. Has both viewport size and viewBox.
2. Has viewBox, but no viewport size.
3. Has viewport size, but no viewBox.
4. No viewport size and viewBox.

In bug 619500, we fixed the situation 1 & 2, it sets the preserveAspectRatio to none, so that SVG image can scale non-uniformly to fit border-image area [1].

This change should only effect the behavior of the SVG images who has viewBox attribute; however, the reset of preserveAspectRatio will trigger SetImageOverridePreserveAspectRatio which sets the flag  mIsPaintingSVGImageElement to true [2] and because we have changed viewport size in [3] to get proper size for rendering which cause system to recalculate a new viewBox value with modified viewport size and scale the SVG image improperly which cause situation 3 broken.

So, the solution to this problem is to avoid the reset of preserveAspectRatio if SVG source doesn't have viewBox attribute.


And for situation 4, when we scale the viewport size in [3], we kind of break its width height ratio and make it resize improperly, so my solution here is to select bigger ratio of height and width to scale the viewport size, kind of like what "slice" in preserveAspectRatio do to keep the ratio of viewport size.

Does the solution looks okay to you ?
Thank you.


[1] https://dxr.mozilla.org/mozilla-central/source/image/VectorImage.cpp?from=VectorImage.cpp#840
[2] https://dxr.mozilla.org/mozilla-central/source/dom/svg/SVGSVGElement.cpp#1100
[3] https://dxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#6482

	u459114
	Comment 15 • 8 years ago

Comment on attachment 8758583 [details] [diff] [review]
[WIP] Prevent reseting preserveAspectRatio for SVG without viewBox

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

LGTM, thanks, Kevin

	Daniel Holbert [:dholbert]
	Comment 16 • 8 years ago

(In reply to Kevin Chen[:kechen] from comment #14)
> however, the reset of preserveAspectRatio will trigger
> SetImageOverridePreserveAspectRatio which sets the flag 
> mIsPaintingSVGImageElement to true [2]
[which disables our code that synthesizes a viewBox from width/height attributes]
> So, the solution to this problem is to avoid the reset of
> preserveAspectRatio if SVG source doesn't have viewBox attribute.

I'd prefer that we solve this by *preventing ourselves* from ever incorrectly setting mIsPaintingSVGImageElement, when we override the preserveAspectRatio attribute here.  That seems to be the root of the problem.

(That bool is specifically about whether we're being painted for the SVG "<image>" element, in a SVG host document.  If we're being painted for one of those elements, we need to *not* synthesize a viewBox -- that's why that bool exists.  But otherwise, for HTML/CSS contexts, we absolutely should allow ourselves to synthesize a viewbox.)

So I think we need to decouple SetImageOverridePreserveAspectRatio() from "mIsPaintingSVGImageElement", and add a new private setter (called maybe "SetIsPaintingForSVGImageElement()"), and add a new bool to SVGImageContext (alongside the override preserveAspectRatio value) which controls whether AutoSVGRenderingState will call this new method.

Does that make sense?

	Daniel Holbert [:dholbert]
	Comment 17 • 8 years ago

(In reply to Kevin Chen[:kechen] from comment #14)
> And for situation 4, when we scale the viewport size in [3], we kind of
> break its width height ratio and make it resize improperly, so my solution
> here is to select bigger ratio of height and width to scale the viewport
> size, kind of like what "slice" in preserveAspectRatio do to keep the ratio
> of viewport size.

(For this part: I'm not as familiar with what's going on in this section of ClippedImage, and I'd need to do a bit more poking around to be sure that what you're doing there makes sense. My first reaction is that I think it might be simpler/cleaner to adjust |scale| up-front, rather than to adjust |finalScale|; but maybe it doesn't matter much.)

	Daniel Holbert [:dholbert]
	Comment 18 • 8 years ago

Comment on attachment 8758583 [details] [diff] [review]
[WIP] Prevent reseting preserveAspectRatio for SVG without viewBox

(feedback- since I think we should go about fixing this differently, per comment 16)

	Kevin Chen[:kechen] (UTC + 8) Assignee
	Comment 19 • 8 years ago

I separate this patch into two parts, since situation 3 & 4 mentioned in comment 14 are caused by different reasons.

In this patch, I decouple SetImageOverridePreserveAspectRatio() from mIsPaintingSVGImageElement which solves the problem listed in situation 3.

	Kevin Chen[:kechen] (UTC + 8) Assignee
	Comment 20 • 8 years ago

In this patch, I preserve the width/height ratio of SVG source which has no viewport size when calculating the optimal rendering size.

This can fix the problem listed in situation 4, however, there might be a better way to solve this. Still trying it.

	Kevin Chen[:kechen] (UTC + 8) Assignee
	Comment 21 • 8 years ago

Hi Daniel,

When the border image do the image snapping algorithm[1] separately, it will scale the SVG source's viewport size to get the best resolution[2]. However, for those SVG sources who don't have viewport size or viewBox value, this adjustment will break the ratio and squeeze the twist. Therefore, I add a flag "FLAG_SCALE_WITH_FIXED_RATIO" to prevent the source from non-fix ratio scaling.

Can you help me to review this patch ? Thank you.

[1] https://wiki.mozilla.org/Gecko:Image_Snapping_and_Rendering
[2] https://dxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#6553

	Kevin Chen[:kechen] (UTC + 8) Assignee
	Comment 22 • 8 years ago

Comment on attachment 8759607 [details] [diff] [review]
(Part 1) Decouple SetImageOverridePreserveAspectRatio from mIsPaintingSVGImageElement setting

Hi Daniel,

I modify the patch according to comment 16, can you help me to review this patch ?
Thank you.

	Kevin Chen[:kechen] (UTC + 8) Assignee
	Comment 23 • 8 years ago

Hi Daniel,

I add two reftests, for situation 3 & 4 which is mentioned in comment 14.
Can you help me to review this patch? Thank you.

	Daniel Holbert [:dholbert]
	Comment 24 • 8 years ago

Comment on attachment 8759607 [details] [diff] [review]
(Part 1) Decouple SetImageOverridePreserveAspectRatio from mIsPaintingSVGImageElement setting

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

Right now, there's a newline in the middle of the commit message -- please remove that newline and be sure that the full "main" commit message is on a single line inside the patch-file.

(Many tools, e.g. hg.mozilla.org pushlog, only display the first line of the commit message, i.e. up to the first newline character.  So, an arbitrary linebreak will inadvertantly truncate the message that's shown in those tools.)

::: image/VectorImage.cpp
@@ +842,5 @@
>        Some(SVGPreserveAspectRatio(SVG_PRESERVEASPECTRATIO_NONE,
>                                    SVG_MEETORSLICE_UNKNOWN));
>      svgContext =
>        Some(SVGImageContext(aSVGContext->GetViewportSize(),
> +                           aspectRatio, 1.0, false));

As noted below, I think your new arg to SVGImageContext() should *default* to false, which means you can revert this change here, I think.

::: layout/svg/SVGImageContext.h
@@ +26,5 @@
>  
>    SVGImageContext(CSSIntSize aViewportSize,
>                    Maybe<SVGPreserveAspectRatio> aPreserveAspectRatio,
> +                  gfxFloat aOpacity = 1.0,
> +                  bool aIsPaintingSVGImageElement = true)

Two notes here:
 (1) Please add a line of documentation for "aIsPaintingSVGImageElement" somewhere. (It's extremely easy to misunderstand what this variable would mean, due to "SVG Image" being an ambiguous term -- and every SVGImageContext being associated with an "SVG Image".)

e.g. maybe above the constructor here, add a comment saying:
  // Note: 'aIsPaintingSVGImageElement' should be used to indicate whether
  // the SVG image in question is being painted for an SVG <image> element.


(2) I don't think aIsPaintingSVGImageElement should default to "true". Painting for an SVG <image> element is a rare scenario, so most callers will want this variable to be set to false.

I think the only place where we should set it to true (i.e. the only place we're actually painting an SVG <image> element) is in the SVGImageContext instance that gets created in nsSVGImageFrame.cpp, here:
http://mxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGImageFrame.cpp?rev=0f0586c0b68d&force=1#400

	Daniel Holbert [:dholbert]
	Comment 25 • 8 years ago

Comment on attachment 8760535 [details] [diff] [review]
(Part 2) Preserve scale ratio if SVG dosen't have viewport size and viewBox.

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

This patch has a newline in the middle of its commit message -- please fix that, as noted above for part 1.

::: image/ClippedImage.cpp
@@ +505,5 @@
>    }
>  
>    int32_t imgWidth, imgHeight;
> +  enum ScaleFlag {DONT_SCALE, NEED_SCALE, SCALE_WITH_RATIO};
> +  ScaleFlag needScale = DONT_SCALE;

I tend to think this enum stuff is more confusing than it's worth. (In part because the names here are a bit confusing -- e.g. "needScale" still sounds like a bool, and needScale/NEED_SCALE have the same name and yet the latter is just one possible value that the former can take.)

Seems to me it'd be simpler to revert all of the enum stuff here, and just control the new uniform-scaling behavior with a new bool called e.g. "mustScaleUniformly".

So, I think this should go back to just:
  bool needScale = false;
...and you should add after that:
  bool mustScaleUniformly = false;

And all other usages of "needScale" should be reverted.

@@ +510,5 @@
>    if (mSVGViewportSize && !mSVGViewportSize->IsEmpty()) {
>      imgWidth = mSVGViewportSize->width;
>      imgHeight = mSVGViewportSize->height;
> +    needScale = (aFlags & imgIContainer::FLAG_SCALE_WITH_FIXED_RATIO)
> +      ? SCALE_WITH_RATIO : NEED_SCALE;

And this line should reverting back to:
  needScale = true;
...and you'll want to insert after that:
  mustScaleUniformly = (aFlags & imgIContainer::FLAG_SCALE_WITH_FIXED_RATIO);

@@ +525,5 @@
>      nsIntSize scale(ceil(aDest.width / mClip.width),
>                      ceil(aDest.height / mClip.height));
>  
> +    if (needScale == SCALE_WITH_RATIO) {
> +      scale.width = scale.height = max(scale.height, scale.width);

And this would just be "if (mustScaleUniformly)"

::: image/imgIContainer.idl
@@ +206,5 @@
>    const unsigned long FLAG_HIGH_QUALITY_SCALING            = 0x40;
>    const unsigned long FLAG_WANT_DATA_SURFACE               = 0x80;
>    const unsigned long FLAG_BYPASS_SURFACE_CACHE            = 0x100;
>    const unsigned long FLAG_FORCE_PRESERVEASPECTRATIO_NONE  = 0x200;
> +  const unsigned long FLAG_SCALE_WITH_FIXED_RATIO          = 0x400;

I'd suggest we name this...
 FLAG_FORCE_UNIFORM_SCALING

That seems clearer to me; "uniform scaling" is pretty unambiguous, I think. (and it happens to match the variable-naming I'm suggesting in ClippedImage.cpp)

(The "ratio" in "SCALE_WITH_FIXED_RATIO" is ambiguous -- e.g. it could be misread as saying that the ratio *between our scale factors* might have a fixed ratio, if perhaps the caller wanted scaleX to always be twice as large as scaleY, for some reason.  Anyway, that's not what you want this flag to mean, so that sort of ambiguity is worth avoiding.)

::: layout/base/nsCSSRendering.cpp
@@ +3866,5 @@
>        // In this condition, we pass imageSize(a resolved size comes from
>        // default sizing algorithm) to renderer as the viewport size.
> +      Maybe<nsSize> svgViewportSize =
> +        (intrinsicSize.CanComputeConcreteSize()) ?
> +          Nothing() : Some(imageSize);

Probably revert this change, unless it's doing something substantial that I'm missing.

(It doesn't seem to be actually changing the code -- it's just adding a linebreak and some extra parens, I think.  IMO, the old version is easier to read.

@@ +5486,5 @@
>      // FLAG_FORCE_PRESERVEASPECTRATIO_NONE flag here, to tell mImage to ignore
>      // preserveAspectRatio attribute, and always do non-uniform stretch.
>      uint32_t drawFlags = ConvertImageRendererToDrawFlags(mFlags) |
>                             imgIContainer::FLAG_FORCE_PRESERVEASPECTRATIO_NONE;
> +    // For those SVG image sources which don't have fix ratio (i.e without

Typos:
  s/fix ratio/fixed ratio/
  s/i.e/i.e./  (Add a period after "e")

@@ +5487,5 @@
>      // preserveAspectRatio attribute, and always do non-uniform stretch.
>      uint32_t drawFlags = ConvertImageRendererToDrawFlags(mFlags) |
>                             imgIContainer::FLAG_FORCE_PRESERVEASPECTRATIO_NONE;
> +    // For those SVG image sources which don't have fix ratio (i.e without
> +    // viewport size and viewbox), we should scale the source in a fixed ratio

s/in a fixed ratio/uniformly/

@@ +5488,5 @@
>      uint32_t drawFlags = ConvertImageRendererToDrawFlags(mFlags) |
>                             imgIContainer::FLAG_FORCE_PRESERVEASPECTRATIO_NONE;
> +    // For those SVG image sources which don't have fix ratio (i.e without
> +    // viewport size and viewbox), we should scale the source in a fixed ratio
> +    // after the viewport size is desided by "Default Sizing Algorithm".

Typo:
 s/desided/decided/

@@ +5489,5 @@
>                             imgIContainer::FLAG_FORCE_PRESERVEASPECTRATIO_NONE;
> +    // For those SVG image sources which don't have fix ratio (i.e without
> +    // viewport size and viewbox), we should scale the source in a fixed ratio
> +    // after the viewport size is desided by "Default Sizing Algorithm".
> +    if (!ComputeIntrinsicSize().CanPreserveRatio()) {

As noted below, I think this should perhaps just be "HasAspectRatio()" (I don't think we need to add CanPreserveRatio()).

::: layout/base/nsCSSRendering.h
@@ +50,5 @@
>      return mHasWidth + mHasHeight + HasRatio() >= 2;
>    }
> +  bool CanPreserveRatio() const
> +  {
> +    return (mHasWidth & mHasHeight) | HasRatio();

You're using bitwise operators here... I think you really meant to use logical operators? (&& and ||, instead of & and |)

Also, more importantly -- do we actually need this function?  I'm pretty sure "HasRatio()" on its own should be sufficient.  (Could you give that a try? If that fails, I'd be curious to see why).

I think you're trying to cover the "<svg> has |width| and |height| specified, but doesn't have viewBox" scenario here.  BUT, HasRatio() already effectively covers that scenario, because it ends up being backed[1] by nsSVGOuterSVGFrame::GetIntrinsicRatio(), which already has custom code to handle width & height being specified. (and it successfully generates an aspect ratio from them)

[1] (HasRatio returns mRatio, which is set via nsLayoutUtils::ComputeSizeForDrawing, which invokes VectorImage::GetIntrinsicRatio, which calls GetIntrinsicRatio() on the outer frame, and the outer frame for an SVG document will be of type nsSVGOuterSVGFrame ).

	Daniel Holbert [:dholbert]
	Comment 26 • 8 years ago

Comment on attachment 8760675 [details] [diff] [review]
(Part 3) Add reftest.

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

::: layout/reftests/border-image/reftest.list
@@ +87,5 @@
>  == svg-as-border-image-1a.html svg-as-border-image-1-ref.html
>  == svg-as-border-image-1b.html svg-as-border-image-1-ref.html
>  == svg-as-border-image-1c.html svg-as-border-image-1-ref.html
> +fuzzy(75,500) == svg-as-border-image-1d.html svg-as-border-image-1-ref-2.html
> +== svg-as-border-image-1e.html svg-as-border-image-1-ref-3.html

Why do these files have "-1-ref-2" / "-1-ref-3" suffixes? Are they reference cases, or testcases? (I think they're testcases, in which case they should be "-1f" and "-1g" perhaps?)

If it's not clear, our naming convention for reftests is to name the files like so:
 foo-1.html foo-1-ref.html
 foo-2.html foo-2-ref.html

...and if there are multiple files that match a single reference case (as is the case in the preexisting tests here), we'll have:
 foo-1a.html foo-1-ref.html
 foo-1b.html foo-1-ref.html
 foo-1c.html foo-1-ref.html
 foo-2.html  foo-2-ref.html

Subsequent tests should only have "foo-1" in their name if they're actually testcases that match "foo-1-ref.html". Otherwise, they want to be numbered "2" or "3" or whatever.

So: perhaps you want to be adding these new files with names like...
== svg-as-border-image-2.html svg-as-border-image-2-ref.html
== svg-as-border-image-3.html svg-as-border-image-3-ref.html

? (I'm not sure)

Also: "fuzzy(75,500)" is a pretty large amount of fuzziness/mismatch for your first new test here. Do you know why your test needs that? Is it actually passing?

	Daniel Holbert [:dholbert]
	Comment 27 • 8 years ago

(In reply to Daniel Holbert [:dholbert] from comment #25)
> Seems to me it'd be simpler to revert all of the enum stuff here, and just
> control the new uniform-scaling behavior with a new bool called e.g.
> "mustScaleUniformly".
[...]
> > +  const unsigned long FLAG_SCALE_WITH_FIXED_RATIO          = 0x400;
> 
> I'd suggest we name this...
>  FLAG_FORCE_UNIFORM_SCALING

Er -- actually, I should have suggested "bool forceUniformScaling" instead of "bool mustScaleUniformly", for better consistency with my suggested replacement FLAG_ name (FLAG_FORCE_UNIFORM_SCALING).  Seems worth having that naming be consistent.

	Kevin Chen[:kechen] (UTC + 8) Assignee
	Comment 28 • 8 years ago

Hi Daniel,

I modified this patch according to comment 24, can you help me to review it ?

Thank you.

	Kevin Chen[:kechen] (UTC + 8) Assignee
	Comment 29 • 8 years ago

Hi Daniel,

I modified this patch according to comment 25 and comment 27, can you help me to review it ?

Thank you.

	Kevin Chen[:kechen] (UTC + 8) Assignee
	Comment 30 • 8 years ago

Hi Daniel,

I modified this patch according to comment 26 and found out that test svg-as-border-image-1b.html is covered by the test svg-as-border-image-2.html in this patch, should I also remove it ?

[1] https://dxr.mozilla.org/mozilla-central/source/layout/reftests/border-image/svg-as-border-image-1b.html

	Kevin Chen[:kechen] (UTC + 8) Assignee
	Comment 31 • 8 years ago

Try for these patches, an error occurs at Linux x64 debug tc-VP[tier 2], but that is not caused by these patches.

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

	Daniel Holbert [:dholbert]
	Comment 32 • 8 years ago

Comment on attachment 8765726 [details] [diff] [review]
(Part 1) Decouple SetImageOverridePreserveAspectRatio from mIsPaintingSVGImageElement setting

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

r=me with nits addressed.

Nit #1: As I noted in comment 24, your commit message still has a newline in the middle of it. Please look at the text of the patch file -- you'll see that it starts out like this:
> From: Kevin Chen <kechen@mozilla.com>
> Date: Fri, 3 Jun 2016 18:45:42 +0800
> Subject: Bug 1264809 - (Part 1) Decouple SetImageOverridePreserveAspectRatio
>  from mIsPaintingSVGImageElement setting.

Nit #2: Notice the hardcoded newline in the middle of the commit message there. Please drop it (and add "r=dholbert" at the end).  If a tool is auto-generating that newline for you when you populate your commit message, please reconfigure whatever tool you're using to edit commit messages so that it stops doing that.

::: layout/svg/SVGImageContext.h
@@ +48,5 @@
>      return mGlobalOpacity;
>    }
>  
> +  bool GetIsPaintingForSVGImageElement() const {
> +    return mIsPaintingSVGImageElement;

Please drop the "Get" from the function name here -- it's unnecessary.  This accessor can just be called "IsPaintingForSVGImageElement()".

(This makes it slightly asymmetric since we've got a SetIsPaintingForSVGImageElement(), but that's OK.  My recollection is that this is common practice for naming getters & setters for true/false things in Gecko.)

	Daniel Holbert [:dholbert]
	Comment 33 • 8 years ago

(Sorry, during some last-minute comment editing there, I misplaced my "Nit #2" label. I meant to associate that label with the "drop the Get from the function name" thing at the end, but I accidentally typed it into the middle of my first nit. Feel free to just ignore my mis-inserted "Nit #1" and "Nit #2" labels, and just refer to the text of my bug-comment. :))

	Daniel Holbert [:dholbert]
	Comment 34 • 8 years ago

Your updated "part2" still has a newline in the commit message, too. Please get rid of this newline there as well.

Also: there's a hardcoded date-stamp in the patch header for each of your patches here, which happens to be about a month out of date -- e.g. "part 2" has this in the second line of the patch file:
 "Date: Fri, 3 Jun 2016 19:11:26 +0800"

Please either remove those datestamps from your patch files when you repost the final versions here, or (if your tools insist on including a datestamp) please refresh the patches to use a more recent date. (Otherwise, this stale datestamp will stick around as the "official" datestamp of your commit on hg.mozilla.org, and it can cause confusion if hg-archeologists are trying to figure out when your patch landed when browsing hg history in the future.)

	Daniel Holbert [:dholbert]
	Comment 35 • 8 years ago

Comment on attachment 8765727 [details] [diff] [review]
(Part 2) Preserve scale ratio if SVG dosen't have viewport size and viewBox.

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

r=me with commit message / datestamp issues fixed (comment 34) and the following nits addressed:

::: layout/base/nsCSSRendering.cpp
@@ +5523,5 @@
>      // preserveAspectRatio attribute, and always do non-uniform stretch.
>      uint32_t drawFlags = ConvertImageRendererToDrawFlags(mFlags) |
>                             imgIContainer::FLAG_FORCE_PRESERVEASPECTRATIO_NONE;
> +    // For those SVG image sources which don't have fixed ratio (i.e. without
> +    // viewport size and viewbox), we should scale the source uniformly after

Two tweaks here:
 (a) s/ratio/aspect ratio/ for clarity (sorry, I should have said this up in comment 25)

 (b) Please use the correct capitalization for "viewBox" in the comment here (capitalize the letter "B" in the middle of the word.)

	Daniel Holbert [:dholbert]
	Comment 36 • 8 years ago

(In reply to Kevin Chen[:kechen] from comment #30)
> Created attachment 8765729 [details] [diff] [review]
> (Part 3) Add reftest.
> I modified this patch according to comment 26 and found out that test
> svg-as-border-image-1b.html is covered by the test
> svg-as-border-image-2.html in this patch, should I also remove it ?

Thanks for noticing -- I'd say we should just keep the old test around, even if your new test is similar.  Since the older test passes currently (and your new test does not), that makes me think they're testing scenarios that might be subtly-different enough to be worth testing independently.

(It may also be somewhat useful for completeness, to be able to compare it against the other svg-as-border-image-1a and svg-as-border-image-1c flavors and see what we're testing there.)

	Daniel Holbert [:dholbert]
	Comment 37 • 8 years ago

Comment on attachment 8765729 [details] [diff] [review]
(Part 3) Add reftest.

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

r=me on the test patch, with the month-old datestamp removed or made more current as noted above, and with the following nits:

::: layout/reftests/border-image/svg-as-border-image-2-ref.html
@@ +1,4 @@
> +<!DOCTYPE html>
> +<html lang="en-US">
> +<head>
> +<title>test of svg-as-border-image</title>

Nit: s/test of/reference for/ in the <title> here.  (since this file a reference case, not a testcase)

::: layout/reftests/border-image/svg-as-border-image-2.html
@@ +7,5 @@
> +  width: 10px;
> +  height: 10px;
> +  background-color: blue;
> +  border: 10px solid;
> +  border-image: url('data:image/svg+xml,<svg xmlns="http://www.w3.org/2000/svg" ><circle cx="50%" cy="50%" r="50%" fill="red"/></svg>') 5% stretch;

Nit: drop the stray space character just below the first ">" on this line. (after the xmlns attribute)

::: layout/reftests/border-image/svg-as-border-image-3-ref.html
@@ +1,4 @@
> +<!DOCTYPE html>
> +<html lang="en-US">
> +<head>
> +<title>test of svg-as-border-image</title>

As above, s/test of/reference for/

	Kevin Chen[:kechen] (UTC + 8) Assignee
	Comment 39 • 8 years ago

Carry r+ from comment 32

Try result shows in comment 31

	Kevin Chen[:kechen] (UTC + 8) Assignee
	Comment 40 • 8 years ago

Carry r+ from comment 35

Try result shows in comment 31

	Kevin Chen[:kechen] (UTC + 8) Assignee
	Comment 41 • 8 years ago

Carry r+ from comment 37

Try result shows in comment 31

	Carsten Book [:Tomcat]
	Comment 42 • 8 years ago

has problems to apply:

renamed 1264809 -> 0002-Bug-1264809-Part-2-Preserve-scale-ratio-if-SVG-dosen.patch
applying 0002-Bug-1264809-Part-2-Preserve-scale-ratio-if-SVG-dosen.patch
patching file image/ClippedImage.cpp
Hunk #1 FAILED at 0
Hunk #2 succeeded at 24 with fuzz 2 (offset 0 lines).
1 out of 3 hunks FAILED -- saving rejects to file image/ClippedImage.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 0002-Bug-1264809-Part-2-Preserve-scale-ratio-if-SVG-dosen.patch

	Kevin Chen[:kechen] (UTC + 8) Assignee
	Comment 43 • 8 years ago

Fix the conflict which is caused by reorder of header file in Bug 1282670

Carry r+ from comment 35

Try result shows in comment 31

	Pulsebot
	Comment 44 • 8 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bd0b92309c7
(Part 1) Decouple SetImageOverridePreserveAspectRatio from mIsPaintingSVGImageElement setting. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a7eb4a7864c
(Part 2) Preserve scale ratio if SVG dosen't have viewport size and viewBox. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/d19e8687583e
(Part 3) Add reftest. r=dholbert

	Carsten Book [:Tomcat]
	Comment 45 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/5bd0b92309c7
https://hg.mozilla.org/mozilla-central/rev/8a7eb4a7864c
https://hg.mozilla.org/mozilla-central/rev/d19e8687583e

	Sebastian Zartner [:sebo] Reporter
	Comment 46 • 8 years ago

The cases with the size 60 and 300 SVGs are fixed now. Thank you for that!

Unfortunately the one with the SVG without sizes is still unproportionally stretched. Also the size 10 SVG is incorrectly stretched, as the circles are cut a bit outside of their middle (I'll attach a screenshot for clarification).
In both cases the border images should look the same as for the size 60 and 300 SVGs.

Sebastian

	Robert Longson [:longsonr]
	Comment 47 • 8 years ago

Additional issues should be raised in a new bug.

	Sebastian Zartner [:sebo] Reporter
	Comment 48 • 8 years ago

Here´s a screenshot with the erroneous parts marked and explained.
Again, all four cases with SVGs are expected to look the same.

Sebastian

	Sebastian Zartner [:sebo] Reporter
	Comment 49 • 8 years ago

(In reply to Robert Longson from comment #47)
> Additional issues should be raised in a new bug.

Those are part of the same issue, but I´ll file additional bugs if that's easier to track.

Sebastian

	Robert Longson [:longsonr]
	Comment 50 • 8 years ago

Please file new bugs, it's easier to track that way. We'd only reopen this bug if its patches need to be backed out.

	Sebastian Zartner [:sebo] Reporter
	Comment 51 • 8 years ago

(In reply to Robert Longson from comment #50)
> Please file new bugs, it's easier to track that way. We'd only reopen this
> bug if its patches need to be backed out.

Ok. So I've created bug 1284797 and bug 1284798 accordingly and provided adjusted test cases for them.

Sebastian

	Sebastian Zartner [:sebo] Reporter
	Comment 52 • 8 years ago

I've added a compatibility note about this to https://developer.mozilla.org/en-US/docs/Web/CSS/border-image.

Sebastian

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 53 • 8 years ago

Is 49 also affected? Is this work all planned to ride with 50 or does any of it need uplift? I'm asking because bug 1269528 is marked as a regression in 48 and 49 and so it shows up in platform triage.

	Kevin Chen[:kechen] (UTC + 8) Assignee
	Comment 54 • 8 years ago

Yes, I think the versions after bug 619500, which is firefox48, are affected.
Should I also mark the regression tags in bug 1269528 and uplift the patches ?

	Astley Chen (inactive)
	Comment 55 • 8 years ago

Hi Kevin,
  Talked with FF48 release manager Gerry, both of us think that for such trivial visual artifact and regressed from FF48 will need to be fixed before we punt 48 to release channel users.
As long as you think the risk is manageable, please uplift the patch to FF48 and FF49.

Thanks.

	Gerry Chang [:gchang]
	Comment 56 • 8 years ago

Track this in 48/49/50 as this is a regression. After the patch is uplift to beta, need QE to verify it.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 57 • 8 years ago

What was this a regression from?

I thought this was simply an issue that was not fixed by bug 619500?  Did something actually *regress*?

Uplifting to beta at this point in the cycle is risky and shouldn't be done without a good reason.  We don't get substantial Web-compatibility testing and feedback, plus a chance to fix based on that feedback, in 3 weeks.

	Sebastian Zartner [:sebo] Reporter
	Comment 58 • 8 years ago

(In reply to David Baron :dbaron: ⌚️UTC-4 (away July 7-10) (review requests must explain patch) from comment #57)
> What was this a regression from?
> 
> I thought this was simply an issue that was not fixed by bug 619500?  Did
> something actually *regress*?

Bug 619500 regressed some behavior of 'border-image-slice', see bug 619500 comment 85.

Sebastian

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 59 • 8 years ago

Is that regression fully fixed in this bug?  Or are bug 1284797 and bug 1284798 also required to fix the regression from bug 619500?

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 60 • 8 years ago

How common will it be for users to hit these issues? I don't understand comment 55. If this is a trivial issue let's keep it in 50.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 61 • 8 years ago

Marking this as fix-optional for 48, while we could still consider uplift to beta I don't think it needs to block release.  We could still take this for aurora.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 62 • 8 years ago

Could you answer comment 59?

(Also, it helps to explain this more clearly when you report the bug.)

	Kevin Chen[:kechen] (UTC + 8) Assignee
	Comment 63 • 8 years ago

Approval Request Comment
[Feature/regressing bug #]:
  Bug 619500
[User impact if declined]:
  The websites who uses SVG , which doesn't contain a viewBox attribute, as the source of border-image might find display errors.
[Describe test coverage new/current, TreeHerder]:
  Two reftests are contained in patch part 3.
[Risks and why]: 
  I think the risk is low, because in this patch the changes I made only affect the flow of rendering border-image.

	Kevin Chen[:kechen] (UTC + 8) Assignee
	Comment 64 • 8 years ago

Comment on attachment 8767071 [details] [diff] [review]
(Part 3) Add reftest.

Approval Request Comment
[Feature/regressing bug #]:
  Bug 619500
[User impact if declined]:
  The websites who uses SVG , which doesn't contain a viewBox attribute, as the source of border-image might find display errors.
[Describe test coverage new/current, TreeHerder]:
  Two reftests are contained in patch part 3.
[Risks and why]: 
  I think the risk is very low, because in this patch I only add two tests and doesn't change any code flow.

	Kevin Chen[:kechen] (UTC + 8) Assignee
	Comment 65 • 8 years ago

Comment on attachment 8767069 [details] [diff] [review]
(Part 1) Decouple SetImageOverridePreserveAspectRatio from mIsPaintingSVGImageElement setting

Approval Request Comment
[Feature/regressing bug #]:
  Bug 619500
[User impact if declined]:
  The websites who uses SVG , which doesn't contain a viewBox attribute, as the source of border-image might find display errors.
[Describe test coverage new/current, TreeHerder]:
  Two reftests are contained in patch part 3.
[Risks and why]: 
  I think the risk is low, because the changes I made is only to decide whether the SVG image are allowed to synthesize the viewBox. And the logic is pretty clear.
But, still, I think it's better if someone who is more familiar with this module can help to evaluate the risk of this patch because the codes which is affected here is relatively big.

	Kevin Chen[:kechen] (UTC + 8) Assignee
	Comment 66 • 8 years ago

Hi Liz,

I've already applied my patches to aurora and run the related tests in my local machine. The only conflict happened in patch part 2, which I've fixed it and uploaded it as the attachment above.

Please notice me if these patches are also needed to be uplifted to beta.

Thanks.

	Kevin Chen[:kechen] (UTC + 8) Assignee
	Comment 69 • 8 years ago

Screenshot of small-size SVG before bug 619500.

	Kevin Chen[:kechen] (UTC + 8) Assignee
	Comment 70 • 8 years ago

Hi David,

I think maybe I can provide some information to question in comment 59.

Yes, I think this bug can fully fix the regression from Bug 619500.

The regression caused by Bug 619500 happened only when we use SVG, which has viewport size but no viewBox size and also contains <circle> elements, as source of the border-image attribute.

The image in attachment 8769552 [details] is the screenshot of the testcase in attachment 8769551 [details].

The left-hand side is the result with build after Bug 619500[1] and the right-hand side is the result with build before Bug 619500[2], the result shows that:
  1. Bug 619500 fix the case which SVG source has viewBox attribute set.
  2. Break the case which SVG source has viewport size but doesn't has viewBox.
  3. The act of SVG source which has no viewport size nor viewBox attribute changes but still remains broken.

And the patches in this bug solved the case 2 & 3 mentioned above.

And the image in attachment 8769553 [details] is the screenshot with small-size SVG source with build before Bug 619500[2], which is the problem Bug 1284797 wants to solve.
In the other hand, the situation Bug 1284798 wants to discuss is just like the first case in testcase in attachment 8769551 [details], it is broken before and after Bug 619500.
Therefore, IMHO, these two bugs are not regression from Bug619500.


Needinfo to Sebastian again if I miss something. Thank you.

[1] https://hg.mozilla.org/mozilla-central/rev/bc277a82a03e
[2] https://hg.mozilla.org/mozilla-central/rev/4adaa4b1ddb2

	Kevin Chen[:kechen] (UTC + 8) Assignee
	Comment 71 • 8 years ago

Needinfo to Sebastian again if I miss something. Thank you.

	Sebastian Zartner [:sebo] Reporter
	Comment 77 • 8 years ago

Excuse my late reply! Thank you Kevin for explaining the regression in detail!

(In reply to David Baron :dbaron: ⌚️UTC-4 (away July 7-10) (review requests must explain patch) from comment #57)
> What was this a regression from?
> 
> I thought this was simply an issue that was not fixed by bug 619500?  Did
> something actually *regress*?

As Kevin wrote, bug 619500 regressed the display for SVGs that don't have a viewBox defined. That got fixed in this bug.

> Uplifting to beta at this point in the cycle is risky and shouldn't be done
> without a good reason.  We don't get substantial Web-compatibility testing
> and feedback, plus a chance to fix based on that feedback, in 3 weeks.

(In reply to David Baron :dbaron: ⌚️UTC-4 (away July 7-10) (review requests must explain patch) from comment #59)
> Is that regression fully fixed in this bug?

Yes.

> Or are bug 1284797 and bug 1284798 also required to fix the regression from bug 619500?

Both are independent from bug 619500.

Sebastian

	Daniel Holbert [:dholbert]
	Comment 78 • 8 years ago

(In reply to Kevin Chen[:kechen] (UTC + 8) from comment #65)
> [Risks and why]: 
>   I think the risk is low, because the changes I made is only to decide
> whether the SVG image are allowed to synthesize the viewBox. And the logic
> is pretty clear.
> But, still, I think it's better if someone who is more familiar with this
> module can help to evaluate the risk of this patch because the codes which
> is affected here is relatively big.

I'll back up Kevin on this -- I think the risk here is sufficiently low for an Aurora uplift.

(I think it's too late for a beta uplift, though, for a patch of this size/complexity.)

	Astley Chen (inactive)
	Comment 79 • 8 years ago

Thanks Daniel. Kevin, per discussion, please uplift your patches to aurora only. We will back out the fix in bug 619500 in beta to get rid of regression in firefox 48.

	Kevin Chen[:kechen] (UTC + 8) Assignee
	Comment 80 • 8 years ago

Hi Liz,

I've tagged my patches as "approval-mozilla-aurora?" and evaluated the risks.

Can you help me to approve the requests and uplifts the patches?

Thank you.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 81 • 8 years ago

Comment on attachment 8767069 [details] [diff] [review]
(Part 1) Decouple SetImageOverridePreserveAspectRatio from mIsPaintingSVGImageElement setting

Better graphics support, adds new tests, let's uplift to aurora.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 82 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/d183d71a7545
https://hg.mozilla.org/releases/mozilla-aurora/rev/9b3ee31983dc
https://hg.mozilla.org/releases/mozilla-aurora/rev/39f61988e8fb

	Sebastian Zartner [:sebo] Reporter
	Comment 83 • 8 years ago

Thank you for the uplift! I've updated the documentation at https://developer.mozilla.org/en-US/docs/Web/CSS/border-image#Browser_compatibility accordingly.

Sebastian

	Gerry Chang [:gchang]
	Comment 84 • 8 years ago

Since the fix of bug 619500 is back out, mark 48 as won't fix.

	Gerry Chang [:gchang]
	Comment 85 • 8 years ago

Comment on attachment 8769538 [details] [diff] [review]
(Uplift - aurora)(Part 2) Preserve scale ratio if SVG dosen't have viewport size and viewBox.

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

As mentioned in comment #81, better graphics support. Let's uplift to aurora.

	u459114
	Comment 86 • 8 years ago

Kevin, please add a patch to comment what FLAG_FORCE_UNIFORM_SCALING means(bug 619500 comment 107)

	Kevin Chen[:kechen] (UTC + 8) Assignee
	Comment 87 • 8 years ago

Hi Daniel,

I add a comment for FLAG_FORCE_UNIFORM_SCALING in this patch.

Can you help me to review it ?

Thank you.

	Daniel Holbert [:dholbert]
	Comment 88 • 8 years ago

Comment on attachment 8777637 [details] [diff] [review]
(Part 4) Add comment for FLAG_FORCE_UNIFORM_SCALING.

Hi Kevin! Sorry for the delay -- I was away for a few days and am now catching up on review backlog.

>+++ b/image/imgIContainer.idl
>+   * FLAG_FORCE_UNIFORM_SCALING: Force scaling this image uniformly. This flag
>+   * is for vector image only. A raster image should ignore this flag. This
>+   * flag prevents vector image with no fixed ratio from twisting itself after
>+   * the ratio is assigned.

This isn't clear enough -- it's kind of restating the name of the flag (first and 3rd sentence), and saying it's vector-specific.

In particular:
 * It's not clear enough (from this description) why/when this flag would actually take effect. Please clarify that a bit.
 * "for vector image only" -- VectorImage.* never use this flag at all. ClippedImage.cpp is the only place it's used (for *clipped* vector images).
 * "twisting" is probably the wrong word-choice here -- that implies "rotating", which isn't what you mean here. Maybe "distorting" would be a better choice?
 * "after the ratio is assigned" is unclear/confusing.

Please reword this to explain the scenario where this flag comes into play and why we need it a bit better.

	Daniel Holbert [:dholbert]
	Comment 89 • 8 years ago

Comment on attachment 8777637 [details] [diff] [review]
(Part 4) Add comment for FLAG_FORCE_UNIFORM_SCALING.

(er, sorry, meant to tag as r- for now; accidentally set r+)

	Kevin Chen[:kechen] (UTC + 8) Assignee
	Comment 90 • 8 years ago

Hi Daniel can you help me to review this again, thank you.

	Daniel Holbert [:dholbert]
	Comment 91 • 8 years ago

Comment on attachment 8781004 [details] [diff] [review]
(Part 4) Add comment for FLAG_FORCE_UNIFORM_SCALING.:

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

::: image/imgIContainer.idl
@@ +196,5 @@
>     * viewBox (if specified or implied by height/width attributes) exactly
>     * matches the viewport rectangle.
> +   *
> +   * FLAG_FORCE_UNIFORM_SCALING: Force scaling this image uniformly if
> +   * necessary. When painting border image, nsImageRenderer uses ClippedImage

This first sentence is too broad still. "Force scaling this image uniformly" isn't correct - there are lots of places where images are scaled (e.g. the imgIContainer::Draw API scales images, I'm pretty sure) -- but we don't check for this flag in most of those places.  We only pay attention to this flag in *one specific place* -- ClippedImage::OptimalSizeForDest.

How about something like the following text -- this makes more sense in my head, at least, and I think it's more precise about where this flag has an effect & why we need it (I think -- correct me if I'm misstating something):

 FLAG_FORCE_UNIFORM_SCALING: Signal to ClippedImage::OptimalSizeForDest that its returned size can only scale the image's size *uniformly* (by the same factor in each dimension).  We need this flag when painting border-image section with SVG image source-data, if the SVG image has no viewBox and no intrinsic size.  In such a case, we synthesize a viewport for the SVG image (a "window into SVG space") based on the border image area, and we need to be sure we don't subsequently scale that viewport in a way that distorts its contents by stretching them more in one dimension than the other.

	Kevin Chen[:kechen] (UTC + 8) Assignee
	Comment 92 • 8 years ago

Hi Daniel, very thank you for the modification. The version in your comment is more precise.

	Daniel Holbert [:dholbert]
	Comment 93 • 8 years ago

Comment on attachment 8783493 [details] [diff] [review]
(Part 4) Add comment for FLAG_FORCE_UNIFORM_SCALING

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

Thanks! r+

	Kevin Chen[:kechen] (UTC + 8) Assignee
	Comment 94 • 8 years ago

Please help me to checkin patch part 4, which only adds some comments to the modified codes.

	Pulsebot
	Comment 95 • 8 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec83da3f24a9
(Part 4) Add comment for FLAG_FORCE_UNIFORM_SCALING. r=dholbert

	Ryan VanderMeulen [:RyanVM]
	Comment 96 • 8 years ago
bugherder

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

	Seth Fowler [:seth] [:s2h]
	Comment 97 • 8 years ago

Comment on attachment 8767069 [details] [diff] [review]
(Part 1) Decouple SetImageOverridePreserveAspectRatio from mIsPaintingSVGImageElement setting

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

This needs another followup, I'm afraid.

::: layout/svg/SVGImageContext.h
@@ +33,4 @@
>      : mViewportSize(aViewportSize)
>      , mPreserveAspectRatio(aPreserveAspectRatio)
>      , mGlobalOpacity(aOpacity)
> +    , mIsPaintingSVGImageElement(aIsPaintingSVGImageElement)

So you added a new member variable...

@@ +55,4 @@
>    bool operator==(const SVGImageContext& aOther) const {
>      return mViewportSize == aOther.mViewportSize &&
>             mPreserveAspectRatio == aOther.mPreserveAspectRatio &&
>             mGlobalOpacity == aOther.mGlobalOpacity;

But you didn't add it to the operator==() implementation, so two SVGImageContexts with two different values for mIsPaintingSVGImageElement will compare equal. This is bad.

It's not strictly necessary, but you should've also mixed it into the hash value in Hash().

	Seth Fowler [:seth] [:s2h]
	Comment 98 • 8 years ago

Kevin, can you please write a followup to fix SVGImageContext::operator==() and SVGImageContext::Hash()? Whoever reviewed the patches in this bug should probably review the followup; I didn't read through the bug, but it looks like that might be Daniel.

	Daniel Holbert [:dholbert]
	Comment 99 • 8 years ago

Thanks for catching that, Seth! (Sorry for missing it in review)

	Kevin Chen[:kechen] (UTC + 8) Assignee
	Comment 100 • 8 years ago

Seth, thanks for the reminding! I've filed a followup bug for this and I will upload the patch later.

Bug number: bug 1298318

	Iulia Cristescu, QA [:JuliaC] (away, please needinfo? cornel.ionce@softvision.ro)
	Comment 101 • 8 years ago

Hello! I managed to reproduce this issue on 48.0a1 (2016-04-14). Also, I investigated it on 
- Windows 10 x64
- Ubuntu 14.04 x86
- Mac OS X 10.11
using  
- latest Nightly 51.0a1 (2016-09-08) 
- latest Aurora 50.0a2 (2016-09-08)
- 49.0 build2 (20160907153016)

Indeed, 60 and 300 SVGs are fixed. 
No-size SVGs and 10 SVGs are still problematic, but based on comment 51, these issues are separately filed.
These being said, I will set the corresponding flags.