[webkit-reviews] review granted: [Bug 21227] Implement placeholder-color CSS property : [Attachment 23955] patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Sep 30 15:32:28 PDT 2008


Darin Adler <darin at apple.com> has granted Adele Peterson <adele at apple.com>'s
request for review:
Bug 21227: Implement placeholder-color CSS property
https://bugs.webkit.org/show_bug.cgi?id=21227

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

------- Additional Comments from Darin Adler <darin at apple.com>
+    String placeholder;
+    if (value().isEmpty() && document()->focusedNode() != this)
+	 placeholder = getAttribute(placeholderAttr);

It'd be more efficient to have a boolean here instead of a String. It's not
necessary to copy the placeholder attribute into a local String just to later
check if it's 0.

+    if (!placeholder.isEmpty() || m_placeholderShouldBeVisible)
+	 m_placeholderShouldBeVisible = !placeholder.isEmpty();

The if statement here doesn't add value. The assignment would work the same
without the if. I think I see how you cut this down from the original
RenderTextControl function.

I'd just write it like this:

    m_placeholderShouldBeVisible = value().isEmpty()
	&& document()->focusedNode() != this
	&& !getAttribute(placeholderAttr).isEmpty();

That doesn't require any if statements or local variables.

+    if (oldPlaceholderShouldBeVisible != m_placeholderShouldBeVisible)
+	 setChanged();
+
+}

There's an extra blank line here at the end of this function.

It seems a little weak that both the DOM and the render tree cache a boolean to
remember whether the placeholder is visible, but I guess it's smart for the
render object to cache it so it can properly handle changes in state, and the
DOM needs to cache it to make style resolution fast.

+// Value chosen by observation.  This can be tweaked.
+static const int minColorDifferenceSquared = 1300;
+static Color disabledTextColor(const Color& textColor, const Color&
backgroundColor)

Normally the constant would go in a separate paragraph at the start of the
file. Maybe all that's missing is a blank line. I think the constant name
should include the word "contrast".

+    // If there's not very much contrast between the disabled color and the
background color,
+    // just leave the text color alone.  We don't want to change a good
contrast color scheme so that it has really bad contrast.
+    // If the the contrast was already poor, then it doesn't do any good to
change it to a different poor contrast color scheme.
+    if (differenceSquared(disabledColor, backgroundColor) >=
minColorDifferenceSquared)
+	 return disabledColor;
+    return textColor;

The code is the opposite of the comment. It says "if there's enough contrast,
return the computed color". You should reverse the code to match the comment,
which says "if there's not enough contrast, leave the color alone".

+    if (!m_multiLine &&
static_cast<HTMLInputElement*>(element)->placeholderShouldBeVisible())
+	 textBlockStyle->setTextSecurity(TSNONE);

This needs a comment. Also, I think it's fixing a separate bug. Is there a
regression test for this bug?

+	 ExceptionCode ec = 0;

No need to set this to 0 here if you're not going to look at the result. This
was the same in the old code.

r=me as-is, or with some or all of my suggestions


More information about the webkit-reviews mailing list