[webkit-reviews] review granted: [Bug 68773] BDI element should have dir=auto by default : [Attachment 133130] Removed the extra change log entry

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 68773: BDI element should have dir=auto by default
https://bugs.webkit.org/show_bug.cgi?id=68773

Attachment 133130: Removed the extra change log entry
https://bugs.webkit.org/attachment.cgi?id=133130&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=133130&action=review


This patch looks straight forward to me. I had some minor comments.

> Source/WebCore/css/CSSStyleSelector.cpp:1370
> +    if ((element->isHTMLElement() &&
toHTMLElement(element)->hasDirectionAuto()) || (m_element->isHTMLElement() &&
toHTMLElement(m_element)->hasDirectionAuto()))

The disjuncts in this disjunction are the same up to the object they operate
on. You may want to consider extracting a disjunct into an inline function, say
elementHasDirectionAuto() or doesElementHaveDirectionAuto?

inline bool elementHasDirectionAuto(Element* element)
{
    return element->isHTMLElement() &&
toHTMLElement(element)->hasDirectionAuto();
}

Then you can write this line as:

if (elementHasDirectionAuto(element) || elementHasDirectionAuto(m_element)) {
    ...
}

> Source/WebCore/html/HTMLBDIElement.h:41
> +	   fprintf(stderr, "HTMLBDIElement:%s\n",
selfOrAncestorHasDirAutoAttribute() ? "true" : "false");

Did you mean to have this fprintf() in the patch?

> Source/WebCore/html/HTMLBDIElement.h:45
> +} // namespace

This should read:

// namespace WebCore

> Source/WebCore/html/HTMLElement.cpp:945
> +    while (oldMarkedNode && oldMarkedNode->isHTMLElement() &&
elementAffectsDirectionality(toHTMLElement(oldMarkedNode)))

As we talked about on IRC, you may want to consider having
elementAffectsDirectionality() take a const Node* instead of a const Element*.
And moving the HTMLElement cast (it's sufficient to call toElement()) into
elementAffectsDirectionality() such that we can write this line as:

while (oldMarkedNode && oldMarkedNode->isHTMLElement() &&
elementAffectsDirectionality(oldMarkedNode))