700926 - Unify CSS image-value rendering

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Right now each site that uses CSS image values (url(), -moz-*-gradient(), -moz-image-rect(), -moz-element(), extensions that we'll add in the future) is implemented differently. Basically 'background-image' supports all of the above, and other sites (generated content, list-style-image, border-image, etc) support only url() images.

We need to generalize this code so that
a) All the properties that can take CSS image values can parse any CSS image value into an nsStyleImage value
b) The code in nsCSSRendering that renders nsStyleImages is used everywhere we have an nsStyleImage (nsBulletFrame, border-image, etc).

Maybe we should finish and land the border-image changes in bug 497995 first.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

To make this very precise, the properties that should accept any CSS image value are:
-- 'background-image'
-- 'border-image'
-- 'list-style-image'
-- 'content'

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Bug 597778 might be involved here too.

Comment 3

[Jean-Yves Perrier [:teoli]]

It looks like, according the CSS3 Images examples list, that the 'cursor' property should also accept CSS image value:
http://dev.w3.org/csswg/css3-images/#image

Comment 4

[nemo]

Well, that would be nice. Javascript animation is not as efficient as an animated gif for a cursor, since I have to update the cursor like 15 times per second at minimum.
I have no idea about efficiency of a SMIL SVG curosr though :)

Comment 5

[Alexei]

Please - add support moz-element for CSS "content".

Comment 6

[Nick Cameron [:nrc]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #1)
> To make this very precise, the properties that should accept any CSS image
> value are:
> -- 'background-image'
> -- 'border-image'
> -- 'list-style-image'
> -- 'content'

Do we really want border-image-source (and the border-image shorthand) to accept all image types? The spec says only URLs and 'none' (http://www.w3.org/TR/css3-background/#the-border-image-source).

Comment 7

[Florian Bender]

IMO, the spec should say "any valid image resource" ;). File a bug to the spec, maybe?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Where does the spec say "only URLs and 'none'"? The section on "computed value"? That's not controlling, probably just an oversight. 'Value' says "none | <image>", and that means any image value.

Comment 9

[Nick Cameron [:nrc]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8)
> Where does the spec say "only URLs and 'none'"? The section on "computed
> value"? That's not controlling, probably just an oversight. 'Value' says
> "none | <image>", and that means any image value.

Yeah, I was looking at the computed value. Thanks!

Comment 10

[Nick Cameron [:nrc]]

try push for refactor of image sizing code in nsImageRenderer: https://tbpl.mozilla.org/?tree=Try&rev=8a6249fb31ef

Comment 11

[Nick Cameron [:nrc]]

with some bugs fixed, a mostly green Try push for the resizing: https://tbpl.mozilla.org/?tree=Try&rev=2a90f21b04d5

Comment 12

[Nick Cameron [:nrc]]

And finally, spread across two runs, green-ness!
https://tbpl.mozilla.org/?tree=Try&rev=8e105264c4b6
https://tbpl.mozilla.org/?tree=Try&rev=088cd5c10961

Comment 13

[Nick Cameron [:nrc]]

Attachment: part 1: refactor sizing in nsImageRenderer to make it less background-specific and to closer match the algorithm from the spec

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 771488 [details] [diff] [review]
part 1: refactor sizing in nsImageRenderer to make it less background-specific and to closer match the algorithm from the spec

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

This looks great. I really like the way the code aligns with the spec.

::: layout/base/nsCSSRendering.cpp
@@ +2884,5 @@
>    return bgPositioningArea;
>  }
>  
> +// Apply the CSS image sizing algorithm as it applies to background images.
> +// See http://www.w3.org/TR/css3-background/#the-background-size .

Can you list the preconditions on aIntrinsicSize for this function to be called?

@@ +2898,5 @@
> +      aLayerSize.mWidthType == nsStyleBackground::Size::eCover
> +        ? nsImageRenderer::COVER
> +        : nsImageRenderer::CONTAIN;
> +    return nsImageRenderer::ComputeConstrainedSize(aBgPositioningArea,
> +                                                   aIntrinsicSize.mRatio,

Give aIntrinsicSize a getter method to get the ratio, and assert in that method that there is a valid ratio.

@@ +4419,5 @@
> +      double(mRatio.height) / mRatio.width);
> +    return nsSize(mWidth, height);
> +  } 
> +
> +  // mHasHeight == true

