[webkit-reviews] review granted: [Bug 17108] CSS parser should NOT return raw CSSValue* from creation functions : [Attachment 18813] Beat CSSParser with the RefPtr stick

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Eric Seidel <eric at webkit.org>'s
request for review:
Bug 17108: CSS parser should NOT return raw CSSValue* from creation functions
http://bugs.webkit.org/show_bug.cgi?id=17108

Attachment 18813: Beat CSSParser with the RefPtr stick
http://bugs.webkit.org/attachment.cgi?id=18813&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
 1097			  values->append(parsedValue.get()); // FIXME: why
woule one ever append the same value twice?

Typo "woule".

I also think that this comment should be more clear on the point. Something
along the lines of, "If we know this could never be done with the same value
twice, we could change this to release() and get rid of a little bit of
reference count churn."

And I think the comment should go on its own line. The way to do that is to
change the code to early-break if parsedValue is 0 instead of having an else.

Same below for values->length(). It should be an early exit that calls return
false.

 1138	      PassRefPtr<Pair> pair = new Pair(parsedValue1.release(),
parsedValue2.release());
 1139	      RefPtr<CSSPrimitiveValue> val = new CSSPrimitiveValue(pair);

How about just doing this all on one line without the local variable? Or doing
it with a RefPtr and a call to release(). It seems a little ugly to have a
local variable of type PassRefPtr.

 1585	  Vector<RefPtr<CSSValue>, numProperties> values(numProperties);

Why not

     RefPtr<CSSValue> values[numProperties];

here? I don't think a Vector is better than an array here.

I think CSSParser::parseSVGValue would look better with early exits too.

I'll say r=me, although I'm not so happy with the Vector.