[Webkit-unassigned] [Bug 56055] Hook up new AppKit autocorrection UI with WK2.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=56055


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #86252|review?                     |review-
               Flag|                            |




--- Comment #6 from Darin Adler <darin at apple.com>  2011-03-21 16:32:16 PST ---
(From update of attachment 86252)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.