[webkit-reviews] review denied: [Bug 56055] Hook up new AppKit autocorrection UI with WK2. : [Attachment 86252] Patch (v2)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 86252: Patch (v2)
https://bugs.webkit.org/attachment.cgi?id=86252&action=review

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

> Source/WebCore/editing/Editor.cpp:2628
>      if (client())
> -	   client()->dismissCorrectionPanel(reasonForDismissing);
> +	   return client()->dismissCorrectionPanelSoon(reasonForDismissing);
> +    else
> +	   return String();

We normally do not put else after return in WebKit code. And we use early
return style, returning when there is an error. So it should be:

    if (!client())
	return String();
    return client()->dismissCorrectionPanelSoon(reasonForDismissing);

> Source/WebKit/mac/WebCoreSupport/CorrectionPanel.mm:50
> +    m_resultCondition.adoptNS([[NSCondition alloc] init]);

You can do that allocation in the constructor:

    , m_resultCondition(AdoptNS, [[NSCondition alloc] init])

> Source/WebKit/mac/WebCoreSupport/CorrectionPanel.mm:80
> +    [spellChecker showCorrectionBubbleOfType:bubbleType
primaryString:replacementStringAsNSString alternativeStrings:alternativeStrings
forStringInRect:boundingBoxOfReplacedString view:m_view.get()
completionHandler:^(NSString* acceptedString) {

This completion handler block is so big that I think you should factor most of
it into a function.

What prevents this block from being called after the CorrectionPanel is
deleted?

> Source/WebKit/mac/WebCoreSupport/CorrectionPanel.mm:122
> +    if (!isShowing()) {
> +	   return String();
> +    }

We normally do not use braces around a single line like this.

> Source/WebKit/mac/WebCoreSupport/CorrectionPanel.mm:131
> +void
CorrectionPanel::dismissInternal(WebCore::ReasonForDismissingCorrectionPanel
reason, bool dismissingExternally)

Extra WebCore:: here.

> Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:891
> +void
WebEditorClient::showCorrectionPanel(WebCore::CorrectionPanelInfo::PanelType
panelType, const FloatRect& boundingBoxOfReplacedString, const String&
replacedString, const String& replacementString, const Vector<String>&
alternativeReplacementStrings) {

We put braces on the next line, not on the end of this line. Extra WebCore::
here.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2699
> +#if PLATFORM(MAC) && !defined(BUILDING_ON_SNOW_LEOPARD)
> +    dismissCorrectionPanel(ReasonForDismissingCorrectionPanelIgnored);
> +#endif

This change seems wrong; we are quite early in the loading process at the point
where we set a pending API request URL, and that’s not the right time to take
down the correction panel. And most loads don’t have a pending API request URL,
such as clicks on links.

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

We normally use int32_t rather than int in messages like these.

> Source/WebKit2/UIProcess/mac/CorrectionPanel.h:55
> +    WTF::String dismissSoon(WebCore::ReasonForDismissingCorrectionPanel);
> +    static void recordAutocorrectionResponse(WKView*, NSCorrectionResponse,
const WTF::String& replacedString, const WTF::String& replacementString);
> +
> +private:
> +    bool isShowing() const { return m_view; }
> +    void dismissInternal(WebCore::ReasonForDismissingCorrectionPanel, bool
dismissingExternally);
> +
> +    bool m_wasDismissedExternally;
> +    WebCore::ReasonForDismissingCorrectionPanel m_reasonForDismissing;
> +    WTF::RetainPtr<WKView> m_view;
> +    WTF::RetainPtr<NSString> m_resultForSynchronousDismissal;
> +    WTF::RetainPtr<NSCondition> m_resultCondition;

No need for the WTF:: prefix on String or RetainPtr.

> Source/WebKit2/UIProcess/mac/CorrectionPanel.mm:57
> +    m_resultCondition.adoptNS([NSCondition new]);

Same comments as above. But especially strange to use new here instead of alloc
init.

> Source/WebKit2/UIProcess/mac/CorrectionPanel.mm:87
> +    [spellChecker showCorrectionBubbleOfType:bubbleType
primaryString:replacementStringAsNSString alternativeStrings:alternativeStrings
forStringInRect:boundingBoxOfReplacedString view:m_view.get()
completionHandler:^(NSString* acceptedString) {

Same comment as above about the size of this block.