[webkit-reviews] review granted: [Bug 49452] Placeholder should not be swapped in and out of the text control's inner text element : [Attachment 73756] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Adele Peterson <adele at apple.com>'s
request for review:
Bug 49452: Placeholder should not be swapped in and out of the text control's
inner text element
https://bugs.webkit.org/show_bug.cgi?id=49452

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

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

> WebCore/rendering/RenderTextControl.cpp:572
> +	       return;

Is it OK that this does not fall through to RenderBlock::paintObject? Maybe we
could put all this code in another function so return can be used without
skipping the call to the base class.

> WebCore/rendering/RenderTextControl.cpp:576
> +	       return;

Same question.

> WebCore/rendering/RenderTextControl.cpp:590
> +	   unsigned length = placeholderText.length();
> +	   const UChar* string = placeholderText.characters();

I’m not sure you need these local variables. They are only used once.

> WebCore/rendering/RenderTextControl.cpp:596
> +	       int textX;
> +	       int textY = ty + borderTop() + paddingTop() +
textRenderer->paddingTop() + placeholderStyle->font().ascent();

Since we will need an IntPoint to use this, how about using an IntPoint from
the start and use setY and setX?

> WebCore/rendering/RenderTextControlSingleLine.cpp:680
> +	   if (node()->isHTMLElement()) {

Given that we cast to HTMLInputElement it seems strange that this existing code
you did not modify checks isHTMLElement rather than hasTagName(inputTag).