[webkit-reviews] review granted: [Bug 118162] CSS: Implement the :placeholder-shown pseudo-class from Selectors Level 4 : [Attachment 236889] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 118162: CSS: Implement the :placeholder-shown pseudo-class from Selectors
Level 4
https://bugs.webkit.org/show_bug.cgi?id=118162

Attachment 236889: Patch
https://bugs.webkit.org/attachment.cgi?id=236889&action=review

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
View in context: https://bugs.webkit.org/attachment.cgi?id=236889&action=review


> Source/WebCore/html/HTMLTextFormControlElement.cpp:169
> +    setNeedsStyleRecalc();

Why explicit setNeedsStyleRecalc? Shouldn't style change trigger recalc?

> Source/WebCore/html/HTMLTextFormControlElement.cpp:175
> +	   if (m_isPlaceholderVisible)
> +	       placeholder->setInlineStyleProperty(CSSPropertyDisplay,
CSSValueBlock, true);
> +	   else
> +	       placeholder->setInlineStyleProperty(CSSPropertyDisplay,
CSSValueNone, true);

I would use

placeholder->setInlineStyleProperty(CSSPropertyDisplay, m_isPlaceholderVisible
? CSSValueBlock : CSSValueNone, true);

> Source/WebCore/html/HTMLTextFormControlElement.cpp:643
> +	   placeholder->setInlineStyleProperty(CSSPropertyVisibility,
CSSValueVisible : CSSValueHidden);

This doesn't look like it would compile.

> Source/WebCore/html/HTMLTextFormControlElement.cpp:650
> +	       placeholder->setInlineStyleProperty(CSSPropertyVisibility,
CSSValueVisible : CSSValueHidden);

Nor does this.