[webkit-reviews] review denied: [Bug 15598] font property does not show up as "shorthand" in inspector : [Attachment 111791] [PATCH] Darin's 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 111791: [PATCH] Darin's comments addressed
https://bugs.webkit.org/attachment.cgi?id=111791&action=review

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


review- because of the issue Julien mentions

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

A more efficient empty string is emptyAtom. Is empty string really right for
this? I don’t see any test cases covering this.

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

Ditto.

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

Ditto.

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

Ditto.

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

Ditto.

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

Ditto.

> Source/WebCore/css/CSSParser.cpp:3959
> -		   // do nothing, it's the inital value for all three
> +		   // Do nothing, it's the initial value for all three.
> +		   switch (valueOrdinal) {
> +		   case 0:
> +		       styleImplicit = false;
> +		       break;
> +		   case 1:
> +		       variantImplicit = false;
> +		       break;
> +		   case 2:
> +		       weightImplicit = false;
> +		       break;
> +		   }

The comment says “do nothing”, but now the code does something, so “do nothing”
no longer makes sense.

> Source/WebCore/css/CSSParser.cpp:4039
> +	       // Default value, nothing to do

You didn’t add a period. I’m not sure “nothing to do” really describes code
that does something. May need better wording.