[webkit-reviews] review denied: [Bug 61931] WebKitTestRunner needs an implementation of setTextDirection : [Attachment 98476] Patch v4 (applied comments)

[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 98476: Patch v4 (applied comments)
https://bugs.webkit.org/attachment.cgi?id=98476&action=review

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