[webkit-reviews] review denied: [Bug 46875] Web Inspector: content: counter displays decimal number instead of name : [Attachment 70694] [PATCH] Take 3

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Joseph Pecoraro <joepeck at webkit.org>'s
request for review:
Bug 46875: Web Inspector: content: counter displays decimal number instead of
name
https://bugs.webkit.org/show_bug.cgi?id=46875

Attachment 70694: [PATCH] Take 3
https://bugs.webkit.org/attachment.cgi?id=70694&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=70694&action=review

Can't commit this with so many failing EWS bots.

> LayoutTests/fast/css/counters/counters-cssText.html:28
> +	   <ul class="second"><li>one.one</li></ul>

do you mean for this to say one.one?

> WebCore/css/CSSPrimitiveValue.cpp:733
> +	       if (!counterValue->separator()) {
> +		   text = "counter(";
> +		   text += counterValue->identifier();
> +		   text += ", ";
> +		   text += counterValue->listStyle();
> +		   text += ")";
> +	       } else {
> +		   text = "counters(";
> +		   text += counterValue->identifier();
> +		   text += ", ";
> +		   text += quoteCSSStringIfNeeded(counterValue->separator());
> +		   text += ", ";
> +		   text += counterValue->listStyle();
> +		   text += ")";
> +	       }
>	       break;

It seems using multiple ifs you coudl share more code here.

> WebCore/css/Counter.h:38
> +    String listStyle() const { return m_listStyle ?
CSSPrimitiveValue::create((EListStyleType)
m_listStyle->getIntValue())->getStringValue() : String(); }

Normally we avoid c-style casts and use static_cast instead.