Bug 18349

Summary: Add support for CSS-based gradients
Product: WebKit Reporter: Dave Hyatt <hyatt>
Component: CSSAssignee: Dave Hyatt <hyatt>
Status: RESOLVED FIXED    
Severity: Normal CC: jakub.rusinek
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Attachments:
Description Flags
Work in progress
none
Ready for initial comments.
eric: review-
Test case for radial gradients
none
Screenshot of the radial gradient example
none
Patch with test cases and changelog
none
Patch that addresses lots of comments
none
More improvements. Make sure not to break SVG zooming (not that it works anyway). oliver: review+

Description Dave Hyatt 2008-04-07 18:25:58 PDT
This bug is tracking an experimental feature to do gradients in CSS.
Comment 1 Dave Hyatt 2008-04-07 18:26:21 PDT
Created attachment 20390 [details]
Work in progress
Comment 2 Dave Hyatt 2008-04-07 18:34:58 PDT
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.

Comment 3 Dave Hyatt 2008-04-10 23:27:14 PDT
Created attachment 20470 [details]
Ready for initial comments.
Comment 4 Eric Seidel (no email) 2008-04-10 23:50:57 PDT
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).
Comment 5 Dave Hyatt 2008-04-11 00:12:40 PDT
Created attachment 20472 [details]
Test case for radial gradients
Comment 6 Dave Hyatt 2008-04-11 00:14:41 PDT
Created attachment 20473 [details]
Screenshot of the radial gradient example
Comment 7 Dave Hyatt 2008-04-11 01:32:54 PDT
Created attachment 20474 [details]
Patch with test cases and changelog
Comment 8 Dave Hyatt 2008-04-11 14:00:43 PDT
Created attachment 20486 [details]
Patch that addresses lots of comments
Comment 9 Dave Hyatt 2008-04-11 15:17:12 PDT
Created attachment 20488 [details]
More improvements.  Make sure not to break SVG zooming (not that it works anyway).
Comment 10 Dave Hyatt 2008-04-11 15:32:23 PDT
I patched all the project files (Windows, wx, Qt and GTK).

Comment 11 Oliver Hunt 2008-04-11 16:16:29 PDT
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%