Bug 19660 - Implement CSS Variables
: Implement CSS Variables
Product: WebKit
Classification: Unclassified
Component: CSS
: 528+ (Nightly build)
: Macintosh Mac OS X 10.5
: P2 Normal
Assigned To: Dave Hyatt
Depends on:
  Show dependency treegraph
Reported: 2008-06-18 15:01 PDT by Dave Hyatt
Modified: 2008-06-20 19:28 PDT (History)
2 users (show)

See Also:

Initial cut at CSS variables. (127.32 KB, patch)
2008-06-18 15:04 PDT, Dave Hyatt
no flags 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 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
Updated to deal with conflicts. (129.94 KB, patch)
2008-06-19 09:50 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
Ready for review. Now with many tests and changelogs. (149.56 KB, patch)
2008-06-19 13:05 PDT, Dave Hyatt
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Hyatt 2008-06-18 15:01:10 PDT
This bug covers the initial landing of CSS variables support in WebKit.
Comment 1 Dave Hyatt 2008-06-18 15:04:15 PDT
Created attachment 21824 [details]
Initial cut at CSS variables.
Comment 2 Dave Hyatt 2008-06-19 01:26:18 PDT
Created attachment 21830 [details]
Updated to deal with Darin's big cleanup patch to the style system
Comment 3 Dave Hyatt 2008-06-19 09:50:02 PDT
Created attachment 21839 [details]
Updated to deal with conflicts.
Comment 4 Dave Hyatt 2008-06-19 13:05:32 PDT
Created attachment 21847 [details]
Ready for review.  Now with many tests and changelogs.
Comment 5 Darin Adler 2008-06-19 15:05:11 PDT
Comment on attachment 21847 [details]
Ready for review.  Now with many tests and changelogs.

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 Dave Hyatt 2008-06-19 15:08:06 PDT
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 Dave Hyatt 2008-06-19 15:36:28 PDT