[webkit-reviews] review denied: [Bug 89544] Redraw slider tick marks when datalist changes. : [Attachment 152984] 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 152984: Patch
https://bugs.webkit.org/attachment.cgi?id=152984&action=review

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


> Source/WebCore/html/HTMLInputElement.cpp:90
> +class ListAttributeTargetObserver : IdTargetObserver {
> +public:
> +    static PassOwnPtr<ListAttributeTargetObserver> create(const
AtomicString& id, HTMLInputElement*);
> +    virtual void idTargetChanged() OVERRIDE;
> +
> +private:
> +    ListAttributeTargetObserver(const AtomicString& id, HTMLInputElement*);
> +
> +    HTMLInputElement* m_element;
> +};

should be enclosed with #if ENABLE(DATALIST)

> Source/WebCore/html/HTMLInputElement.cpp:1522
> +{
> +    if (!inDocument())
> +	   return;
> +    m_listAttributeTargetObserver =
ListAttributeTargetObserver::create(fastGetAttribute(listAttr), this);
> +}

I know this works well.  However this code might make a reader nervous because
of possibility of leak of a wrong ListAttributeTargetObserver object.
I recommend clearing m_listAttributeTargetObserver if !inDocument(), and use
resetListAttributeTargetObserver() in removedFrom() too.

> Source/WebCore/html/HTMLInputElement.cpp:1811
> +PassOwnPtr<ListAttributeTargetObserver>
ListAttributeTargetObserver::create(const AtomicString& id, HTMLInputElement*
element)
> +{
> +    return adoptPtr(new ListAttributeTargetObserver(id, element));
> +}
> +
> +ListAttributeTargetObserver::ListAttributeTargetObserver(const AtomicString&
id, HTMLInputElement* element)
> +    : IdTargetObserver(element->treeScope()->idTargetObserverRegistry(), id)

> +    , m_element(element)
> +{
> +}
> +
> +void ListAttributeTargetObserver::idTargetChanged()
> +{
> +    m_element->listAttributeTargetChanged();
> +}

should be enclosed with #if ENABLE(DATALIST)

> Source/WebCore/html/HTMLInputElement.h:398
> +    OwnPtr<ListAttributeTargetObserver> m_listAttributeTargetObserver;

should be enclosed with #if ENABLE(DATALIST)