[Webkit-unassigned] [Bug 61931] WebKitTestRunner needs an implementation of setTextDirection

[bugzilla-daemon at webkit.org]

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


Hironori Bono <hbono at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #98321|0                           |1
        is obsolete|                            |
  Attachment #98476|                            |review?
               Flag|                            |




--- Comment #8 from Hironori Bono <hbono at chromium.org>  2011-06-24 03:09:11 PST ---
Created an attachment (id=98476)
 --> (https://bugs.webkit.org/attachment.cgi?id=98476&action=review)
Patch v4 (applied comments)

Greeting Adam,

Thank you for your review and comments.
I have updated my change to apply your comments. (I may misunderstand your comments, though.)

(In reply to comment #7)
> (From update of attachment 98321 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98321&action=review
> 
> Thanks for implementing this! I think this could use another pass.
> 
> > Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp:327
> > +void InjectedBundle::setTextDirection(WebFrame* frame, const String& direction)
> > +{
> > +    Frame* coreFrame = frame ? frame->coreFrame() : 0;
> > +    if (!coreFrame || !coreFrame->editor())
> > +        return;
> > +
> > +    if (direction == "auto")
> > +        coreFrame->editor()->setBaseWritingDirection(NaturalWritingDirection);
> > +    else if (direction == "ltr")
> > +        coreFrame->editor()->setBaseWritingDirection(LeftToRightWritingDirection);
> > +    else if (direction == "rtl")
> > +        coreFrame->editor()->setBaseWritingDirection(RightToLeftWritingDirection);
> > +}
> 
> This function should be a member of WebFrame, not InjectedBundle. The corresponding API would be WKBundleFrameSetTextDirection.

I have added a new function WebFrame::setTextDirection and moved this code there. Even though I have renamed WKBundleSetTextDirection to WKBundleFrameSetTextDirection, I left this InjectedBundle::setTextDirection (it just calls WebFrame::setTextDirection) since I'm not sure if WKBundleFrameSetTextDirection should call WebFrame::setTextDirection directly.

> > Source/WebKit/win/Interfaces/IWebFramePrivate.idl:122
> > +    HRESULT setTextDirection([in] BSTR direction);
> 
> New functions must be added to the end of the interface to avoid breaking nightly builds.

Done. Thank you for noticing it.

> > Tools/DumpRenderTree/win/LayoutTestControllerWin.cpp:1578
> > +    wstring directionWstring = jsStringRefToWString(direction);
> > +    BSTR directionBSTR = SysAllocStringLen((OLECHAR*)directionWstring.c_str(), directionWstring.length());
> > +    framePrivate->setTextDirection(directionBSTR);
> > +    SysFreeString(directionBSTR);
> 
> Please use the bstrT() helper function instead of doing this manually.

Done. Thank you for noticing it.

Regards,

Hironori Bono

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