Last Comment Bug 700926 - Unify CSS image-value rendering
: Unify CSS image-value rendering
Status: NEW
[leave-open]
: dev-doc-needed
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal with 13 votes (vote)
: ---
Assigned To: Nick Cameron [:nrc]
:
Mentors:
Depends on: 993325 1264597 897787 898596 900214
Blocks: 507052 711326 801844 877294 css3test 1285811 686281 709587
  Show dependency treegraph
 
Reported: 2011-11-08 20:56 PST by Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
Modified: 2016-07-10 06:56 PDT (History)
27 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1: refactor sizing in nsImageRenderer to make it less background-specific and to closer match the algorithm from the spec (31.84 KB, patch)
2013-07-04 14:43 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Splinter Review
part 1: refactor sizing in nsImageRenderer to make it less background-specific and to closer match the algorithm from the spec (32.23 KB, patch)
2013-07-08 17:14 PDT, Nick Cameron [:nrc]
roc: review+
Details | Diff | Splinter Review
part 2: refactor background image drawing a little (13.69 KB, patch)
2013-07-08 17:16 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Splinter Review
part 2: refactor background image drawing a little (14.92 KB, patch)
2013-07-08 22:14 PDT, Nick Cameron [:nrc]
roc: review+
Details | Diff | Splinter Review
vol 2 part1: Use nsStyleImage and nsRenderingContext to draw border-images (38.64 KB, patch)
2014-01-16 18:15 PST, Nick Cameron [:nrc]
roc: review+
Details | Diff | Splinter Review
vol 2 part 2: Fix border-image-source gradient with slice (24.91 KB, patch)
2014-01-16 18:16 PST, Nick Cameron [:nrc]
roc: review-
Details | Diff | Splinter Review
vol 2 part 3 border-image-source css parsing (1.73 KB, patch)
2014-01-16 18:17 PST, Nick Cameron [:nrc]
roc: review+
Details | Diff | Splinter Review
vol 2 part4: Tests for border-image-source gradient (214.90 KB, patch)
2014-01-16 18:18 PST, Nick Cameron [:nrc]
roc: review+
Details | Diff | Splinter Review
vol 2 part 5: Python script to generate reftest reference pages (14.15 KB, patch)
2014-01-16 18:18 PST, Nick Cameron [:nrc]
roc: review+
Details | Diff | Splinter Review
vol 2 part 6: Change DrawPaintServer to DrawableFromPaintServer (7.78 KB, patch)
2014-01-16 18:19 PST, Nick Cameron [:nrc]
roc: review+
Details | Diff | Splinter Review
vol 2 part 7: Support -moz-element for border-image-source (9.55 KB, patch)
2014-01-16 18:20 PST, Nick Cameron [:nrc]
roc: review+
Details | Diff | Splinter Review
vol 2 part 8: Tests for border-image-source/-moz-element (58.01 KB, patch)
2014-01-16 18:21 PST, Nick Cameron [:nrc]
roc: review+
Details | Diff | Splinter Review
vol 2 part 2: Fix border-image-source gradient with slice (26.32 KB, patch)
2014-01-22 17:39 PST, Nick Cameron [:nrc]
roc: review+
Details | Diff | Splinter Review
vol 3 part 1: Factor out GetDefaultSrcRect() (2.21 KB, patch)
2014-01-22 17:40 PST, Nick Cameron [:nrc]
roc: review+
Details | Diff | Splinter Review
vol 3 part 2: list-style-image (26.74 KB, patch)
2014-01-22 17:41 PST, Nick Cameron [:nrc]
no flags Details | Diff | Splinter Review
vol 3 part 3: list-style-image tests (4.23 KB, patch)
2014-01-22 17:42 PST, Nick Cameron [:nrc]
roc: review+
Details | Diff | Splinter Review
vol 3 part 2: list-style-image (27.64 KB, patch)
2014-01-26 18:41 PST, Nick Cameron [:nrc]
seth.bugzilla: feedback+
Details | Diff | Splinter Review

