[Webkit-unassigned] [Bug 77299] Reduce non-CSSOM API of CSSStyleDeclaration

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=77299





--- Comment #15 from Nikolas Zimmermann <zimmermann at kde.org>  2012-01-30 04:09:02 PST ---
(From update of attachment 124518)
View in context: https://bugs.webkit.org/attachment.cgi?id=124518&action=review

First round of comments, not changing r? state yet.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2569
> +    for (unsigned i = 0; i < length; i++) {

nitpick: ++i

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2571
> +        RefPtr<CSSValue> value = getPropertyCSSValue(set[i]);
> +        if (value)

This can be turned into one line.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2582
> +    if (!propertyID)
> +        return 0;
> +    return getPropertyCSSValue(propertyID);

If the common case is property is found (which I assume), I'd swap these.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2589
> +    if (!propertyID)
> +        return String();

Ditto.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2596
> +    return "";

return emptyString(), it's shared and more efficient.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2601
> +    return "";

Ditto.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2617
> +    return String();

Ditto.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:1052
> +    if (!propertyID)
> +        return 0;

Same swapping possible.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:1060
> +    if (!propertyID)
> +        return String();

Ditto + use emptyString().

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:1069
> +    if (!propertyID)
> +        return String();
> +    return getPropertyPriority(propertyID) ? "important" : "";

Ditto + use emptyString().

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:1080
> +    if (!propertyID)
> +        return String();
> +    int shorthandID = getPropertyShorthand(propertyID);
> +    if (!shorthandID)
> +        return String();
> +    return getPropertyName(static_cast<CSSPropertyID>(shorthandID));

Ditto + use emptyString().

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:1086
> +    if (!propertyID)

etc, .. not listing the others.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:1147
> +    {

Is the extra scope intentional? I don't see it's needed.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:1158
> +    // FIXME: This should use mass removal.
> +    for (unsigned i = 0; i < propertiesToRemove.size(); i++)
> +        removeProperty(propertiesToRemove[i]);

How about adding removeProperty(Vector<int>&) and moving the FIXME into that method?

> Source/WebCore/editing/Editor.cpp:2754
> +    style->setProperty(CSSPropertyWordWrap, "break-word", false);

While you're at it, how about using getValueName(CSSValueBreakWord) instead of hard-coding it here?

> Source/WebCore/editing/Editor.cpp:2755
> +    style->setProperty(CSSPropertyWebkitNbspMode, "space", false);

Ditto, for all other props.

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