Bug 56055 – Hook up new AppKit autocorrection UI with WK2.

• Component: New Bugs
• Status: RESOLVED FIXED

[Jia Pu]

This task is to bring autocorrection UI in WK2 on Mac OS X to parity with that in WK1.
<rdar://problem/8947463>

[Jia Pu]

Created attachment 85249 [details]
Patch (v1)

[Darin Adler]

Comment on attachment 85249 [details]
Patch (v1)

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)

WebKit2 doesn’t support Tiger or Leopard, so there is no need to mention them here.

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

WebKit2 doesn’t support Tiger or Leopard, so there is no need to mention them here.

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

WebKit2 doesn’t support Tiger or Leopard, so there is no need to mention them here.

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

WebKit2 doesn’t support Tiger or Leopard, so there is no need to mention them here.

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

WebKit2 doesn’t support Tiger or Leopard, so there is no need to mention them here.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:2050
> +    _data->_page->handleCorrectionPanelResult(WTF::String(result));

Should not need an existing WTF::String here.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2805
> +#if PLATFORM(MAC) && !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD)

WebKit2 doesn’t support Tiger or Leopard, so there is no need to mention them here.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2885
> +#if PLATFORM(MAC) && !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD)

WebKit2 doesn’t support Tiger or Leopard, so there is no need to mention them here.

> Source/WebKit2/UIProcess/WebPageProxy.h:657
> +#if PLATFORM(MAC) && !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD)

WebKit2 doesn’t support Tiger or Leopard, so there is no need to mention them here.

> Source/WebKit2/UIProcess/WebPageProxy.messages.in:212
> +#if PLATFORM(MAC) && !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD)

WebKit2 doesn’t support Tiger or Leopard, so there is no need to mention them here.

> Source/WebKit2/UIProcess/WebPageProxy.messages.in:214
> +    ShowCorrectionPanel(WebCore::CorrectionPanelInfo::PanelType panelType, WebCore::FloatRect boundingBoxOfReplacedString, WTF::String replacedString, WTF::String replacementString, Vector<WTF::String> alternativeReplacementStrings)

We normally just use integers instead of adding enums to the cross process machinery for things like panelType.

> Source/WebKit2/UIProcess/WebPageProxy.messages.in:217
> +    RecordAutocorrectionResponse(WebCore::EditorClient::AutocorrectionResponseType responseType, WTF::String replacedString, WTF::String replacementString);

We normally just use integers instead of adding enums to the cross process machinery for things like responseType.

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

WebKit2 doesn’t support Tiger or Leopard, so there is no need to mention them here.

> Source/WebKit2/UIProcess/mac/CorrectionPanelMac.mm:27
> +#if !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD)

WebKit2 doesn’t support Tiger or Leopard, so there is no need to mention them here.

> Source/WebKit2/WebProcess/WebCoreSupport/mac/WebEditorClientMac.mm:249
>  #if !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD)

WebKit2 doesn’t support Tiger or Leopard, so there is no need to mention them here.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2110
> +#if PLATFORM(MAC) && !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD)

WebKit2 doesn’t support Tiger or Leopard, so there is no need to mention them here.

> Source/WebKit2/WebProcess/WebPage/WebPage.h:359
> +#if PLATFORM(MAC) && !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD)

WebKit2 doesn’t support Tiger or Leopard, so there is no need to mention them here.

> Source/WebKit2/WebProcess/WebPage/WebPage.messages.in:196
> +#if PLATFORM(MAC) && !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD)

WebKit2 doesn’t support Tiger or Leopard, so there is no need to mention them here.

[Jia Pu]

