Bug 18349 - Add support for CSS-based gradients
Summary: Add support for CSS-based gradients
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Dave Hyatt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-04-07 18:25 PDT by Dave Hyatt
Modified: 2008-04-12 11:54 PDT (History)
1 user (show)

See Also:


Attachments
Work in progress (25.88 KB, patch)
2008-04-07 18:26 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
Ready for initial comments. (70.09 KB, patch)
2008-04-10 23:27 PDT, Dave Hyatt
eric: review-
Details | Formatted Diff | Diff
Test case for radial gradients (634 bytes, text/html)
2008-04-11 00:12 PDT, Dave Hyatt
no flags Details
Screenshot of the radial gradient example (20.69 KB, image/png)
2008-04-11 00:14 PDT, Dave Hyatt
no flags Details
Patch with test cases and changelog (80.45 KB, patch)
2008-04-11 01:32 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch that addresses lots of comments (100.34 KB, patch)
2008-04-11 14:00 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
More improvements. Make sure not to break SVG zooming (not that it works anyway). (102.48 KB, patch)
2008-04-11 15:17 PDT, Dave Hyatt
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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%