[Webkit-unassigned] [Bug 17108] CSS parser should NOT return raw CSSValue* from creation functions

[bugzilla-daemon at webkit.org]

http://bugs.webkit.org/show_bug.cgi?id=17108


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #18813|review?                     |review+
               Flag|                            |




------- Comment #4 from darin at apple.com  2008-01-31 10:19 PDT -------
(From update of attachment 18813)
 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.


-- 
Configure bugmail: http://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.