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

Wed Mar 16 12:10:26 PDT 2011

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

------- Additional Comments from Darin Adler <darin at apple.com>
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)