Closed Bug 700926 Opened 13 years ago Closed 6 years ago

Unify CSS image-value rendering

Categories

(Core :: Layout, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: roc, Assigned: nrc)

References

(Depends on 2 open bugs, Blocks 6 open bugs)

Details

(Keywords: dev-doc-needed)

Attachments

(14 files, 3 obsolete files)

32.23 KB, patch
roc
: review+
Details | Diff | Splinter Review
14.92 KB, patch
roc
: review+
Details | Diff | Splinter Review
38.64 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.73 KB, patch
roc
: review+
Details | Diff | Splinter Review
214.90 KB, patch
roc
: review+
Details | Diff | Splinter Review
14.15 KB, patch
roc
: review+
Details | Diff | Splinter Review
7.78 KB, patch
roc
: review+
Details | Diff | Splinter Review
9.55 KB, patch
roc
: review+
Details | Diff | Splinter Review
58.01 KB, patch
roc
: review+
Details | Diff | Splinter Review
26.32 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.21 KB, patch
roc
: review+
Details | Diff | Splinter Review
26.74 KB, patch
Details | Diff | Splinter Review
4.23 KB, patch
roc
: review+
Details | Diff | Splinter Review
27.64 KB, patch
seth
: feedback+
Details | Diff | Splinter Review
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.
Summary: Unify CSS image0-value rendering → Unify CSS image-value rendering
To make this very precise, the properties that should accept any CSS image value are:
-- 'background-image'
-- 'border-image'
-- 'list-style-image'
-- 'content'
Bug 597778 might be involved here too.
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
Keywords: dev-doc-needed
Blocks: 709587
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 :)
Please - add support moz-element for CSS "content".
Blocks: 711326
Blocks: 801844
Assignee: nobody → seth
Blocks: 877294
Blocks: 686281
Assignee: seth → ncameron
(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).
Flags: needinfo?(roc)
IMO, the spec should say "any valid image resource" ;). File a bug to the spec, maybe?
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.
Flags: needinfo?(roc)
(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!
try push for refactor of image sizing code in nsImageRenderer: https://tbpl.mozilla.org/?tree=Try&rev=8a6249fb31ef
with some bugs fixed, a mostly green Try push for the resizing: https://tbpl.mozilla.org/?tree=Try&rev=2a90f21b04d5
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?
(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.
Flags: needinfo?(roc)
(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.
Flags: needinfo?(roc)
(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?
More preparatory stuff, these changes make the following patches possible/nicer, they are not really necessary for their own sake.
Attachment #772404 - Flags: review?(roc)
(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.
Attachment #772401 - Flags: review?(roc) → review+
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.
Attachment #772404 - Attachment is obsolete: true
Attachment #772404 - Flags: review?(roc)
Attachment #772500 - Flags: review?(roc)
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
Attachment #772500 - Flags: review?(roc) → review+
Try push for first two patches: https://tbpl.mozilla.org/?tree=Try&rev=c61b3cb78dca
Whiteboard: [leave-open]
Depends on: 897787
Depends on: 898596
Depends on: 900214
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).
(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.
(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!
Whiteboard: [leave-open] → [leave-open][Australis:P?][Australis:M?]
Whiteboard: [leave-open][Australis:P?][Australis:M?] → [leave-open]
Blocks: css3test
Nick, any ETA on this?
Flags: needinfo?(ncameron)
(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.
Flags: needinfo?(ncameron)
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!
… and it also blocks some other work that would be really nice to have (like -moz-element and mask-image*).
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.
Attachment #8361439 - Flags: review?(roc)
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> >.
Attachment #8361437 - Flags: review?(roc) → review+
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.
Attachment #8361439 - Flags: review?(roc) → review-
Attachment #8361440 - Flags: review?(roc) → review+
Attachment #8361442 - Flags: review?(roc) → review+
Attachment #8361443 - Flags: review?(roc) → review+
Attachment #8361444 - Flags: review?(roc) → review+
(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.
No problem, just ignore that comment.
Attachment #8361445 - Flags: review?(roc) → review+
Attachment #8361446 - Flags: review?(roc) → review+
(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.
Updated to pass css pixels for src rect rather than app units
Attachment #8361439 - Attachment is obsolete: true
Attachment #8364079 - Flags: review?(roc)
vol 3 is list-style-image
Attachment #8364080 - Flags: review?(roc)
Attachment #8364081 - Flags: review?(roc)
Attachment #8364082 - Flags: review?(roc)
try push for border-image: https://tbpl.mozilla.org/?tree=Try&rev=06154841651b
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.
Attachment #8364081 - Flags: review?(roc)
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!
Attachment #8365753 - Flags: feedback?(seth)
(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 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
Attachment #8364079 - Flags: review?(roc) → review+
Attachment #8364080 - Flags: review?(roc) → review+
Attachment #8364082 - Flags: review?(roc) → review+
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.
Attachment #8365753 - Flags: feedback?(seth) → feedback+
Whiteboard: [leave-open] → [leave-open] p=0
No longer blocks: fxdesktopbacklog
Whiteboard: [leave-open] p=0 → [leave-open]
Depends on: 993325
What happened to the patches for 'list-style-image'?

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

Sebastian
Depends on: 1264597
Nick, do you think we could now close this bug? Thanks
Flags: needinfo?(ncameron)
Keywords: leave-open
Whiteboard: [leave-open]
FWIW, WRT closing. Bug #492237 which was the whole reason I took an interest in this bug, is still open…
Blocks: css-images-3
No idea here either, also been years. Maybe Matt or Cameron know?
Flags: needinfo?(ncameron)
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(cam)
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.
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(cam)
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
<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
You need to log in before you can comment on or make changes to this bug.