Unify CSS image-value rendering

NEW
Assigned to

Status

()

Core
Layout
6 years ago
9 months ago

People

(Reporter: roc, Assigned: nrc)

Tracking

(Depends on: 2 bugs, Blocks: 6 bugs, {dev-doc-needed})

Trunk
x86_64
Windows 7
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [leave-open] )

Attachments

(14 attachments, 3 obsolete attachments)

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

Comment 4

5 years ago
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

5 years ago
Please - add support moz-element for CSS "content".

Updated

5 years ago
Blocks: 711326
Blocks: 801844
Assignee: nobody → seth
(Assignee)

Updated

4 years ago
Blocks: 877294
(Assignee)

Updated

4 years ago
Blocks: 686281
(Assignee)

Updated

4 years ago
Assignee: seth → ncameron
(Assignee)

Comment 6

4 years ago
(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)

Comment 7

4 years ago
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)
(Assignee)

Comment 9

4 years ago
(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!
(Assignee)

Comment 10

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

Comment 11

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

Comment 12

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

Comment 13

4 years ago
Created 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
Attachment #771488 - Flags: review?(roc)
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?
(Assignee)

Comment 15

4 years ago
(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.
(Assignee)

Updated

4 years ago
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)
(Assignee)

Comment 17

4 years ago
(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?
(Assignee)

Comment 18

4 years ago
Created attachment 772401 [details] [diff] [review]
part 1: refactor sizing in nsImageRenderer to make it less background-specific and to closer match the algorithm from the spec
Attachment #771488 - Attachment is obsolete: true
Attachment #771488 - Flags: review?(roc)
Attachment #772401 - Flags: review?(roc)
(Assignee)

Comment 19

4 years ago
Created attachment 772404 [details] [diff] [review]
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.
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.
(Assignee)

Comment 22

4 years ago
Created attachment 772500 [details] [diff] [review]
part 2: refactor background image drawing a little
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+
(Assignee)

Comment 24

4 years ago
Try push for first two patches: https://tbpl.mozilla.org/?tree=Try&rev=c61b3cb78dca
(Assignee)

Updated

4 years ago
Whiteboard: [leave-open]
(Assignee)

Comment 25

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/783e53214215
https://hg.mozilla.org/integration/mozilla-inbound/rev/612b03080cb0
https://hg.mozilla.org/mozilla-central/rev/783e53214215
https://hg.mozilla.org/mozilla-central/rev/612b03080cb0

Updated

4 years ago
Depends on: 897787

Updated

4 years ago
Depends on: 898596
Depends on: 900214

Comment 27

4 years ago
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).
(Assignee)

Comment 28

4 years ago
(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

4 years ago
(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]

Updated

4 years ago
Blocks: 913153

Comment 30

3 years ago
Nick, any ETA on this?
Flags: needinfo?(ncameron)
(Assignee)

Comment 31

3 years ago
(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)

Comment 32

3 years ago
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

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

Comment 34

3 years ago
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.
(Assignee)

Comment 35

3 years ago
Created attachment 8361437 [details] [diff] [review]
vol 2 part1: Use nsStyleImage and nsRenderingContext to draw border-images
Attachment #8361437 - Flags: review?(roc)
(Assignee)

Comment 36

3 years ago
Created attachment 8361439 [details] [diff] [review]
vol 2 part 2: Fix border-image-source gradient with slice
Attachment #8361439 - Flags: review?(roc)
(Assignee)

Comment 37

3 years ago
Created attachment 8361440 [details] [diff] [review]
vol 2 part 3 border-image-source css parsing
Attachment #8361440 - Flags: review?(roc)
(Assignee)

Comment 38

3 years ago
Created attachment 8361442 [details] [diff] [review]
vol 2 part4: Tests for border-image-source gradient
Attachment #8361442 - Flags: review?(roc)
(Assignee)

Comment 39

3 years ago
Created attachment 8361443 [details] [diff] [review]
vol 2 part 5: Python script to generate reftest reference pages
Attachment #8361443 - Flags: review?(roc)
(Assignee)

Comment 40

3 years ago
Created attachment 8361444 [details] [diff] [review]
vol 2 part 6: Change DrawPaintServer to DrawableFromPaintServer
Attachment #8361444 - Flags: review?(roc)
(Assignee)

Comment 41

3 years ago
Created attachment 8361445 [details] [diff] [review]
vol 2 part 7: Support -moz-element for border-image-source
Attachment #8361445 - Flags: review?(roc)
(Assignee)

Comment 42

3 years ago
Created attachment 8361446 [details] [diff] [review]
vol 2 part 8: Tests for border-image-source/-moz-element
Attachment #8361446 - Flags: review?(roc)
(Assignee)

Comment 43

3 years ago
Try push: https://tbpl.mozilla.org/?tree=Try&rev=be1bd55d9be2
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+
(Assignee)

Comment 46

3 years ago
(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+
(Assignee)

Comment 48

3 years ago
(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.
(Assignee)

Comment 49

3 years ago
Created attachment 8364079 [details] [diff] [review]
vol 2 part 2: Fix border-image-source gradient with slice

Updated to pass css pixels for src rect rather than app units
Attachment #8361439 - Attachment is obsolete: true
Attachment #8364079 - Flags: review?(roc)
(Assignee)

Comment 50

3 years ago
Created attachment 8364080 [details] [diff] [review]
vol 3 part 1: Factor out GetDefaultSrcRect()

vol 3 is list-style-image
Attachment #8364080 - Flags: review?(roc)
(Assignee)

Comment 51

3 years ago
Created attachment 8364081 [details] [diff] [review]
vol 3 part 2: list-style-image
Attachment #8364081 - Flags: review?(roc)
(Assignee)

Comment 52

3 years ago
Created attachment 8364082 [details] [diff] [review]
vol 3 part 3: list-style-image tests
Attachment #8364082 - Flags: review?(roc)
(Assignee)

Comment 53

3 years ago
try push for border-image: https://tbpl.mozilla.org/?tree=Try&rev=06154841651b
(Assignee)

Comment 54

3 years ago
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)
(Assignee)

Comment 55

3 years ago
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.

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+
(Assignee)

Comment 59

3 years ago
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
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

Updated

3 years ago
Blocks: 950073
Whiteboard: [leave-open] → [leave-open] p=0

Updated

3 years ago
No longer blocks: 950073
Whiteboard: [leave-open] p=0 → [leave-open]

Updated

3 years ago
Depends on: 993325
What happened to the patches for 'list-style-image'?

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

Sebastian

Updated

a year ago
Depends on: 1264597
Blocks: 1285811
You need to log in before you can comment on or make changes to this bug.