[webkit-reviews] review granted: [Bug 65542] Need support for dirname attribute : [Attachment 113632] Updated patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Rakesh <rakesh.kn at motorola.com>'s
request for review:
Bug 65542: Need support for dirname attribute
https://bugs.webkit.org/show_bug.cgi?id=65542

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

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


review+ because I think we could land just like this

but I didn’t set commit-queue+ because I think you may want to make some
improvements based on the comments below

> Source/WebCore/html/HTMLTextFormControlElement.cpp:576
> +	   const AtomicString& dirAttributeValue =
element->fastGetAttribute(dirAttr);

I think the non-HTMLElement coding here is not quite right. Is it right to do
anything for a "dir" attribute on an element that is not an HTML element? I
think we probably should just have:

    if (!element->isHTMLElement())
	continue;

at the start of the function, before we do anything with the dir attribute. It
would be good to have some test cases for this edge case.

> Source/WebCore/html/HTMLTextFormControlElement.cpp:587
> +	   if (equalIgnoringCase(dirAttributeValue, "auto") &&
element->isHTMLElement()) {
> +	       bool isAuto;
> +	       TextDirection textDirection = static_cast<const
HTMLElement*>(element)->directionalityIfhasDirAutoAttribute(isAuto);
> +	       return textDirection == RTL ? "rtl" : "ltr";
> +	   }

It’s good that the bad cast is now gone. But I’m not sure this does the right
thing when "auto" is specified on an element and the element is not an HTML
element. It seems strange to look at the value of a dir attribute, respect it
if it’s "rtl" or "ltr", but then continue looking at ancestors if it is “auto”.
If this attribute is really specific to HTML elements, then I think we should
filter out non-HTML elements earlier as I suggested above.

In fact, we could even write a parentHTMLElement function (not a member
function) and use that and not deal with non-HTMLElement objects at all here.

> Source/WebCore/html/TextFieldInputType.h:39
>  class SpinButtonElement;
> +class FormDataList; 

We keep lists like this alphabetical. This is not alphabetical.

> Source/WebCore/html/TextFieldInputType.h:60
> +    virtual bool appendFormData(FormDataList&, bool multipart) const;

This should be protected or private, not public.