Assert this.

::: layout/base/nsCSSRendering.h
@@ +42,5 @@
> +    COVER
> +  };
> +
> +  // An nsAbstractSize represents a (possibly partially specified) size for use
> +  // in computing image sizes. Either or both of the wdith and height might be

width

@@ +45,5 @@
> +  // An nsAbstractSize represents a (possibly partially specified) size for use
> +  // in computing image sizes. Either or both of the wdith and height might be
> +  // given. A ratio of width to height may also be given. If we at least two
> +  // of these then we can compute a concrete size, that is a width and height.
> +  struct nsAbstractSize

I think this should be in the mozilla namespace instead of nested in this class.

How about calling it CSSIntrinsicSize?

@@ +55,5 @@
> +
> +    bool CanComputeConcreteSize() const
> +    {
> +      return (mHasWidth && mHasHeight) ||
> +             (HasRatio() && (mHasWidth || mHasHeight));

A cheekier way to write this would be
  return mHasWidth + mHasHeight + HasRatio() >= 2;

@@ +63,5 @@
> +    bool IsEmpty() const
> +    {
> +      return (mHasWidth && mWidth <= 0) ||
> +             (mHasHeight && mHeight <= 0) ||
> +             (HasRatio() && (mRatio.width == 0 || mRatio.height == 0)); 

This doesn't make sense since HasRatio requires both width and height to be zero. Should HasRatio use ||?

@@ +66,5 @@
> +             (mHasHeight && mHeight <= 0) ||
> +             (HasRatio() && (mRatio.width == 0 || mRatio.height == 0)); 
> +    }
> +
> +    nsSize ComputeConcreteSize() const;

Add a comment saying that CanComputeConcreteSize must be true when this is called?

Comment 15

[Nick Cameron [:nrc]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> Comment on attachment 771488 [details] [diff] [review]
> part 1: refactor sizing in nsImageRenderer to make it less
> background-specific and to closer match the algorithm from the spec
> 
> Review of attachment 771488 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks great. I really like the way the code aligns with the spec.
> 
> ::: layout/base/nsCSSRendering.cpp
> @@ +2884,5 @@
> >    return bgPositioningArea;
> >  }
> >  
> > +// Apply the CSS image sizing algorithm as it applies to background images.
> > +// See http://www.w3.org/TR/css3-background/#the-background-size .
> 
> Can you list the preconditions on aIntrinsicSize for this function to be
> called?
> 

I don't think there are any, it should be able to cope with any kind of intrinsic size.

> @@ +2898,5 @@
> > +      aLayerSize.mWidthType == nsStyleBackground::Size::eCover
> > +        ? nsImageRenderer::COVER
> > +        : nsImageRenderer::CONTAIN;
> > +    return nsImageRenderer::ComputeConstrainedSize(aBgPositioningArea,
> > +                                                   aIntrinsicSize.mRatio,
> 
> Give aIntrinsicSize a getter method to get the ratio, and assert in that
> method that there is a valid ratio.
> 

It is valid to take the ratio, even when HasRatio is false (so maybe HasValidRatio is a better name), I test if aIntrinsicSize.mRatio is valid in ComputeConstrainedSize and return 
aBgPositioningArea if not. I guess I could take aIntrinsicSize instead of the ratio and test HasRatio in ComputeConstrainedSize and then add the assertion. Do you have a preference between these options?

> @@ +45,5 @@
> > +  // An nsAbstractSize represents a (possibly partially specified) size for use
> > +  // in computing image sizes. Either or both of the wdith and height might be
> > +  // given. A ratio of width to height may also be given. If we at least two
> > +  // of these then we can compute a concrete size, that is a width and height.
> > +  struct nsAbstractSize
> 
> I think this should be in the mozilla namespace instead of nested in this
> class.
> 

We don't use mozilla namespace elsewhere in these files. Is it sill OK to use it? (I'm not sure about mixing old-style ns... with new style mozilla:: in a single file). Also, mozilla or mozilla:css?

> How about calling it CSSIntrinsicSize?

I use if for specified sizes as well as intrinsic sizes, so I prefer not to use this. How about CSSAbstractSize (although I don't much like 'abstract' because of the 'base class' meaning.

> @@ +63,5 @@
> > +    bool IsEmpty() const
> > +    {
> > +      return (mHasWidth && mWidth <= 0) ||
> > +             (mHasHeight && mHeight <= 0) ||
> > +             (HasRatio() && (mRatio.width == 0 || mRatio.height == 0)); 
> 
> This doesn't make sense since HasRatio requires both width and height to be
> zero. Should HasRatio use ||?
> 

I think HasRatio is not needed here and == should be <=. HasRatio used to use <=, but I changed it without changing this function.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Nick Cameron [:nrc] from comment #15)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> > Can you list the preconditions on aIntrinsicSize for this function to be
> > called?
> 
> I don't think there are any, it should be able to cope with any kind of
> intrinsic size.

OK

> It is valid to take the ratio, even when HasRatio is false (so maybe
> HasValidRatio is a better name), I test if aIntrinsicSize.mRatio is valid in
> ComputeConstrainedSize and return 
> aBgPositioningArea if not. I guess I could take aIntrinsicSize instead of
> the ratio and test HasRatio in ComputeConstrainedSize and then add the
> assertion. Do you have a preference between these options?

What you have is fine.

> > @@ +45,5 @@
> > > +  // An nsAbstractSize represents a (possibly partially specified) size for use
> > > +  // in computing image sizes. Either or both of the wdith and height might be
> > > +  // given. A ratio of width to height may also be given. If we at least two
> > > +  // of these then we can compute a concrete size, that is a width and height.
> > > +  struct nsAbstractSize
> > 
> > I think this should be in the mozilla namespace instead of nested in this
> > class.
> 
> We don't use mozilla namespace elsewhere in these files. Is it sill OK to
> use it? (I'm not sure about mixing old-style ns... with new style mozilla::
> in a single file). Also, mozilla or mozilla:css?

We should put new things into the mozilla namespace and opportunistically move existing nsThings to the mozilla namespace. Stuff in a namespace should not have "ns", classes outside a namespace should have "ns".

mozilla vs mozilla::css doesn't matter much. I'd go with "mozilla".

> > How about calling it CSSIntrinsicSize?
> 
> I use if for specified sizes as well as intrinsic sizes, so I prefer not to
> use this. How about CSSAbstractSize (although I don't much like 'abstract'
> because of the 'base class' meaning.

Right, I don't like 'abstract' either. How about CSSSizeOrRatio?

> > @@ +63,5 @@
> > > +    bool IsEmpty() const
> > > +    {
> > > +      return (mHasWidth && mWidth <= 0) ||
> > > +             (mHasHeight && mHeight <= 0) ||
> > > +             (HasRatio() && (mRatio.width == 0 || mRatio.height == 0)); 
> > 
> > This doesn't make sense since HasRatio requires both width and height to be
> > zero. Should HasRatio use ||?
> 
> I think HasRatio is not needed here and == should be <=. HasRatio used to
> use <=, but I changed it without changing this function.

OK.

Comment 17

[Nick Cameron [:nrc]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> (In reply to Nick Cameron [:nrc] from comment #15)
> > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> > > @@ +45,5 @@
> > > > +  // An nsAbstractSize represents a (possibly partially specified) size for use
> > > > +  // in computing image sizes. Either or both of the wdith and height might be
> > > > +  // given. A ratio of width to height may also be given. If we at least two
> > > > +  // of these then we can compute a concrete size, that is a width and height.
> > > > +  struct nsAbstractSize
> > > 
> > > I think this should be in the mozilla namespace instead of nested in this
> > > class.
> > 
> > We don't use mozilla namespace elsewhere in these files. Is it sill OK to
> > use it? (I'm not sure about mixing old-style ns... with new style mozilla::
> > in a single file). Also, mozilla or mozilla:css?
> 
> We should put new things into the mozilla namespace and opportunistically
> move existing nsThings to the mozilla namespace. Stuff in a namespace should
> not have "ns", classes outside a namespace should have "ns".
> 
> mozilla vs mozilla::css doesn't matter much. I'd go with "mozilla".
> 

Sorry, I meant is there a problem with using the two schemes in the same file?

Comment 18

[Nick Cameron [:nrc]]

Attachment: part 1: refactor sizing in nsImageRenderer to make it less background-specific and to closer match the algorithm from the spec

Comment 19

[Nick Cameron [:nrc]]

Attachment: part 2: refactor background image drawing a little

More preparatory stuff, these changes make the following patches possible/nicer, they are not really necessary for their own sake.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Nick Cameron [:nrc] from comment #17)
> Sorry, I meant is there a problem with using the two schemes in the same
> file?

No.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 772404 [details] [diff] [review]
part 2: refactor background image drawing a little

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

::: layout/style/nsStyleStruct.h
@@ +264,5 @@
>    }
> +  /**
> +   * Ensure we get invalidated for loads and animations of the image.
> +   */
> +  void AssociateToFrame(nsPresContext* aPresContext, nsIFrame* aForFrame) const;

nsStyleImage should be stateless, and this looks like a stateful method, so I would prefer this being a method on the frame that takes an nsStyleImage* parameter. And call it AssociateWithFrame.

Comment 22

[Nick Cameron [:nrc]]

Attachment: part 2: refactor background image drawing a little

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 772500 [details] [diff] [review]
part 2: refactor background image drawing a little

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

::: layout/generic/nsIFrame.h
@@ +1362,5 @@
> +    mozilla::css::ImageLoader* loader =
> +      aPresContext->Document()->StyleImageLoader();
> +
> +    // If this fails there's not much we can do ...
> +    loader->AssociateRequestToFrame(req, this);    

remove trailing whitespace

Comment 24

[Nick Cameron [:nrc]]

Try push for first two patches: https://tbpl.mozilla.org/?tree=Try&rev=c61b3cb78dca

Comment 25

[Nick Cameron [:nrc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/783e53214215
https://hg.mozilla.org/integration/mozilla-inbound/rev/612b03080cb0

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/783e53214215
https://hg.mozilla.org/mozilla-central/rev/612b03080cb0

Comment 27

[Florian Bender]

Why is this bug leave-open? What's left to do here? This bug blocks bug 801844 which in turn blocks bug 703217 and bug 790640 (which would really simplify developers' lifes).

Comment 28

[Nick Cameron [:nrc]]

(In reply to Florian Bender from comment #27)
> Why is this bug leave-open? What's left to do here? This bug blocks bug
> 801844 which in turn blocks bug 703217 and bug 790640 (which would really
> simplify developers' lifes).

Most of this is left to do, although I have patches for a good chunk of it. The patches landed so far are really just preparatory stuff. I have been really busy with graphics stuff so haven't had a chance to look at this. Hopefully in the next couple of weeks I'll get back to it.

Bug 801844 is a tracking bug for anything to do with CSS images. This bug should be thought of as a strong blocker for it. In particular, I'm pretty sure this bug doesn't prevent work on 703217 or 790640.

Comment 29

[Florian Bender]

(In reply to Nick Cameron [:nrc] from comment #28)
> I'm pretty sure this bug doesn't prevent work on 703217 or 790640.

I was basing my comment on bug 703217 comment 6. Either way, the naming and dependency tree is a bit confusing to me, so I thought I'd ask. ;) Thanks for the info!

Comment 30

[Florian Bender]

Nick, any ETA on this?

Comment 31

[Nick Cameron [:nrc]]

(In reply to Florian Bender from comment #30)
> Nick, any ETA on this?

This is mostly done, but I'm not sure how much more work there is to go - maybe a week or two. That probably depends to some extent on how badly it's rotted. It has been pretty low priority for me so I haven't done anything for ages. If there is something making it a higher priority, I can try and make time for it.

Comment 32

[Florian Bender]

Well, this bug eats at least a few poitns on css3test.com/#css3-images (Bug 913153) because gradients don't work in list-style-image, border-image, cursor, and content. This test is part of many browser comparisons (including Tom's Hardware Web Browser Grand Prix) and thus loses us at least PR.

On a personal note, I'd relly like to use gradients in border-image because it allows some nice and easy decoration without the need for images, and Firefox is stopping me here. So another point is developer frustration. 

So please, would be nice if you could spend some time on it and get this into Fx 29!

Comment 33

[Florian Bender]

… and it also blocks some other work that would be really nice to have (like -moz-element and mask-image*).

Comment 34

[Nick Cameron [:nrc]]

The following slew of patches implement css images for border-image{-source} and do a bit of refactoring along the way which the other uses of css images make use of. After these patches you can use gradients and -moz-element with border-image and all the various width, slice, etc. properties should work properly.

Comment 35

[Nick Cameron [:nrc]]

Attachment: vol 2 part1: Use nsStyleImage and nsRenderingContext to draw border-images

Comment 36

[Nick Cameron [:nrc]]

Attachment: vol 2 part 2: Fix border-image-source gradient with slice

Comment 37

[Nick Cameron [:nrc]]

Attachment: vol 2 part 3 border-image-source css parsing

Comment 38

[Nick Cameron [:nrc]]

Attachment: vol 2 part4: Tests for border-image-source gradient

Comment 39

[Nick Cameron [:nrc]]

Attachment: vol 2 part 5: Python script to generate reftest reference pages

Comment 40

[Nick Cameron [:nrc]]

Attachment: vol 2 part 6: Change DrawPaintServer to DrawableFromPaintServer

Comment 41

[Nick Cameron [:nrc]]

Attachment: vol 2 part 7: Support -moz-element for border-image-source

Comment 42

[Nick Cameron [:nrc]]

Attachment: vol 2 part 8: Tests for border-image-source/-moz-element

Comment 43

[Nick Cameron [:nrc]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=be1bd55d9be2

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8361437 [details] [diff] [review]
vol 2 part1: Use nsStyleImage and nsRenderingContext to draw border-images

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

::: layout/base/nsCSSRendering.cpp
@@ +4549,5 @@
> +static nsRect
> +ComputeTile(const nsRect&        aFill,
> +            uint8_t              aHFill,
> +            uint8_t              aVFill,
> +            const nsSize&        aUnitSize)

Give this function a comment explaining what its parameters mean, at least.

Might as well fix indent too.

@@ +4594,5 @@
> +static bool
> +RequiresScaling(const nsRect&        aFill,
> +                uint8_t              aHFill,
> +                uint8_t              aVFill,
> +                const nsSize&        aUnitSize)

Same here

::: layout/generic/nsFrame.cpp
@@ +796,1 @@
>      : nullptr;

Add a method nsStyleBorder::GetBorderImageRequest to do this for you, so you can call it twice here.

::: layout/style/nsRuleNode.cpp
@@ +1084,5 @@
>                            bool& aCanStoreInRuleTree)
>  {
> +  if (aValue.GetUnit() == eCSSUnit_Null) {
> +    return;
> +  }

Shouldn't this be after aResult.SetNull()?

::: layout/style/nsStyleStruct.h
@@ +290,5 @@
>  private:
>    void DoCopy(const nsStyleImage& aOther);
>  
> +  // Cache for border-image painting.
> +  nsCOMArray<imgIContainer> mSubImages;

nsCOMArray is deprecated. Use nsTArray<nsCOMPtr<imgIContainer> >.

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8361439 [details] [diff] [review]
vol 2 part 2: Fix border-image-source gradient with slice

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

::: layout/base/nsCSSRendering.cpp
@@ +4614,5 @@
>  nsImageRenderer::DrawBorderImageComponent(nsPresContext*       aPresContext,
>                                            nsRenderingContext&  aRenderingContext,
>                                            const nsRect&        aDirtyRect,
>                                            const nsRect&        aFill,
> +                                          const nsRect&        aSrc,

I'm a bit confused. If these must be integer CSS pixels (and I think they must be), why are we changing this to appunits?

::: layout/base/nsCSSRendering.h
@@ +171,5 @@
>              nsRenderingContext&  aRenderingContext,
>              const nsRect&        aDirtyRect,
>              const nsRect&        aFill,
> +            const nsRect&        aDest,
> +            const nsRect&        aSrc);

You need to document aSrc

@@ +348,5 @@
>     * Render a gradient for an element.
> +   * aDest is the rect for a single tile of the gradient on the destination.
> +   * aFill is the rect on the destination to be covered by repeated tiling of
> +   * the gradient.
> +   * aSrc is the part of the gradient to be rendered into a tile.

If it's a different size from aDest, do we scale? Looks like it. Document this.

Comment 46

[Nick Cameron [:nrc]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away January 20-24) from comment #44)
> Comment on attachment 8361437 [details] [diff] [review]
> vol 2 part1: Use nsStyleImage and nsRenderingContext to draw border-images
> 
> Review of attachment 8361437 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/nsRuleNode.cpp
> @@ +1084,5 @@
> >                            bool& aCanStoreInRuleTree)
> >  {
> > +  if (aValue.GetUnit() == eCSSUnit_Null) {
> > +    return;
> > +  }
> 
> Shouldn't this be after aResult.SetNull()?
> 

No, it is part of the start struct optimisation that setting a property to eCSSUnit_Null in Compute*Data does not change anything. See http://dxr.mozilla.org/mozilla-central/source/layout/style/test/test_compute_data_with_start_struct.html#24. I can add a comment to explain.

Comment 47

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No problem, just ignore that comment.

Comment 48

[Nick Cameron [:nrc]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away January 20-24) from comment #44)
> Comment on attachment 8361437 [details] [diff] [review]
> vol 2 part1: Use nsStyleImage and nsRenderingContext to draw border-images
> 
> Review of attachment 8361437 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/nsStyleStruct.h
> @@ +290,5 @@
> >  private:
> >    void DoCopy(const nsStyleImage& aOther);
> >  
> > +  // Cache for border-image painting.
> > +  nsCOMArray<imgIContainer> mSubImages;
> 
> nsCOMArray is deprecated. Use nsTArray<nsCOMPtr<imgIContainer> >.

I just moved this from here http://dxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.h#1003

nsCOMArray is still used all over layout and the users of mSubImages still expect an nsCOMArray. Changing to an nsTArray I get compile errors because it is missing Count and ReplaceObjectAt methods. So, I prefer to leave it as is, if that is OK.

Comment 49

[Nick Cameron [:nrc]]

Attachment: vol 2 part 2: Fix border-image-source gradient with slice

Updated to pass css pixels for src rect rather than app units

Comment 50

[Nick Cameron [:nrc]]

Attachment: vol 3 part 1: Factor out GetDefaultSrcRect()

vol 3 is list-style-image

Comment 51

[Nick Cameron [:nrc]]

Attachment: vol 3 part 2: list-style-image

Comment 52

[Nick Cameron [:nrc]]

Attachment: vol 3 part 3: list-style-image tests

Comment 53

[Nick Cameron [:nrc]]

try push for border-image: https://tbpl.mozilla.org/?tree=Try&rev=06154841651b

Comment 54

[Nick Cameron [:nrc]]

Comment on attachment 8364081 [details] [diff] [review]
vol 3 part 2: list-style-image

Not quite ready for review. Although it looked green on try, it introduced intermittent orange which I have only managed to partially address so far.

Comment 55

[Nick Cameron [:nrc]]

Attachment: vol 3 part 2: list-style-image

This part of the refactoring changes list-style-image to use nsStyleImage rather than do its own image handling. One problem I ran into was I had to request sync image decoding in order to avoid intermittent failure on an existing test where the image was decoded in time for rendering. I wanted to check that this is the right thing to do.

Now I have another, rarer intermittent failure which so far I have only observed on Android and on Try (can't reproduce locally). I'm not sure if it is Android only or only less common on desktop (https://tbpl.mozilla.org/?tree=Try&rev=f86e078c2d9f). The failure is again that the image used for list-style-image is failing to load. But I have no idea why. Is there anything I've done wrong in this patch which might cause this behaviour?

Thanks!

Comment 56

[Timothy Nikkel (:tnikkel)]

(In reply to Nick Cameron [:nrc] from comment #55)
> Created attachment 8365753 [details] [diff] [review]
> vol 3 part 2: list-style-image
> 
> This part of the refactoring changes list-style-image to use nsStyleImage
> rather than do its own image handling. One problem I ran into was I had to
> request sync image decoding in order to avoid intermittent failure on an
> existing test where the image was decoded in time for rendering. I wanted to
> check that this is the right thing to do.

reftests should be passing the sync decode flag to drawWindow, which should show up on the display list builder. So the proper thing to do is to make sure that flag is getting correctly propagated, this is what we do for other image types.

Comment 57

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8364079 [details] [diff] [review]
vol 2 part 2: Fix border-image-source gradient with slice

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

::: layout/base/nsCSSRendering.h
@@ +172,5 @@
>              nsRenderingContext&  aRenderingContext,
>              const nsRect&        aDirtyRect,
>              const nsRect&        aFill,
> +            const nsRect&        aDest,
> +            const nsIntRect&     aSrc);

I think you can use the new shiny CSSIntRect here. More self-descriptive.

@@ +362,5 @@
>                              nsStyleGradient* aGradient,
>                              const nsRect& aDirtyRect,
> +                            const nsRect& aDest,
> +                            const nsRect& aFill,
> +                            const nsIntRect& aSrc,

Same here

Comment 58

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8365753 [details] [diff] [review]
vol 3 part 2: list-style-image

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

Looks good!

::: layout/generic/nsBulletFrame.cpp
@@ +300,4 @@
>          return;
>        }
> +
> +      mRenderer->SetFlags(aFlags | nsImageRenderer::FLAG_SYNC_DECODE_IMAGES);

I concur with Timothy that this shouldn't be necessary.

Comment 59

[Nick Cameron [:nrc]]

border-image:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1502afcd3a6f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2497cf461fb2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cd45d0bc480e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5afef9152eba
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fe6340c672a9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7b8826cd919f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c4af6c845f68
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/13a5fb1e8525

Comment 60

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1502afcd3a6f
https://hg.mozilla.org/mozilla-central/rev/2497cf461fb2
https://hg.mozilla.org/mozilla-central/rev/cd45d0bc480e
https://hg.mozilla.org/mozilla-central/rev/5afef9152eba
https://hg.mozilla.org/mozilla-central/rev/fe6340c672a9
https://hg.mozilla.org/mozilla-central/rev/7b8826cd919f
https://hg.mozilla.org/mozilla-central/rev/c4af6c845f68
https://hg.mozilla.org/mozilla-central/rev/13a5fb1e8525

Comment 61

[Sebastian Zartner [:sebo]]

What happened to the patches for 'list-style-image'?

Also patches for 'cursor' and 'content' are currently missing.

Sebastian

Comment 62

[Sylvestre Ledru [:Sylvestre]]

Nick, do you think we could now close this bug? Thanks

Comment 63

[nemo]

FWIW, WRT closing. Bug #492237 which was the whole reason I took an interest in this bug, is still open…

Comment 64

[Nick Cameron [:nrc]]

No idea here either, also been years. Maybe Matt or Cameron know?

Comment 65

[Cameron McCormack (:heycam)]

The remainder of this work got done in other bugs as part of stylo.  Although cursors now do use nsStyleImageRequest, that's not going to make animated cursors work, which would need changes to all of the nsWindow::SetCursor implementations I guess.

Comment 66

[Estelle Weyl]

<image> may also be supported in @counter-style symbols, but is a feature that is at at risk. 
bug 1024179

See https://developer.mozilla.org/en-US/docs/Web/CSS/@counter-style and https://developer.mozilla.org/en-US/docs/Web/CSS/@counter-style/symbols and