[webkit-reviews] review granted: [Bug 50961] <title> should support dir attribute : [Attachment 87293] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Evan Martin <evan at chromium.org>'s
request for review:
Bug 50961: <title> should support dir attribute
https://bugs.webkit.org/show_bug.cgi?id=50961

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=87293&action=review

Seems OK.

> Source/WebCore/dom/Document.h:816
> +    String title() const { return m_title.string(); }
> +    void setTitle(const String&); // Used by DOM bindings; no direction
known.

These two are used by teh DOM bindings, and you could separate them by a
newline to indicate that.  But this is also OK.

> Source/WebCore/html/HTMLTitleElement.cpp:37
> +    , m_title("", LTR)

This is no longer needed, right?

> Source/WebCore/loader/FrameLoader.cpp:615
> -	   if (!ptitle.isNull())
> +	   if (!ptitle.isEmpty())

We decided that the old isNull behavior was in error?

> Source/WebCore/platform/text/StringWithDirection.h:50
> +    StringWithDirection(const String& string, TextDirection dir) :
m_string(string), m_direction(dir) {}

Should this have a default parameter for dir?  Then this could be implicitly
constructed from string.  i'm not sure if that's good or bad.