[webkit-reviews] review granted: [Bug 56055] Hook up new AppKit autocorrection UI with WK2. : [Attachment 86635] Patch (v5)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jia Pu <jpu at apple.com>'s request for
review:
Bug 56055: Hook up new AppKit autocorrection UI with WK2.
https://bugs.webkit.org/show_bug.cgi?id=56055

Attachment 86635: Patch (v5)
https://bugs.webkit.org/attachment.cgi?id=86635&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=86635&action=review

> Source/WebKit/mac/WebCoreSupport/CorrectionPanel.h:30
> +#import <JavaScriptCore/Platform.h>
> +#import <JavaScriptCore/RetainPtr.h>

We normally use <wtf/Platform.h> and <wtf/RetainPtr.h> even though the
<JavaScriptCore/> style works.

The Platform.h include is not needed because the prefix takes care of that.

The RetainPtr.h include can go inside the ifdefs instead of outside.

> Source/WebKit/mac/WebCoreSupport/CorrectionPanel.mm:26
> + */
> +#import "CorrectionPanel.h"
> +#import "WebViewPrivate.h"

Normally we have a blank line before the “own file” include and another after
the “own file” include.

> Source/WebKit/mac/WebCoreSupport/CorrectionPanel.mm:30
> +using namespace WTF;

You should not need a using namespace WTF because of how WTF itself invokes
using to put things into the global namespace.

> Source/WebKit/mac/WebCoreSupport/WebEditorClient.h:33
>  #import <wtf/RetainPtr.h>

This include is now redundant so can be removed.

> Source/WebKit/mac/WebCoreSupport/WebEditorClient.h:145
> +    virtual WTF::String
dismissCorrectionPanelSoon(WebCore::ReasonForDismissingCorrectionPanel);

Should be no need for the WTF::String here or anywhere else in this file.
Should just be String.

> Source/WebKit2/UIProcess/WebPageProxy.h:454
> +    void handleCorrectionPanelResult(const WTF::String& result);

Should just be String, not WTF::String.

> Source/WebKit2/UIProcess/WebPageProxy.h:654
> +    void showCorrectionPanel(int32_t panelType, const WebCore::FloatRect&
boundingBoxOfReplacedString, const WTF::String& replacedString, const
WTF::String& replacementString, const Vector<WTF::String>&
alternativeReplacementStrings);
> +    void dismissCorrectionPanel(int32_t reason);
> +    void dismissCorrectionPanelSoon(int32_t reason, String& result);
> +    void recordAutocorrectionResponse(int32_t responseType, const
WTF::String& replacedString, const WTF::String& replacementString);

Same here. Just String, not WTF::String.

> Source/WebKit2/UIProcess/WebPageProxy.messages.in:217
> +    ShowCorrectionPanel(int32_t panelType, WebCore::FloatRect
boundingBoxOfReplacedString, WTF::String replacedString, WTF::String
replacementString, Vector<WTF::String> alternativeReplacementStrings)
> +    DismissCorrectionPanel(int32_t reason)
> +    DismissCorrectionPanelSoon(int32_t reason) -> (String result)
> +    RecordAutocorrectionResponse(int32_t responseType, WTF::String
replacedString, WTF::String replacementString);

Just String, not WTF::String.

> Source/WebKit2/UIProcess/mac/CorrectionPanel.mm:35
> +using namespace WTF;

No need for this.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2135
> +void WebPage::handleCorrectionPanelResult(const WTF::String& result)

Just String, not WTF::String.

> Source/WebKit2/WebProcess/WebPage/WebPage.h:365
> +    void handleCorrectionPanelResult(const WTF::String&);

Just String, not WTF::String.

> Source/WebKit2/WebProcess/WebPage/WebPage.messages.in:197
> +    HandleCorrectionPanelResult(WTF::String result)

Just String, not WTF::String.