[webkit-reviews] review denied: [Bug 89544] Redraw slider tick marks when datalist changes. : [Attachment 152747] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Keishi Hattori
<keishi at webkit.org>'s request for review:
Bug 89544: Redraw slider tick marks when datalist changes.
https://bugs.webkit.org/show_bug.cgi?id=89544

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

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=152747&action=review


> Source/WebCore/html/HTMLInputElement.cpp:693
> -    }
>  #if ENABLE(DATALIST)
> -    else if (attribute.name() == listAttr)
> +    } else if (attribute.name() == listAttr) {

I recommend not to change '}' position.

> Source/WebCore/html/HTMLInputElement.cpp:701
>  #endif
>  #if ENABLE(INPUT_SPEECH)
> -    else if (attribute.name() == webkitspeechAttr) {
> +    } else if (attribute.name() == webkitspeechAttr) {

I recommend move '}' before #endif.

> Source/WebCore/html/HTMLInputElement.cpp:1517
> +    m_listAttributeTargetObserver =
ListAttributeTargetObserver::create(fastGetAttribute(listAttr), this);

Do we need to create LIstAttributeTargetObserver when the input element is
!inDocument()?

> Source/WebCore/html/HTMLInputElement.cpp:1523
> +    if (isRangeControl())
> +	   setNeedsStyleRecalc();

Should introduce new virtual member function in InputType, or call
setNeedsStyleRecalc() unconditionally.

> Source/WebCore/html/HTMLOptionElement.cpp:282
> +    ContainerNode* dataList = parentNode();
> +    while (dataList && !dataList->hasTagName(datalistTag))
> +	   dataList = dataList->parentNode();
> +
> +    if (!dataList)
> +	   return 0;
> +
> +    return static_cast<HTMLDataListElement*>(dataList);

Readability of this code isn't good.
 - Not following an idiom of parent traversal
 - static_cast<> is far from its requirement (hasTagName(datalistTag))

How about the following?

for (ContainerNode parent = parentNode(); parent ; parent =
parent->parentNode()) {
    if (parent->hasTagName(datalistTag))
	return static_cast<HTMLDataListElement*>(dataList);
}
return 0;

> LayoutTests/ChangeLog:12
> +	   * fast/forms/datalist/update-range-with-datalist-expected.txt:
Added.
> +	   * fast/forms/datalist/update-range-with-datalist.html: Added.
> +	   *
platform/chromium-mac/fast/forms/datalist/update-range-with-datalist-expected.p
ng: Added.

This can be a ref-test.