[webkit-reviews] review denied: [Bug 18349] Add support for
CSS-based gradients : [Attachment 20470] Ready for initial comments.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Apr 10 23:50:57 PDT 2008
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).
More information about the webkit-reviews
mailing list