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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> 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 92972: [PATCH] Comments addressed
https://bugs.webkit.org/attachment.cgi?id=92972&action=review

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

review- because this seems to affect normal property value fetching, not just
the inspector functions, and that should be tested and because this needs to
use StringBuilder, not String.

> Source/WebCore/ChangeLog:5
> +	   font property does not show up as "shorthand" in inspector

I know you discovered this while working on the inspector but the code change
here does not seem at all inspector-specific. Can we add some non-inspector CSS
test cases to demonstrate this bug and fix?

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:298
> +bool
CSSMutableStyleDeclaration::appendMandatoryPropertyValueIfExplicit(const
CSSProperty* property, const String& stringBefore, String* result) const

For WebKit coding style, result should be a reference, not a pointer.

For a code path that is appending, result needs to be a StringBuilder, not a
String.

Since stringBefore is always a string constant, it should be a const char*,
since StringBuilder already has an optimized code path for appending those.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:312
> +    DEFINE_STATIC_LOCAL(String, spaceString, (" "));

Should get rid of this and just use a space after making the change I suggested
above.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:313
> +    String result("");

Needs to be a StringBuilder.

> Source/WebCore/css/CSSParser.h:76
>	   void addProperty(int propId, PassRefPtr<CSSValue>, bool important);
> +	   void addPropertyImplicit(int propId, PassRefPtr<CSSValue>, bool
important, bool implicit);

I think this would be clearer as a default argument rather than a separate
function. All the callers are already passing a boolean argument, not a
constant, and it could simply default to true.