Bug 61931 – WebKitTestRunner needs an implementation of setTextDirection

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jessie Berlin]

fast/html/set-text-direction.html has been failing on SnowLeopard Intel Release (WebKit2 Tests) since its introduction in http://trac.webkit.org/changeset/87770.

http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(WebKit2%20Tests)/r87768%20(12169)/results.html passed
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(WebKit2%20Tests)/r87770%20(12170)/results.html failed

I am going to add it to the mac-wk2 skipped list for now.

[Jessie Berlin]

Added to the mac-wk2 skipped list in http://trac.webkit.org/changeset/87917

[Jessie Berlin]

<rdar://problem/9542509>

[Hironori Bono]

Created attachment 98124 [details]
Patch v1

Greetings,

I have quickly implemented this function for WebKit2 and Windows. Even though I have created this change without deep knowledge about them and it may be wrong, would it be possible to review this change and give me your comments?

Regards,

Hironori Bono

[Hironori Bono]

Created attachment 98127 [details]
Patch v2 (fixed style issues)

Greetings,

Unfortunately, I have uploaded a wrong change. (This is the one before I fixed style issues.) I have uploaded the correct one.

Regards,

Hironori Bono

[Jessie Berlin]

Comment on attachment 98127 [details]
Patch v2 (fixed style issues)

View in context: https://bugs.webkit.org/attachment.cgi?id=98127&action=review

Unofficial (I am not a reviewer yet) r=me if you fix the leak.

> Tools/DumpRenderTree/win/LayoutTestControllerWin.cpp:1577
> +    framePrivate->setTextDirection(directionBSTR);

Potential leak: where is directionBSTR freed?

[Hironori Bono]

Created attachment 98321 [details]
Patch v3 (fixed a memory leak)

Greetings Jessie,

Thank you for your review and comment.

(In reply to comment #5)
> (From update of attachment 98127 [details])
> > Tools/DumpRenderTree/win/LayoutTestControllerWin.cpp:1577
> > +    framePrivate->setTextDirection(directionBSTR);
> 
> Potential leak: where is directionBSTR freed?

Thank you for noticing this leak. I totally forgot calling SysFreeString(). I have added a SysFreeString() call after this line.

Regards,

Hironori Bono

[Adam Roben (:aroben)]

Comment on attachment 98321 [details]
Patch v3 (fixed a memory leak)

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.

[Hironori Bono]

Created attachment 98476 [details]
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

[Adam Roben (:aroben)]

Comment on attachment 98321 [details]
Patch v3 (fixed a memory leak)

View in context: https://bugs.webkit.org/attachment.cgi?id=98321&action=review

>>> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp:327
>>> +}
>> 
>> 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.

Yes, I think WKBundleFrameSetTextDirection should call WebFrame::setTextDirection directly.

[Adam Roben (:aroben)]

Comment on attachment 98476 [details]
Patch v4 (applied comments)

View in context: https://bugs.webkit.org/attachment.cgi?id=98476&action=review

This looks really close!

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundle.cpp:219
> +void WKBundleFrameSetTextDirection(WKBundleRef bundleRef, WKBundleFrameRef frameRef, WKStringRef directionRef)
> +{
> +    toImpl(bundleRef)->setTextDirection(toImpl(frameRef), toImpl(directionRef)->string());
> +}

This function should be in WKBundleFrame.cpp, and shouldn't take a WKBundleRef parameter.

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePrivate.h:86
> +WK_EXPORT void WKBundleFrameSetTextDirection(WKBundleRef bundle, WKBundleFrameRef frameRef, WKStringRef);

This should be in WKBundleFramePrivate.h.

> Source/WebKit/win/WebFrame.cpp:1065
> +HRESULT STDMETHODCALLTYPE WebFrame::setTextDirection(
> +        /* [in] */ BSTR direction)

You can remove the STDMETHODCALLTYPE (it's only needed on the declaration, not the definition) and the /* [in] */ comment (even though most other functions in this file have left them in place). We'd like to be as concise as possible.

[Hironori Bono]

Created attachment 98697 [details]
Patch v5

Greetings Adam,

Thank you for your review and comments. (This is a very good practice about WebKitTestRunner for me.)

(In reply to comment #10)
> This function should be in WKBundleFrame.cpp, and shouldn't take a WKBundleRef parameter.

Thank you for noticing it. I did not noticed WKBundleFrame.cpp. I have moved this WKBundleFrameSetTextDirection. (It makes this change simpler.)

> This should be in WKBundleFramePrivate.h.

Done.

> You can remove the STDMETHODCALLTYPE (it's only needed on the declaration, not the definition) and the /* [in] */ comment (even though most other functions in this file have left them in place). We'd like to be as concise as possible.

Thank you so much for your description. (I have pasted this code just because every other function uses this style even though I'm not sure why.) This description answers my question.

Regards,

Hironori Bono

[Adam Roben (:aroben)]

Comment on attachment 98697 [details]
Patch v5

Looks great!

[WebKit Review Bot]

Comment on attachment 98697 [details]
Patch v5

Rejecting attachment 98697 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-02', '--port..." exit_code: 2

Last 500 characters of output:
edBundle/Bindings/LayoutTestController.idl
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/platform/mac-wk2/Skipped
Hunk #1 FAILED at 409.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/mac-wk2/Skipped.rej
patching file LayoutTests/platform/win/Skipped
Hunk #1 succeeded at 1014 (offset 7 lines).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Adam Roben', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/9000381

[Hajime Morrita]

Created attachment 100415 [details]
For cqing again.

[WebKit Review Bot]

Comment on attachment 100415 [details]
For cqing again.

Clearing flags on attachment: 100415

Committed r90806: <http://trac.webkit.org/changeset/90806>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.