Bug 19660 - Implement CSS Variables
: Implement CSS Variables
: WebKit
: 528+ (Nightly build)
: Macintosh Mac OS X 10.5
: P2 Normal
Assigned To:
: http://disruptive-innovations.com/zoo...
  Show dependency treegraph
Reported: 2008-06-18 15:01 PST by
Modified: 2008-06-20 19:28 PST (History)

Initial cut at CSS variables. (127.32 KB, patch)
2008-06-18 15:04 PST, Dave Hyatt
no flags Review Patch | Details | Formatted Diff | Diff
Updated to deal with Darin's big cleanup patch to the style system (50.94 KB, patch)
2008-06-19 01:26 PST, Dave Hyatt
no flags Review Patch | Details | Formatted Diff | Diff
Updated to deal with conflicts. (129.94 KB, patch)
2008-06-19 09:50 PST, Dave Hyatt
no flags Review Patch | Details | Formatted Diff | Diff
Ready for review. Now with many tests and changelogs. (149.56 KB, patch)
2008-06-19 13:05 PST, Dave Hyatt
darin: review+
Review Patch | Details | Formatted Diff | Diff


You need to log in before you can comment on or make changes to this bug.

Description From 2008-06-18 15:01:10 PST
This bug covers the initial landing of CSS variables support in WebKit.
------- Comment #1 From 2008-06-18 15:04:15 PST -------
Created an attachment (id=21824) [details]
Initial cut at CSS variables.
------- Comment #2 From 2008-06-19 01:26:18 PST -------
Created an attachment (id=21830) [details]
Updated to deal with Darin's big cleanup patch to the style system
------- Comment #3 From 2008-06-19 09:50:02 PST -------
Created an attachment (id=21839) [details]
Updated to deal with conflicts.
------- Comment #4 From 2008-06-19 13:05:32 PST -------
Created an attachment (id=21847) [details]
Ready for review.  Now with many tests and changelogs.
------- Comment #5 From 2008-06-19 15:05:11 PST -------
(From update of attachment 21847 [details])
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
------- Comment #6 From 2008-06-19 15:08:06 PST -------
Thanks for the comments.  This was actually already reviewed by Beth (who forgot to + the bug I guess).  I will make another checkin to clean things up based off your suggestions.
------- Comment #7 From 2008-06-19 15:36:28 PST -------