[webkit-reviews] review denied: [Bug 27756] [HTML5][Forms] Support for HTMLInputElement::list and HTMLInputElement::selectedOption : [Attachment 33694] Support for HTMLInputElement::list and HTMLInputElement::selectedOption.

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied TAMURA, Kent <tkent at chromium.org>'s
request for review:
Bug 27756: [HTML5][Forms] Support for HTMLInputElement::list and
HTMLInputElement::selectedOption
https://bugs.webkit.org/show_bug.cgi?id=27756

Attachment 33694: Support for HTMLInputElement::list and
HTMLInputElement::selectedOption.
https://bugs.webkit.org/attachment.cgi?id=33694&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
You can just check here for dataListTag and cast directly to an
HTMLDataListElement:
742	    if (element && element->isHTMLElement() &&
element->hasTagName(datalistTag))
 1743		  return static_cast<HTMLElement*>(element)

Not required but slightly cleaner.

isEmpty() checks null:
 697	     m_hasNonEmptyList = !attr->isNull() && !attr->isEmpty();

This sseems like the long way around:
      bool hadNonEmptyList = m_hasNonEmptyList;
 697	     m_hasNonEmptyList = !attr->isNull() && !attr->isEmpty();
 698	     if (hadNonEmptyList != m_hasNonEmptyList && attached()) {
 699		 // Re-create a renderer because m_hasNonEmptyList affects UI.
 700		 detach();
 701		 attach();
 702	     }

Do you really need to attach/detach every time?  Can't you do a layout or a
style recalc?  What does the list do to the renderer?

list() clearly should return a HTMLDataListElement*, or?  Otherwise you
certainly shouldn't be blindly casting to one in callers of list()