[webkit-reviews] review granted: [Bug 19660] Implement CSS Variables : [Attachment 21847] Ready for review. Now with many tests and changelogs.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Dave Hyatt <hyatt at apple.com>'s
request for review:
Bug 19660: Implement CSS Variables
https://bugs.webkit.org/show_bug.cgi?id=19660

Attachment 21847: Ready for review.  Now with many tests and changelogs.
https://bugs.webkit.org/attachment.cgi?id=21847&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
You fixed the "sepcification" typo but added a new typo:

// he CSS3 specification defines the format of a HSL color as

It should be "The", not "he".

+    bool ok = false;
+    if (m_variableNames.size()) {
+	 ok = true;
+	 declaration->addParsedVariable(variableName, m_variableValues[0]);
+	 clearVariables();
+    } else
+	 clearVariables();

Why not put the clearVariables function call outside the if statement?

+	     break;
+	 } else if (valueList->valueAt(i)->unit == CSSParserValue::Function) {

You don't need an else here after the break.

+    addProperty(propId, val.get(), important);

This doesn't need to be "val.get()" -- it should be "val.release()" since this
is the last use of val in this function. Or just plain "val" would work too,
but be less efficient. No need for get() in any case.

+	 RefPtr<CSSPrimitiveValue> primitiveValue =
CSSPrimitiveValue::createIdentifier(iValue);
+	
primitiveValue->setPrimitiveType(CSSPrimitiveValue::CSS_PARSER_OPERATOR);
+	 parsedValue = primitiveValue;

This should say "primitiveValue.release()" to avoid a tiny bit of refcount
churn.

+    CSSParserValueList* args;
+
+    ~CSSParserFunction() { delete args; }

Maybe you could use OwnPtr instead here?

+	     char c = static_cast<char>(m_value.ident);

What range of characters does this support? This expression won't work well for
characters outside the 0-7F range.

+    // use with care!!!
+    void setPrimitiveType(unsigned short type) { m_type = type; }

What a crappy comment (I know you were just bringing this back). What does
"with care" even mean? I use all functions with care. The argument should be
the enum type, not just unsigned short, right?

But really we should get rid of this; add a new create function to use instead
of createIdentifier for Operator and use a different union member and not bring
this function back.

+    RefPtr<CSSMutableStyleDeclaration> resolvedDecl =
m_resolvedVariablesDeclarations.get(decl);

You could just use a raw pointer here and add the get() call on this line.

+    virtual CSSParserValue parserValue() const { ASSERT_NOT_REACHED(); return
CSSParserValue(); }

Can this function be pure virtual instead?

+    static PassRefPtr<CSSValueList>
createFromParserValueList(CSSParserValueList* list)

Why not just name this "create"?

+    if (m_list)
+	 return m_list->cssText();
+    return "";

+    CSSValue* val = getParsedVariable(variableName);
+    if (val)
+	 return val->cssText();

I prefer the early return style in cases like these. Return in the error case
then continue in the normal case. It's great when you have non-trivial code.

+    CSSVariableDependentValue(const PassRefPtr<CSSValueList>&);

This needs to just be a PassRefPtr, not a const PassRefPtr&.

+    ~CSSVariableDependentValue();

Please say virtual explicitly here.

+    CSSParser parser(useStrictParsing());
+    if (!parser.parseVariable(this, variableName, variableValue)) // If the
parse succeeds, it will call addParsedVariable (our internal method for doing
the add) with the parsed Value*.

+    RefPtr<MediaList> m_lstMedia;

Please just call this m_media. I've renamed it in other places in the existing
classes.

+    void setDeclaration(CSSVariablesDeclaration* decl) { m_variables = decl; }


Please use PassRefPtr here.

+    CSSVariablesRule(CSSStyleSheet* parent, MediaList*);

Please use PassRefPtr for the MediaList.

+CSSVariablesRule::CSSVariablesRule(CSSStyleSheet* parent, MediaList*
mediaList)
+    : CSSRule(parent)
+{
+    m_lstMedia = mediaList;
+}

Please use constructor syntax to initialize the mediaList -- it avoids an
unneeded store and branch that happens later.

I don't see any tests for the cssText functions. I'd like to make sure we have
some.

There's nothing fundamentally flawed here, so r=me