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

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied 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 87148: Patch
https://bugs.webkit.org/attachment.cgi?id=87148&action=review

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

> Source/WebCore/ChangeLog:11
> +	   Introduce a new StringWithDirection object that carries a String
along
> +	   with the TextDirection associated with the String.  Use this object
for
> +	   document titles used within WebCore.  Put FIXMEs at the WebKit level
to
> +	   expose the new direction information to clients.

You might want to explain here that we need to do this so that titles can be
stored in History as well as for display of the current page.

> Source/WebCore/dom/Document.cpp:392
> +    , m_title("", LTR)

Just have a default constructor here and then you don't even need these lines.

> Source/WebCore/dom/Document.cpp:1328
> +    m_title = StringWithDirection(canonicalizedTitle(this,
m_rawTitle.m_string), m_rawTitle.m_direction);

Why not just pass the StringWithDirection to canonicalizedTitle() and then
return one?

> Source/WebCore/dom/Document.cpp:1335
> +    setTitle(StringWithDirection(title, LTR), 0);

You should comment here that this is called by the JavaScript document.title =
"" setter and thus we always assume an LTR context.

> Source/WebCore/dom/Document.cpp:1367
> +	  
static_cast<HTMLTitleElement*>(m_titleElement.get())->setText(m_title.m_string)
;

You should just add a string() accessor instead of grabbing at m_string.

> Source/WebCore/dom/Document.h:816
> +    void setTitle(const String&);

You should comment here that these two are only used by teh DOM bindings.

> Source/WebCore/dom/Document.h:1300
> +    StringWithDirection m_title;
> +    StringWithDirection m_rawTitle;

It seems that only one of these really needs the direction, or?

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

Default constructor and this goes away. :)

> Source/WebCore/html/HTMLTitleElement.cpp:79
> +StringWithDirection HTMLTitleElement::textWithDirection()

Should this be a const method?

> Source/WebCore/html/HTMLTitleElement.cpp:81
> +    RenderStyle* style = computedStyle();

I believe this can be NULL, do we need to check that?

> Source/WebCore/loader/DocumentLoader.cpp:654
> +    if (title.m_string.isEmpty())

You might add a isEmpty() accessor to StringWithDirection.  Certainly string()
woudl be nicer than m_string here. :)

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

consider adding isNull().

> Source/WebCore/platform/text/StringWithDirection.h:55
> +    TextDirection m_direction;

I think you need to make it clear that this is the directional context of the
string.  I worry folks will get confused that this the "direction" of the
string.  It could be an LTR context but all hebrew and thus render as RTL, no?

> Source/WebCore/svg/SVGTitleElement.cpp:44
> +	   // FIXME: does SVG have a concept of text direction?
> +	   document()->setTitle(StringWithDirection(textContent(), LTR), this);


There is "direction", but I don't think it applies to title elements. 
http://www.w3.org/TR/SVG/text.html#DirectionProperty