WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
61931
WebKitTestRunner needs an implementation of setTextDirection
https://bugs.webkit.org/show_bug.cgi?id=61931
Summary
WebKitTestRunner needs an implementation of setTextDirection
Jessie Berlin
Reported
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.
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
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Jessie Berlin
Comment 1
2011-06-02 09:26:00 PDT
Added to the mac-wk2 skipped list in
http://trac.webkit.org/changeset/87917
Jessie Berlin
Comment 2
2011-06-02 09:26:42 PDT
<
rdar://problem/9542509
>
Hironori Bono
Comment 3
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
Hironori Bono
Comment 4
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
Jessie Berlin
Comment 5
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?
Hironori Bono
Comment 6
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
Adam Roben (:aroben)
Comment 7
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.
Hironori Bono
Comment 8
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
Adam Roben (:aroben)
Comment 9
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.
Adam Roben (:aroben)
Comment 10
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.
Hironori Bono
Comment 11
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
Adam Roben (:aroben)
Comment 12
2011-06-27 05:42:22 PDT
Comment on
attachment 98697
[details]
Patch v5 Looks great!
WebKit Review Bot
Comment 13
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
Hajime Morrita
Comment 14
2011-07-11 19:05:33 PDT
Created
attachment 100415
[details]
For cqing again.
WebKit Review Bot
Comment 15
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
>
WebKit Review Bot
Comment 16
2011-07-11 19:52:01 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug