[webkit-reviews] review denied: [Bug 18349] Add support for CSS-based gradients : [Attachment 20470] Ready for initial comments.

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Dave Hyatt <hyatt at apple.com>'s request
for review:
Bug 18349: Add support for CSS-based gradients
http://bugs.webkit.org/show_bug.cgi?id=18349

Attachment 20470: Ready for initial comments.
http://bugs.webkit.org/attachment.cgi?id=20470&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
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).