[Webkit-unassigned] [Bug 50961] <title> should support dir attribute

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=50961


Eric Seidel <eric at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #87148|review?                     |review-
               Flag|                            |




--- Comment #3 from Eric Seidel <eric at webkit.org>  2011-03-28 08:42:15 PST ---
(From update of attachment 87148)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.