Bug 18349 – Add support for CSS-based gradients

• Component: CSS
• Status: RESOLVED FIXED

[Dave Hyatt]

This bug is tracking an experimental feature to do gradients in CSS.

[Dave Hyatt]

Created attachment 20390 [details]
Work in progress

[Dave Hyatt]

Random notes to self.  The overall goal is to implement gradients in such a way that other kinds of generators should be easily possible as well (e.g., checkerboards, starbursts, halos, whatever).

(1) CSSGradientValue should subclass from a CSSImageGeneratorValue.  This new class should know how to manage a refcounted cache of GeneratedImages.  These should be hashed on width*height.
(2) CSSImageGeneratorValue will have a virtual method, e.g., "image(w, h, client)", that can be invoked to generate, cache, and reference an image.
(3) The back end color stops should go ahead and resolve colors to Color objects.  There's no need to keep them as primitive values.
(4) GeneratedImage and CachedImage need to share a common base class, e.g., ImageWrapper.
(5) The whole engine needs to switch from CachedImage* to ImageWrapper*.
(6) In places where the CachedImage had an "imageContainerSize" set (for SVG images with no intrinsic size), this is a suitable place to handle generated image creation.

[Dave Hyatt]

Created attachment 20470 [details]
Ready for initial comments.

[Eric Seidel (no email)]

Comment on attachment 20470 [details]
Ready for initial comments.

MIght as well fix:
String CSSGradientValue::cssText() const
before landing.

Gradient* CSSGradientValue::createGradient(RenderObject* renderer, const IntSize& size)
Needs to use auto_ptr.  We don't have a PassOwnPtr, sadly.
Returning raw ptrs is just asking for trouble.

void CSSGradientValue::sortStops()
could be
void CSSGradientValue::sortStopsIfNeeded()
Either way works.

I also prefer an early-return style (personal bias):
if (m_stopsSorted)
   return;

 54         if (m_sizes.count(size) == 0) {
Should probably be:
if (!m_sizes.contains(size))
    delete m_images.take(size);

count(size) == 0 was initially confusing to me.


if (!args || args->size() == 0
Seems like ValueList should have an isEmpty() function


My understanding is that PassRefPtrs are generally discouraged on the stack:
PassRefPtr<CSSPrimitiveValue> point = parseGradientPoint(a, true)

the correct way to use that as a PassRefPtr would be to do:
RefPtr<CSSPrimitiveValue> point = parseGradientPoint(a, true)

And then later use point.release() wherever you need it as a PassRefPtr.  That prevents any ref-churn, and prevents any gotchas of using "point" twice by accident (the second time it will be NULL) :)

The same is of course true if you use RefPtr<> point, but point.release() requires no comment about nulling out. :)


StyleImage* CSSStyleSelector::createStyleImage(CSSValue* value)
Should use a PassRefPtr


Otherwise looks good.

r- for the memory management issues.  (And lack of test cases).

[Dave Hyatt]

Created attachment 20472 [details]
Test case for radial gradients

[Dave Hyatt]

Created attachment 20473 [details]
Screenshot of the radial gradient example

[Dave Hyatt]

Created attachment 20474 [details]
Patch with test cases and changelog

[Dave Hyatt]

Created attachment 20486 [details]
Patch that addresses lots of comments

[Dave Hyatt]

Created attachment 20488 [details]
More improvements.  Make sure not to break SVG zooming (not that it works anyway).

[Dave Hyatt]

I patched all the project files (Windows, wx, Qt and GTK).

[Oliver Hunt]

Comment on attachment 20488 [details]
More improvements.  Make sure not to break SVG zooming (not that it works anyway).

Conditional r=me
* needs stubs for other platforms
* I'd like a bug report to cover gradients in border-image
* The test is insufficient -- need tests for both radial and linear gradients, also needs to cover gradients that extend beyond the bounds of an element, also infinite and nan control points for the gradient.  I'd also like the behaviour out side the bounds of the gradient to be defined. eg. a gradient from 10%,10%->90%,90% what is the colour at 5%,5%, or 95%,95%