Bug 61931 - WebKitTestRunner needs an implementation of setTextDirection
Summary: WebKitTestRunner needs an implementation of setTextDirection
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Hajime Morrita
URL: http://build.webkit.org/results/SnowL...
Keywords: InRadar, LayoutTestFailure
Depends on:
Blocks:
 
Reported: 2011-06-02 08:57 PDT by Jessie Berlin
Modified: 2011-07-11 19:52 PDT (History)
6 users (show)

See Also:


Attachments
Patch v1 (13.02 KB, patch)
2011-06-21 23:45 PDT, Hironori Bono
no flags Details | Formatted Diff | Diff
Patch v2 (fixed style issues) (13.00 KB, patch)
2011-06-22 00:00 PDT, Hironori Bono
no flags Details | Formatted Diff | Diff
Patch v3 (fixed a memory leak) (13.28 KB, patch)
2011-06-23 01:02 PDT, Hironori Bono
aroben: review-
Details | Formatted Diff | Diff
Patch v4 (applied comments) (14.30 KB, patch)
2011-06-24 03:09 PDT, Hironori Bono
aroben: review-
Details | Formatted Diff | Diff
Patch v5 (12.93 KB, patch)
2011-06-27 04:08 PDT, Hironori Bono
no flags Details | Formatted Diff | Diff
For cqing again. (13.97 KB, patch)
2011-07-11 19:05 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jessie Berlin 2011-06-02 08:57:22 PDT
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.
Comment 1 Jessie Berlin 2011-06-02 09:26:00 PDT
Added to the mac-wk2 skipped list in http://trac.webkit.org/changeset/87917
Comment 2 Jessie Berlin 2011-06-02 09:26:42 PDT
<rdar://problem/9542509>
Comment 3 Hironori Bono 2011-06-21 23:45:29 PDT
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
Comment 4 Hironori Bono 2011-06-22 00:00:58 PDT
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
Comment 5 Jessie Berlin 2011-06-22 07:15:49 PDT
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?
Comment 6 Hironori Bono 2011-06-23 01:02:52 PDT
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
Comment 7 Adam Roben (:aroben) 2011-06-23 11:58:49 PDT
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.
Comment 8 Hironori Bono 2011-06-24 03:09:11 PDT
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
Comment 9 Adam Roben (:aroben) 2011-06-24 09:04:07 PDT
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.
Comment 10 Adam Roben (:aroben) 2011-06-24 09:06:15 PDT
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.
Comment 11 Hironori Bono 2011-06-27 04:08:15 PDT
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
Comment 12 Adam Roben (:aroben) 2011-06-27 05:42:22 PDT
Comment on attachment 98697 [details]
Patch v5

Looks great!
Comment 13 WebKit Review Bot 2011-07-07 21:21:07 PDT
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
Comment 14 Hajime Morrita 2011-07-11 19:05:33 PDT
Created attachment 100415 [details]
For cqing again.
Comment 15 WebKit Review Bot 2011-07-11 19:51:55 PDT
Comment on attachment 100415 [details]
For cqing again.

Clearing flags on attachment: 100415

Committed r90806: <http://trac.webkit.org/changeset/90806>
Comment 16 WebKit Review Bot 2011-07-11 19:52:01 PDT
All reviewed patches have been landed.  Closing bug.