Description Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-11-08 20:56:15 PST
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) (Exited; email my personal email if necessary) 2011-11-09 15:39:15 PST
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) (Exited; email my personal email if necessary) 2011-11-09 16:56:06 PST
Bug 597778 might be involved here too.
Comment 3 Jean-Yves Perrier [:teoli] 2011-12-09 01:18:41 PST
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 2011-12-11 11:52:57 PST
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 2011-12-15 21:56:39 PST
Please - add support moz-element for CSS "content".
Comment 6 Nick Cameron [:nrc] 2013-06-17 02:33:55 PDT
(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 2013-06-17 02:43:34 PDT
IMO, the spec should say "any valid image resource" ;). File a bug to the spec, maybe?
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-06-17 02:55:03 PDT
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] 2013-06-17 03:12:28 PDT
(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] 2013-07-02 19:03:35 PDT
try push for refactor of image sizing code in nsImageRenderer: https://tbpl.mozilla.org/?tree=Try&rev=8a6249fb31ef
Comment 11 Nick Cameron [:nrc] 2013-07-03 22:51:53 PDT
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] 2013-07-04 14:42:20 PDT
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] 2013-07-04 14:43:43 PDT
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
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-07-04 16:38:45 PDT
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] 2013-07-07 19:25:08 PDT
(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) (Exited; email my personal email if necessary) 2013-07-08 02:52:57 PDT
(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] 2013-07-08 17:13:20 PDT
(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] 2013-07-08 17:14:00 PDT
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
Comment 19 Nick Cameron [:nrc] 2013-07-08 17:16:36 PDT
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.
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-07-08 17:47:14 PDT
(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) (Exited; email my personal email if necessary) 2013-07-08 17:54:34 PDT
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] 2013-07-08 22:14:27 PDT
Created attachment 772500 [details] [diff] [review]
part 2: refactor background image drawing a little
Comment 23 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-07-08 23:32:01 PDT
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] 2013-07-19 01:37:57 PDT
Try push for first two patches: https://tbpl.mozilla.org/?tree=Try&rev=c61b3cb78dca
Comment 27 Florian Bender 2013-08-10 11:20:43 PDT
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] 2013-08-10 13:49:27 PDT
(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 2013-08-10 15:38:49 PDT
(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 2013-12-06 14:59:44 PST
Nick, any ETA on this?
Comment 31 Nick Cameron [:nrc] 2013-12-06 19:25:57 PST
(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 2013-12-07 07:46:27 PST
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 2013-12-07 07:48:20 PST
… 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] 2014-01-16 18:13:53 PST
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] 2014-01-16 18:15:57 PST
Created attachment 8361437 [details] [diff] [review]
vol 2 part1: Use nsStyleImage and nsRenderingContext to draw border-images
Comment 36 Nick Cameron [:nrc] 2014-01-16 18:16:48 PST
Created attachment 8361439 [details] [diff] [review]
vol 2 part 2: Fix border-image-source gradient with slice
Comment 37 Nick Cameron [:nrc] 2014-01-16 18:17:28 PST
Created attachment 8361440 [details] [diff] [review]
vol 2 part 3 border-image-source css parsing
Comment 38 Nick Cameron [:nrc] 2014-01-16 18:18:10 PST
Created attachment 8361442 [details] [diff] [review]
vol 2 part4: Tests for border-image-source gradient
Comment 39 Nick Cameron [:nrc] 2014-01-16 18:18:52 PST
Created attachment 8361443 [details] [diff] [review]
vol 2 part 5: Python script to generate reftest reference pages
Comment 40 Nick Cameron [:nrc] 2014-01-16 18:19:30 PST
Created attachment 8361444 [details] [diff] [review]
vol 2 part 6: Change DrawPaintServer to DrawableFromPaintServer
Comment 41 Nick Cameron [:nrc] 2014-01-16 18:20:24 PST
Created attachment 8361445 [details] [diff] [review]
vol 2 part 7: Support -moz-element for border-image-source
Comment 42 Nick Cameron [:nrc] 2014-01-16 18:21:00 PST
Created attachment 8361446 [details] [diff] [review]
vol 2 part 8: Tests for border-image-source/-moz-element
Comment 43 Nick Cameron [:nrc] 2014-01-16 18:21:31 PST
Try push: https://tbpl.mozilla.org/?tree=Try&rev=be1bd55d9be2
Comment 44 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-01-16 18:28:27 PST
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) (Exited; email my personal email if necessary) 2014-01-16 18:33:47 PST
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] 2014-01-16 19:04:08 PST
(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) (Exited; email my personal email if necessary) 2014-01-16 19:20:56 PST
No problem, just ignore that comment.
Comment 48 Nick Cameron [:nrc] 2014-01-20 18:45:53 PST
(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] 2014-01-22 17:39:58 PST
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
Comment 50 Nick Cameron [:nrc] 2014-01-22 17:40:53 PST
Created attachment 8364080 [details] [diff] [review]
vol 3 part 1: Factor out GetDefaultSrcRect()

vol 3 is list-style-image
Comment 51 Nick Cameron [:nrc] 2014-01-22 17:41:50 PST
Created attachment 8364081 [details] [diff] [review]
vol 3 part 2: list-style-image
Comment 52 Nick Cameron [:nrc] 2014-01-22 17:42:28 PST
Created attachment 8364082 [details] [diff] [review]
vol 3 part 3: list-style-image tests
Comment 53 Nick Cameron [:nrc] 2014-01-22 17:42:55 PST
try push for border-image: https://tbpl.mozilla.org/?tree=Try&rev=06154841651b
Comment 54 Nick Cameron [:nrc] 2014-01-26 12:57:53 PST
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] 2014-01-26 18:41:38 PST
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!
Comment 56 Timothy Nikkel (:tnikkel) 2014-01-26 22:30:08 PST
(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) (Exited; email my personal email if necessary) 2014-01-28 13:51:45 PST
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] 2014-01-28 15:15:33 PST
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 61 Sebastian Zartner [:sebo] 2014-07-18 01:17:45 PDT
What happened to the patches for 'list-style-image'?

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

Sebastian

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