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.
Added to the mac-wk2 skipped list in http://trac.webkit.org/changeset/87917
<rdar://problem/9542509>
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
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 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?
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 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.
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 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 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.
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 on attachment 98697 [details] Patch v5 Looks great!
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
Created attachment 100415 [details] For cqing again.
Comment on attachment 100415 [details] For cqing again. Clearing flags on attachment: 100415 Committed r90806: <http://trac.webkit.org/changeset/90806>
All reviewed patches have been landed. Closing bug.