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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 113458: Updated patch
https://bugs.webkit.org/attachment.cgi?id=113458&action=review

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


> Source/WebCore/html/HTMLTextAreaElement.cpp:178
> +    if (fastHasAttribute(dirnameAttr))
> +	   encoding.appendData(fastGetAttribute(dirnameAttr),
directionForFormData());

This is less efficient than it could be. The right way to write it is:

    const AtomicString& dirname = fastGetAttribute(dirnameAttr);
    if (!dirname.isNull())
	encoding.appendData(dirname, directionForFormData());

> Source/WebCore/html/HTMLTextFormControlElement.cpp:576
> +    const Element* element = this;
> +    while (element) {

This should be written as a for loop, not a while loop.

> Source/WebCore/html/HTMLTextFormControlElement.cpp:578
> +	   if (element->fastHasAttribute(dirAttr)) {
> +	       AtomicString
dirAttributeValue(element->fastGetAttribute(dirAttr));

Not as efficient as it could be. Should be written like this:

    const AtomicString& dirAttributeValue = element->fastGetAttribute(dirAttr);

    if (dirAttributeValue.isNull())
	continue;
    ...

Note that we use “early continue” style instead of nesting the entire loop in
an if statement.

> Source/WebCore/html/HTMLTextFormControlElement.cpp:584
> +		   TextDirection textDirection = static_cast<const
HTMLElement*>(element)->directionalityIfhasDirAutoAttribute(isAuto);

This cast to an HTMLElement can be a bad cast. Webpages can put an HTML element
inside a non-HTML element, such as an SVG element. If we want to assume it’s an
HTMLElement we need to actually check isHTMLElement. A bad cast can cause
crashes, in some cases crashes that are exploitable security vulnerabilities.

> Source/WebCore/html/TextFieldInputType.cpp:375
> +    if (element()->fastHasAttribute(dirnameAttr))
> +	   list.appendData(element()->fastGetAttribute(dirnameAttr),
element()->directionForFormData());

Same point about a more efficient idiom as I mentioned above.