[webkit-reviews] review denied: [Bug 61931] WebKitTestRunner needs an implementation of setTextDirection : [Attachment 98321] Patch v3 (fixed a memory leak)

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has denied Hironori Bono
<hbono at chromium.org>'s request for review:
Bug 61931: WebKitTestRunner needs an implementation of setTextDirection
https://bugs.webkit.org/show_bug.cgi?id=61931

Attachment 98321: Patch v3 (fixed a memory leak)
https://bugs.webkit.org/attachment.cgi?id=98321&action=review

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
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.

> 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.

> 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.