[webkit-reviews] review requested: [Bug 61931] WebKitTestRunner needs an implementation of setTextDirection : [Attachment 98476] Patch v4 (applied comments)

[bugzilla-daemon at webkit.org]

Hironori Bono <hbono at chromium.org> has asked  for review:
Bug 61931: WebKitTestRunner needs an implementation of setTextDirection
https://bugs.webkit.org/show_bug.cgi?id=61931

Attachment 98476: Patch v4 (applied comments)
https://bugs.webkit.org/attachment.cgi?id=98476&action=review

------- Additional Comments from Hironori Bono <hbono at chromium.org>
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