[webkit-reviews] review requested: [Bug 48539] Support the text-emphasis CSS property : [Attachment 75408] Part 1: CSS support

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has asked  for review:
Bug 48539: Support the text-emphasis CSS property
https://bugs.webkit.org/show_bug.cgi?id=48539

Attachment 75408: Part 1: CSS support
https://bugs.webkit.org/attachment.cgi?id=75408&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=75408&action=review

> WebCore/css/CSSComputedStyleDeclaration.cpp:1237
> +	       }
> +	       }

I think it would be slightly better to explicitly list all the cases so we get
a warning if we ever add a new case. If you want to put the default code
outside the switch and use "break" we’d get everything except for the runtime
check for a bad enum value, which I think is a good tradeoff.

> WebCore/css/CSSParser.cpp:5147
> +    if (fill && shape) {
> +	   RefPtr<CSSValueList> parsedValues =
CSSValueList::createSpaceSeparated();
> +	   parsedValues->append(fill.release());
> +	   parsedValues->append(shape.release());
> +	   addProperty(CSSPropertyWebkitTextEmphasisStyle,
parsedValues.release(), important);
> +    } else if (fill)
> +	   addProperty(CSSPropertyWebkitTextEmphasisStyle, fill.release(),
important);
> +    else if (shape)
> +	   addProperty(CSSPropertyWebkitTextEmphasisStyle, shape.release(),
important);
> +    else
> +	   return false;
> +
> +    return true;

I think this would read better if the return true was inside each if and we
thus did not need all those elses.