[webkit-reviews] review denied: [Bug 15598] font property does not show up as "shorthand" in inspector : [Attachment 111929] [PATCH] Comments addressed, take 3

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 15598: font property does not show up as "shorthand" in inspector
https://bugs.webkit.org/show_bug.cgi?id=15598

Attachment 111929: [PATCH] Comments addressed, take 3
https://bugs.webkit.org/attachment.cgi?id=111929&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=111929&action=review


This looks good to me, especially as all bots are green now. I'd still like to
see a test which doesn't use Inspector at all, that examines the font
shorthand. Some comments remaining, that still lead to r-:

> Source/WebCore/ChangeLog:7
> +

This needs more explaination. The inline comments are very helpful, but still
the whole context is missing.
An important fix deserves a longer description!

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:315
> +	   return "";

return emptyString();

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:319
> +    success &=
appendMandatoryPropertyValueIfExplicit(findPropertyWithId(CSSPropertyFontStyle)
, "", result);

Hm, I'd rather pass CSSPropertyFontStyle to
appendMandatoryPropertyValueIfExplicit, instead of the CSSProperty*.

There's no need then to pass in stringBefore, as the value can be determined by
the property.
static inline void appendStringBefore(CSSPropertyID id, StringBuilder& result)
(a better name should be found!)
{
    switch (id) {
    case CSSPropertyFontSize:
	break;
     case CSSPropertyFontVariant:
	result.append(' ');
	return;
...
};

This will also use StringBuilder::append(char) rather than const char* and
avoids an unncessary strlen.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:320
> +    success &=
appendMandatoryPropertyValueIfExplicit(findPropertyWithId(CSSPropertyFontVarian
t), " ", result);

This would just read
success &= appendMandatoryPropertyValueIfExplicit(CSSPropertyFontVariant,
result)
then.

> Source/WebCore/css/FontValue.cpp:40
> +	       result.append(" ");

Use append(' ')

> Source/WebCore/css/FontValue.cpp:45
> +	       result.append(" ");

Ditto.

> Source/WebCore/css/FontValue.cpp:50
> +	       result.append(" ");

Ditto.

> Source/WebCore/css/FontValue.cpp:55
> +	       result.append(" ");

Ditto.

> Source/WebCore/css/FontValue.cpp:56
> +	   result.append("/");

'/'

> Source/WebCore/css/FontValue.cpp:61
> +	       result.append(" ");

' '

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1888
> +    styleSelector->updateFont();

Why is this needed now? This needs to be documented.