(In reply to comment #2)
> (From update of attachment 85249 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85249&action=review
> 
> > 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.
> 

doDismiss() contains the actual implementation, but shall not be called directly. It's used by both dismiss() and two other public functions().

dismiss() is public function which always pass false as second argument to doDismiss().

I'm not sure what kind of naming convention WebKit uses in situation like this. Maybe just use the same name?

[Darin Adler]

(In reply to comment #3)
> I'm not sure what kind of naming convention WebKit uses in situation like this. Maybe just use the same name?

Often we look for some difference in the functions to use to change the names. Other times we have used an “internal” suffix, when we couldn’t come up with anything better.

[Jia Pu]

Created attachment 86252 [details]
Patch (v2)

[Darin Adler]

Comment on attachment 86252 [details]
Patch (v2)

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.

[Jia Pu]

(In reply to comment #6)
> (From update of attachment 86252 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86252&action=review

> 
> > 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?

The lifetime of correction panel is managed by AppKit internally. When a NSView (or WebView) is deleted, AppKit makes sure to detach the correction panel if there is one. And NSCorrectionPanel will not call back if it's not attached to a window anymore.

> > 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.


Do you have a recommended spot to dismiss visible correction panel before loading a new page? In WK1, I don't actually need to do anything. In WK1, reloading a page will always dealloc NSView, hence dismiss the panel. However, in WK2, it doesn't seem always to be the case. Specifically when reloading another page when existing page is OpenSource/Source/WebCore/manual-tests/autocorrection/autocorrection-in-iframe.html.

[Jia Pu]

Created attachment 86505 [details]
Patch (v3)

[Jia Pu]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 86252 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=86252&action=review
> 
> > > 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.
> 
> 
> Do you have a recommended spot to dismiss visible correction panel before loading a new page? In WK1, I don't actually need to do anything. In WK1, reloading a page will always dealloc NSView, hence dismiss the panel. However, in WK2, it doesn't seem always to be the case. Specifically when reloading another page when existing page is OpenSource/Source/WebCore/manual-tests/autocorrection/autocorrection-in-iframe.html.

I took a closer look at this issue. The reason that we don't need to explicitly dismiss correction panel when loading a new page in WK1 is that there's code in AppKit to take care this. Whenever [WebFrameView _setDocumentView:] is called, which calls [NSScrollView setDocumentView:], AppKit find the WebView to which WebFrameView belongs, and removes the correction panel if it exists. This is rather fragile. So I think we should handle this within WK2. I moved the call to dismissCorrectionPanel() from setPendingAPIRequestURL() to didFinishDocumentLoadForFrame().

[Darin Adler]

(In reply to comment #9)
> I moved the call to dismissCorrectionPanel() from setPendingAPIRequestURL() to didFinishDocumentLoadForFrame().

It should be in didCommitLoadForFrame, which is when we switch to a new document, rather than didFinishDocumentLoadForFrame, called only when we finish loading the new document, which could be a long time later.

[Jia Pu]

Created attachment 86516 [details]
Patch (v4)

[Darin Adler]

Comment on attachment 86516 [details]
Patch (v4)

View in context: https://bugs.webkit.org/attachment.cgi?id=86516&action=review

No time for a complete review, but I did get a chance to look over part of this.

> Source/WebKit/mac/WebCoreSupport/CorrectionPanel.h:46
> +    WTF::String dismissSoon(WebCore::ReasonForDismissingCorrectionPanel);
> +    static void recordAutocorrectionResponse(WebView*, NSCorrectionResponse, const WTF::String& replacedString, const WTF::String& replacementString);

This should just be String, not WTF::String.

> Source/WebKit/mac/WebCoreSupport/CorrectionPanel.h:57
> +    WTF::RetainPtr<WebView> m_view;
> +    WTF::RetainPtr<NSString> m_resultForSynchronousDismissal;
> +    WTF::RetainPtr<NSCondition> m_resultCondition;

This should just be RetainPtr, not WTF::RetainPtr.

> Source/WebKit/mac/WebCoreSupport/CorrectionPanel.mm:103
> +    m_wasDismissedExternally = dismissingExternally;

I don’t think we really want to set this to false if it was already set to true, and this was called an extra redundant time.

[Jia Pu]

(In reply to comment #12)
> (From update of attachment 86516 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86516&action=review
> 
> > Source/WebKit/mac/WebCoreSupport/CorrectionPanel.mm:103
> > +    m_wasDismissedExternally = dismissingExternally;
> 
> I don’t think we really want to set this to false if it was already set to true, and this was called an extra redundant time.

Sorry, I don't quite follow this comment. 

Every time dismissInternal() is called, we update m_wasDismissedExternally to keep track whether the dismissal is issued by caller who explicitly call dismiss() and dismissSoon(), in which case dismissingExternally is true. When dismissInternal() is called by ~CorrectionPanel() or show(), this argument is false. And in each of  these cases, dismissInternal() is called only once.

[Jia Pu]

Created attachment 86635 [details]
Patch (v5)

Removed unnecessary WTF namespace.

[Darin Adler]

Comment on attachment 86635 [details]
Patch (v5)

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.

[Jia Pu]

Created attachment 86659 [details]
Patch (v6)

Removed more unnecessary WTF namespace. Merged TOT.

[Darin Adler]

Comment on attachment 86659 [details]
Patch (v6)

The patch is not applying for some reason, so I can’t set commit-queue+.

[Jia Pu]

Created attachment 86694 [details]
Patch (v7)

Resolved merge conflict on WK2 project file.

[WebKit Commit Bot]

Comment on attachment 86694 [details]
Patch (v7)

Rejecting attachment 86694 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'build'..." exit_code: 2

Last 500 characters of output:
 setenv YACC /Developer/usr/bin/yacc
    /bin/sh -c /mnt/git/webkit-commit-queue/WebKitBuild/WebKit.build/Debug/WebKit.build/Script-5D88EE6C11407DE800BC3ABC.sh

** BUILD FAILED **


The following build commands failed:
WebKit:
	CompileC /mnt/git/webkit-commit-queue/WebKitBuild/WebKit.build/Debug/WebKit.build/Objects-normal/x86_64/WebEditorClient.o /mnt/git/webkit-commit-queue/Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm normal x86_64 objective-c++ com.apple.compilers.gcc.4_2
(1 failure)


Full output: http://queues.webkit.org/results/8236024

[Jia Pu]

Created attachment 86722 [details]
Patch (v8)

Fixed build failures.

[Jia Pu]

We need to either put "#import <wtf/RetainPtr.h>" above #if !defined(BUILDING_ON_SNOW_LEOPARD) in CorrectionPanel.h, or keep "#import <wtf/RetainPtr.h>" in WebEditorClient.h and PageClientImpl.h.

I chose the latter to maintain the alphabetic order.

[WebKit Commit Bot]

Comment on attachment 86722 [details]
Patch (v8)

Clearing flags on attachment: 86722

Committed r81847: <http://trac.webkit.org/changeset/81847>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.