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

Wed Mar 16 12:10:27 PDT 2011

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

Darin Adler <darin at apple.com> changed:

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

--- Comment #2 from Darin Adler <darin at apple.com>  2011-03-16 12:10:27 PST ---
(From update of attachment 85249)
View in context: https://bugs.webkit.org/attachment.cgi?id=85249&action=review

Looking pretty good. Still needs a little work.

> Source/WebCore/editing/Editor.h:426
>      void dismissCorrectionPanel(ReasonForDismissingCorrectionPanel);
> +    void dismissCorrectionPanel(ReasonForDismissingCorrectionPanel, String& result);

A more usual idiom for this would be to name the asynchronous version dismissCorrectionPanelSoon and use a normal return value for the synchronous version. Overloading based on an out parameter seems a little awkward to me.

> Source/WebKit/mac/WebCoreSupport/CorrectionPanelMac.h:37
> +class CorrectionPanelMac {

Not sure this class needs the word Mac in its name.

Should be marked noncopyable.

> Source/WebKit/mac/WebCoreSupport/CorrectionPanelMac.h:48
> +    void doDismiss(WebCore::ReasonForDismissingCorrectionPanel, bool dismissingExternally);

Not a great name for this function. It’s unclear how dismiss is different from doDismiss.

> Source/WebKit/mac/WebCoreSupport/CorrectionPanelMac.h:50
> +    bool m_dismissedExternally;

Normally we would call this m_wasDismissedExternally.

> Source/WebKit/mac/WebCoreSupport/CorrectionPanelMac.h:54
> +    WebView* m_view;
> +    NSString* m_resultForSynchronousDismissal;
> +    NSCondition* m_resultCondition;

For Objective-C classes we normally use a different style of where to put the*.

> Source/WebKit/mac/WebCoreSupport/CorrectionPanelMac.mm:40
> +        case CorrectionPanelInfo::PanelTypeCorrection:
> +            return NSCorrectionBubbleTypeCorrection;
> +        case CorrectionPanelInfo::PanelTypeReversion:
> +            return NSCorrectionBubbleTypeReversion;
> +        case CorrectionPanelInfo::PanelTypeSpellingSuggestions:
> +            return NSCorrectionBubbleTypeGuesses;

This is not the WebKit style of how to line up switch statements.

> Source/WebKit/mac/WebCoreSupport/CorrectionPanelMac.mm:52
> +    m_resultCondition = [[NSCondition alloc] init];

This won’t work under GC. You need to have m_resultCondition be a RetainPtr instead.

> Source/WebKit/mac/WebCoreSupport/CorrectionPanelMac.mm:60
> +    [m_resultCondition release];
> +    if (m_resultForSynchronousDismissal)
> +        [m_resultForSynchronousDismissal release];

These won’t work under GC. Both m_resultCondition and m_resultForSynchronousDismissal should be RetainPtr.

> Source/WebKit/mac/WebCoreSupport/CorrectionPanelMac.mm:133
> +    result = String(m_resultForSynchronousDismissal);

The explicit String() should not be needed here.

> Source/WebKit/mac/WebCoreSupport/CorrectionPanelMac.mm:152
> +    if (reason == ReasonForDismissingCorrectionPanelAccepted)
> +        [[NSSpellChecker sharedSpellChecker] dismissCorrectionBubbleForView:m_view];
> +    else
> +        [[NSSpellChecker sharedSpellChecker] cancelCorrectionBubbleForView:m_view];
> +    m_view = 0;

What’s the guarantee that m_view is still good here? I suggest making it a RetainPtr.

> Source/WebKit/mac/WebView/WebView.mm:5315
> +        coreFrame->editor()->handleCorrectionPanelResult(String(result));

Should not need an explicit String() here.

> Source/WebKit2/UIProcess/API/mac/PageClientImpl.h:116
> +#if !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD)