Bug 51049 – IME support in WebKit2 on Windows

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Enrica Casucci]

Adding IME support to WebKit2 on Windows.

[Enrica Casucci]

Created attachment 76553 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 76553 [details]
Patch

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

> WebKit2/ChangeLog:59
> +        * UIProcess/API/qt/qwkpage.cpp:
> +        (QWKPagePrivate::compositionSelectionChanged): Added empty stub.
> +        * UIProcess/API/qt/qwkpage_p.h:
> +        * UIProcess/PageClient.h: Added compositionSelectionChanged.
> +        * UIProcess/WebPageProxy.cpp:
> +        Added new messages to support IME on Windows.
> +        (WebKit::WebPageProxy::firstRectForCharacterRange):
> +        (WebKit::WebPageProxy::getSelectedText):
> +        (WebKit::WebPageProxy::didCompositionSelectionChange):
> +        (WebKit::WebPageProxy::confirmComposition):
> +        (WebKit::WebPageProxy::setComposition):
> +        * UIProcess/WebPageProxy.h:
> +        * UIProcess/WebPageProxy.messages.in:
> +        * UIProcess/win/WebView.cpp:
> +        Added WndProc messages to support IME.
> +        (WebKit::WebView::wndProc):
> +        (WebKit::WebView::WebView):
> +        (WebKit::IMMDict::dict):
> +        (WebKit::IMMDict::IMMDict):
> +        (WebKit::WebView::getIMMContext):
> +        (WebKit::WebView::releaseIMMContext):
> +        (WebKit::WebView::prepareCandidateWindow):
> +        (WebKit::WebView::resetIME):
> +        (WebKit::WebView::setInputMethodState):
> +        (WebKit::WebView::selectionChanged):
> +        (WebKit::WebView::compositionSelectionChanged):
> +        (WebKit::WebView::onIMEStartComposition):
> +        (WebKit::getCompositionString):
> +        (WebKit::compositionToUnderlines):
> +        (WebKit::imeCompositionArgumentNames):
> +        (WebKit::imeNotificationName):
> +        (WebKit::imeRequestName):
> +        Message handlers.
> +        (WebKit::WebView::onIMEComposition):
> +        (WebKit::WebView::onIMEEndComposition):
> +        (WebKit::WebView::onIMEChar):
> +        (WebKit::WebView::onIMENotify):
> +        (WebKit::WebView::onIMERequestCharPosition):
> +        (WebKit::WebView::onIMERequestReconvertString):
> +        (WebKit::WebView::onIMERequest):
> +        (WebKit::WebView::onIMESelect):
> +        (WebKit::WebView::onIMESetContext):
> +        * UIProcess/win/WebView.h:
> +        * WebProcess/WebCoreSupport/WebEditorClient.cpp:
> +        (WebKit::WebEditorClient::respondToChangedSelection):
> +        * WebProcess/WebPage/WebPage.cpp:
> +        (WebKit::WebPage::confirmComposition):
> +        (WebKit::WebPage::setComposition):
> +        (WebKit::WebPage::firstRectForCharacterRange):
> +        (WebKit::WebPage::getSelectedText):
> +        * WebProcess/WebPage/WebPage.h:
> +        * WebProcess/WebPage/WebPage.messages.in:

All this boilerplate without any comments isn't really helpful. It would be great to add comments, but if you aren't going to, you should probably remove all the function names. (Maybe the filenames, too.)

> WebKit2/UIProcess/WebPageProxy.cpp:505
> +WebCore::IntRect WebPageProxy::firstRectForCharacterRange(int characterPosition)

The name of this function seems wrong. You're not passing in a range, you're passing in a position.

> WebKit2/UIProcess/WebPageProxy.cpp:508
> +    process()->sendSync(Messages::WebPage::FirstRectForCharacterRange(characterPosition), Messages::WebPage::FirstRectForCharacterRange::Reply(resultRect), m_pageID);

Likewise, the name of this message seems wrong.

> WebKit2/UIProcess/WebPageProxy.cpp:1349
> +void WebPageProxy::didCompositionSelectionChange(bool hasComposition)

This function sounds like it's asking a question. I think compositionSelectionDidChange would be clearer.

> WebKit2/UIProcess/WebPageProxy.h:188
>  #else
>      void didSelectionChange(bool, bool, bool, bool);
> +    void didCompositionSelectionChange(bool);
> +    void confirmComposition(const String&);
> +    void setComposition(const String&, Vector<WebCore::CompositionUnderline>&, int);
> +    WebCore::IntRect firstRectForCharacterRange(int);
> +    String getSelectedText();
>  #endif

Do non-Windows platforms really want all these new functions?

didSelectionChange sounds like it's asking a question. I think selectionDidChange would be better. All those boolean parameters are very unreadable; enums might be better, but you should at least add some parameter names. "bool, bool, bool, bool" is basically meaningless.

> WebKit2/UIProcess/win/WebView.cpp:695
> +class IMMDict {
> +    typedef HIMC (CALLBACK *getContextPtr)(HWND);
> +    typedef BOOL (CALLBACK *releaseContextPtr)(HWND, HIMC);
> +    typedef LONG (CALLBACK *getCompositionStringPtr)(HIMC, DWORD, LPVOID, DWORD);
> +    typedef BOOL (CALLBACK *setCandidateWindowPtr)(HIMC, LPCANDIDATEFORM);
> +    typedef BOOL (CALLBACK *setOpenStatusPtr)(HIMC, BOOL);
> +    typedef BOOL (CALLBACK *notifyIMEPtr)(HIMC, DWORD, DWORD, DWORD);
> +    typedef BOOL (CALLBACK *associateContextExPtr)(HWND, HIMC, DWORD);
> +
> +public:
> +    getContextPtr getContext;
> +    releaseContextPtr releaseContext;
> +    getCompositionStringPtr getCompositionString;
> +    setCandidateWindowPtr setCandidateWindow;
> +    setOpenStatusPtr setOpenStatus;
> +    notifyIMEPtr notifyIME;
> +    associateContextExPtr associateContextEx;
> +
> +    static const IMMDict& dict();
> +private:
> +    IMMDict();
> +    HMODULE m_instance;
> +};
> +
> +const IMMDict& IMMDict::dict()
> +{
> +    static IMMDict instance;
> +    return instance;
> +}
> +
> +IMMDict::IMMDict()
> +{
> +    m_instance = ::LoadLibrary(TEXT("IMM32.DLL"));
> +    getContext = reinterpret_cast<getContextPtr>(::GetProcAddress(m_instance, "ImmGetContext"));
> +    ASSERT(getContext);
> +    releaseContext = reinterpret_cast<releaseContextPtr>(::GetProcAddress(m_instance, "ImmReleaseContext"));
> +    ASSERT(releaseContext);
> +    getCompositionString = reinterpret_cast<getCompositionStringPtr>(::GetProcAddress(m_instance, "ImmGetCompositionStringW"));
> +    ASSERT(getCompositionString);
> +    setCandidateWindow = reinterpret_cast<setCandidateWindowPtr>(::GetProcAddress(m_instance, "ImmSetCandidateWindow"));
> +    ASSERT(setCandidateWindow);
> +    setOpenStatus = reinterpret_cast<setOpenStatusPtr>(::GetProcAddress(m_instance, "ImmSetOpenStatus"));
> +    ASSERT(setOpenStatus);
> +    notifyIME = reinterpret_cast<notifyIMEPtr>(::GetProcAddress(m_instance, "ImmNotifyIME"));
> +    ASSERT(notifyIME);
> +    associateContextEx = reinterpret_cast<associateContextExPtr>(::GetProcAddress(m_instance, "ImmAssociateContextEx"));
> +    ASSERT(associateContextEx);
> +}

All of this could be replaced with the soft linking macros from WebCore/platform/win/SoftLinking.h. I think that would be a lot cleaner.

> WebKit2/UIProcess/win/WebView.cpp:717
> +    form.ptCurrentPos.y = caret.y() + caret.height();

I think caret.bottom() would work here.

> WebKit2/UIProcess/win/WebView.cpp:727
> +    if (HIMC hInputContext = getIMMContext()) {

An early return would be better here.

> WebKit2/UIProcess/win/WebView.cpp:769
> +    compositionLength = IMMDict::dict().getCompositionString(hInputContext, type, (LPVOID)compositionBuffer.data(), compositionLength);

static_cast<void*> would be better than (LPVOID). But I don't think you need a cast at all.

> WebKit2/UIProcess/win/WebView.cpp:770
> +    result = String(compositionBuffer.data(), compositionLength / 2);

result = String::adopt(compositionBuffer); would be better.

> WebKit2/UIProcess/win/WebView.cpp:783
> +    const size_t numBoundaries = clauses.size() - 1;

We don't normally use const for locals like this.

> WebKit2/UIProcess/win/WebView.cpp:790
> +        underlines[i].color = Color(0,0,0);

Maybe we could use Color::black here instead.

> WebKit2/UIProcess/win/WebView.cpp:805
> +    // ENRICA - OK

Huh?

> WebKit2/UIProcess/win/WebView.cpp:811
> +    if (lparam & GCS_COMPATTR) {
> +        result = "GCS_COMPATTR";
> +        needsComma = true;
> +    }

Why not use the macro for this case, too?

> WebKit2/UIProcess/win/WebView.cpp:904
> +    if (lparam & GCS_RESULTSTR || !lparam) {
> +        String compositionString;
> +        if (!getCompositionString(hInputContext, GCS_RESULTSTR, compositionString) && lparam)
> +            return true;
> +        
> +        m_page->confirmComposition(compositionString);
> +    } else {

You could add an early return at the end of the if block.

> WebKit2/UIProcess/win/WebView.cpp:916
> +        int numClauses = IMMDict::dict().getCompositionString(hInputContext, GCS_COMPCLAUSE, 0, 0);
> +        Vector<DWORD> clauses(numClauses / sizeof(DWORD));

numClauses seems clearly misnamed. It's a number of bytes, not a number of clauses. (The same is true of numAttributes, though it's a little less egregious in that case, since an attribute is apparently just a single byte.)

> WebKit2/UIProcess/win/WebView.cpp:957
> +bool WebView::onIMEChar(WPARAM wparam, LPARAM lparam)
> +{
> +    UNUSED_PARAM(wparam);
> +    UNUSED_PARAM(lparam);
> +    LOG(TextInput, "onIMEChar U+%04X %08X", wparam, lparam);
> +    return true;
> +}
> +
> +bool WebView::onIMENotify(WPARAM wparam, LPARAM, LRESULT*)
> +{
> +    UNUSED_PARAM(wparam);
> +    LOG(TextInput, "onIMENotify %s", imeNotificationName(wparam).latin1().data());
> +    return false;
> +}

Do we need to do anything more here? If so, FIXMEs and bugs would be appropriate.

> WebKit2/UIProcess/win/WebView.cpp:978
> +    if (!reconvertString)
> +        return sizeof(RECONVERTSTRING) + text.length() * sizeof(UChar);
> +
> +    unsigned totalSize = sizeof(RECONVERTSTRING) + text.length() * sizeof(UChar);

If we computed totalSize just a little earlier, we could use it in the early return.

> WebKit2/WebProcess/WebPage/WebPage.messages.in:108
> +#if !PLATFORM(MAC)
> +    ConfirmComposition(WTF::String compositionString)
> +    SetComposition(WTF::String compositionString, WTF::Vector<WebCore::CompositionUnderline> underlines, uint64_t cursorPosition)
> +    FirstRectForCharacterRange(uint64_t characterPosition) -> (WebCore::IntRect resultRect)
> +    GetSelectedText() -> (WTF::String text)
> +#endif

Do non-Windows platforms really want all these new messages?

[Enrica Casucci]

(In reply to comment #2)
> (From update of attachment 76553 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76553&action=review
> 
> > WebKit2/ChangeLog:59
 
> All this boilerplate without any comments isn't really helpful. It would be great to add comments, but if you aren't going to, you should probably remove all the function names. (Maybe the filenames, too.)
I'll add some more comments.
> 
> > WebKit2/UIProcess/WebPageProxy.cpp:505
> > +WebCore::IntRect WebPageProxy::firstRectForCharacterRange(int characterPosition)
> 
> The name of this function seems wrong. You're not passing in a range, you're passing in a position.
> 
Actually, the rect is calculated starting from the selected range and the character is an offset in that range. I could change it to firstRectForSelectedRange?

> This function sounds like it's asking a question. I think compositionSelectionDidChange would be clearer.
Ok.
> 
> > WebKit2/UIProcess/WebPageProxy.h:188
> >  #else
> >      void didSelectionChange(bool, bool, bool, bool);
> > +    void didCompositionSelectionChange(bool);
> > +    void confirmComposition(const String&);
> > +    void setComposition(const String&, Vector<WebCore::CompositionUnderline>&, int);
> > +    WebCore::IntRect firstRectForCharacterRange(int);
> > +    String getSelectedText();
> >  #endif
> 
> Do non-Windows platforms really want all these new functions?
I wasn't sure at all. I can definitely limit it to Windows, even though there is no platform specific code in there.
> 

All your other comments are relative to code that exists in the same form in WebKit today.
I thought that code has been reviewed and r+ before and it was safer not to modify it to avoid potential bugs. I agree that the code needs to be modernized, from a style standpoint.
I'll address the comments and post a new patch. Thanks.

[Adam Roben (:aroben)]

Comment on attachment 76553 [details]
Patch

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

>>> WebKit2/UIProcess/WebPageProxy.cpp:505
>>> +WebCore::IntRect WebPageProxy::firstRectForCharacterRange(int characterPosition)
>> 
>> The name of this function seems wrong. You're not passing in a range, you're passing in a position.
> 
> Actually, the rect is calculated starting from the selected range and the character is an offset in that range. I could change it to firstRectForSelectedRange?

I think the "For" part of the function name (even your new proposed name) is confusing me. Normally, "fooForBar" is a function that takes "bar" and returns "foo". In this case, we're taking something other than "bar". I don't understand what the function does enough to be confident of what a good name would be, but perhaps firstRectForCharacterInSelectedRange would work.

>>> WebKit2/UIProcess/WebPageProxy.h:188
>>>  #endif
>> 
>> Do non-Windows platforms really want all these new functions?
>> 
>> didSelectionChange sounds like it's asking a question. I think selectionDidChange would be better. All those boolean parameters are very unreadable; enums might be better, but you should at least add some parameter names. "bool, bool, bool, bool" is basically meaningless.
> 
> I wasn't sure at all. I can definitely limit it to Windows, even though there is no platform specific code in there.

It seems worth limiting it to Windows unless we're fairly confident that other ports will want to use these functions. If we limit it and end up being wrong, other ports can always un-limit it.

[Enrica Casucci]

Created attachment 76600 [details]
Patch2

The new patch addresses all the comments from Adam.

[Adam Roben (:aroben)]

Comment on attachment 76600 [details]
Patch2

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

> WebKit2/ChangeLog:20
> +        The followinf methods send asynchronous messages to the WebProcess.

Typo: followinf

> WebKit2/UIProcess/PageClient.h:75
>      virtual void selectionChanged(bool, bool, bool, bool) = 0;

I still think it would be good to give names to these parameters. You could do that in a separate patch.

> WebKit2/UIProcess/WebPageProxy.cpp:1343
> -void WebPageProxy::didSelectionChange(bool isNone, bool isContentEditable, bool isPasswordField, bool hasComposition)
> +void WebPageProxy::didChangeSelection(bool isNone, bool isContentEditable, bool isPasswordField, bool hasComposition)

I thought you were going to do this in a separate patch?

> WebKit2/UIProcess/WebPageProxy.h:182
> -    void didSelectionChange(bool, bool, bool, bool);
> +    void didChangeSelection(bool, bool, bool, bool);

Same comment here about parameter names.

> WebKit2/UIProcess/win/WebView.cpp:654
> +HIMC WebView::getIMMContext() 
> +{
> +    return WebKit::ImmGetContext(m_window);

No need for the "WebKit::" here. But I think it would be better to put the soft-linking macros outside of the WebKit namespace so that you can say "::ImmGetContext" like we do for other Win32 API calls.

This function doesn't seem necessary.

> WebKit2/UIProcess/win/WebView.cpp:662
> +void WebView::releaseIMMContext(HIMC hIMC)
> +{
> +    if (!hIMC)
> +        return;
> +    WebKit::ImmReleaseContext(m_window, hIMC);
> +}

This function also doesn't seem necessary, unless the null-checking is helping us. But maybe callers should just null-check. (In fact, I think they already do!)

> WebKit2/UIProcess/win/WebView.cpp:714
> +    HIMC hInputContext = getIMMContext();
> +    prepareCandidateWindow(hInputContext);
> +    releaseIMMContext(hInputContext);

Do we need a null-check here (if you get rid of releaseIMMContext)?

> WebKit2/UIProcess/win/WebView.cpp:738
> +    for (unsigned i = 0; i < numBoundaries; i++) {

You should use size_t here. We tend to prefer prefix (++i) instead of postfix, even when it doesn't matter, as in this case.

> WebKit2/UIProcess/win/WebView.cpp:903
> +            return onIMERequestReconvertString((RECONVERTSTRING*)data);
> +
> +        case IMR_QUERYCHARPOSITION:
> +            return onIMERequestCharPosition((IMECHARPOSITION*)data);

reinterpret_cast would be better here.

> WebKit2/UIProcess/win/WebView.h:145
> +    // Text input state values
> +    bool m_selectionIsNone;
> +    bool m_selectionIsEditable;
> +    bool m_selectionInPasswordField;
> +    bool m_hasMarkedText;
> +    unsigned m_inIMEComposition;

I'd putt these after the other data members, not before. Data members like "m_window" seem far more essential than these, and I think it's nice to have data members in rough order of decreasing importance.

> WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp:198
> +#if PLATFORM(WIN)
> +    if (!frame->editor()->hasComposition() || frame->editor()->ignoreCompositionSelectionChange())
> +        return;
> +
> +    unsigned start;
> +    unsigned end;
> +    m_page->send(Messages::WebPageProxy::DidChangeCompositionSelection(frame->editor()->getCompositionSelection(start, end)));
> +#endif

Maybe you should add a new platformRespondToChangedSelection function to hold this code?

> WebKit2/WebProcess/WebPage/WebPage.cpp:1057
> +#if PLATFORM(WIN)
> +void WebPage::confirmComposition(const String& compositionString)
> +{
> +    Frame* frame = m_page->focusController()->focusedOrMainFrame();
> +    if (!frame || !frame->editor()->canEdit())
> +        return;
> +    frame->editor()->confirmComposition(compositionString);
> +}
> +
> +void WebPage::setComposition(const String& compositionString, const Vector<WebCore::CompositionUnderline>& underlines, uint64_t cursorPosition)
> +{
> +    Frame* frame = m_page->focusController()->focusedOrMainFrame();
> +    if (!frame || !frame->editor()->canEdit())
> +        return;
> +    frame->editor()->setComposition(compositionString, underlines, cursorPosition, 0);
> +}
> +
> +void WebPage::firstRectForCharacterInSelectedRange(const uint64_t characterPosition, WebCore::IntRect& resultRect)
> +{
> +    Frame* frame = m_page->focusController()->focusedOrMainFrame();
> +    IntRect rect;
> +    if (RefPtr<Range> range = frame->editor()->hasComposition() ? frame->editor()->compositionRange() : frame->selection()->selection().toNormalizedRange()) {
> +        ExceptionCode ec = 0;
> +        RefPtr<Range> tempRange = range->cloneRange(ec);
> +        tempRange->setStart(tempRange->startContainer(ec), tempRange->startOffset(ec) + characterPosition, ec);
> +        rect = frame->editor()->firstRectForRange(tempRange.get());
> +    }
> +    resultRect = frame->view()->contentsToWindow(rect);
> +}
> +
> +void WebPage::getSelectedText(String& text)
> +{
> +    Frame* frame = m_page->focusController()->focusedOrMainFrame();
> +    RefPtr<Range> selectedRange = frame->selection()->toNormalizedRange();
> +    text = selectedRange->text();
> +}
> +#endif

This should all go in WebPageWin.cpp.

> WebKit2/WebProcess/WebPage/WebPage.messages.in:98
> +	// This is a dummy message to avoid breaking the build for platforms that don't require
> +	// synchronous messages.
> +	Dummy() -> (bool dummyReturn)

This seems like a bug in our scripts. Ideally this wouldn't be necessary. Can you file a bug about it and reference the bug number here?

[Enrica Casucci]

(In reply to comment #6)
> Typo: followinf
ok.
> 
> > WebKit2/UIProcess/PageClient.h:75
> >      virtual void selectionChanged(bool, bool, bool, bool) = 0;
> 
> I still think it would be good to give names to these parameters. You could do that in a separate patch.
Just followed the style used in this class. I will not change this now.
> 
> > WebKit2/UIProcess/WebPageProxy.cpp:1343
> > -void WebPageProxy::didSelectionChange(bool isNone, bool isContentEditable, bool isPasswordField, bool hasComposition)
> > +void WebPageProxy::didChangeSelection(bool isNone, bool isContentEditable, bool isPasswordField, bool hasComposition)
> 
> I thought you were going to do this in a separate patch?
I was going to do in a separate patch, the Mac version. This is for all the other platforms.
> 
> > WebKit2/UIProcess/WebPageProxy.h:182
> > -    void didSelectionChange(bool, bool, bool, bool);
> > +    void didChangeSelection(bool, bool, bool, bool);
> 
> Same comment here about parameter names.
Again, I followed the existing style. I agree that using parameter names is better, I just don't like mixing different styles.
> 
> > WebKit2/UIProcess/win/WebView.cpp:654
> > +HIMC WebView::getIMMContext() 
> > +{
> > +    return WebKit::ImmGetContext(m_window);
> 
> No need for the "WebKit::" here. But I think it would be better to put the soft-linking macros outside of the WebKit namespace so that you can say "::ImmGetContext" like we do for other Win32 API calls.
> 
> This function doesn't seem necessary.
> 
> > WebKit2/UIProcess/win/WebView.cpp:662
> > +void WebView::releaseIMMContext(HIMC hIMC)
> > +{
> > +    if (!hIMC)
> > +        return;
> > +    WebKit::ImmReleaseContext(m_window, hIMC);
> > +}
> 
> This function also doesn't seem necessary, unless the null-checking is helping us. But maybe callers should just null-check. (In fact, I think they already do!)
> 
> > WebKit2/UIProcess/win/WebView.cpp:714
> > +    HIMC hInputContext = getIMMContext();
> > +    prepareCandidateWindow(hInputContext);
> > +    releaseIMMContext(hInputContext);
> 
> Do we need a null-check here (if you get rid of releaseIMMContext)?
> 
> > WebKit2/UIProcess/win/WebView.cpp:738
> > +    for (unsigned i = 0; i < numBoundaries; i++) {
> 
> You should use size_t here. We tend to prefer prefix (++i) instead of postfix, even when it doesn't matter, as in this case.
> 
Will do.
> > WebKit2/UIProcess/win/WebView.cpp:903
> > +            return onIMERequestReconvertString((RECONVERTSTRING*)data);
> > +
> > +        case IMR_QUERYCHARPOSITION:
> > +            return onIMERequestCharPosition((IMECHARPOSITION*)data);
> 
> reinterpret_cast would be better here.
>
Ok. 
> > WebKit2/UIProcess/win/WebView.h:145
> > +    // Text input state values
> > +    bool m_selectionIsNone;
> > +    bool m_selectionIsEditable;
> > +    bool m_selectionInPasswordField;
> > +    bool m_hasMarkedText;
> > +    unsigned m_inIMEComposition;
> 
> I'd putt these after the other data members, not before. Data members like "m_window" seem far more essential than these, and I think it's nice to have data members in rough order of decreasing importance.
> 
ok.
> > WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp:198
> > +#if PLATFORM(WIN)
> > +    if (!frame->editor()->hasComposition() || frame->editor()->ignoreCompositionSelectionChange())
> > +        return;
> > +
> > +    unsigned start;
> > +    unsigned end;
> > +    m_page->send(Messages::WebPageProxy::DidChangeCompositionSelection(frame->editor()->getCompositionSelection(start, end)));
> > +#endif
> 
> Maybe you should add a new platformRespondToChangedSelection function to hold this code?
> 
Sure...
> > WebKit2/WebProcess/WebPage/WebPage.cpp:1057
> > +#if PLATFORM(WIN)
> > +void WebPage::confirmComposition(const String& compositionString)
> > +{
> > +    Frame* frame = m_page->focusController()->focusedOrMainFrame();
> > +    if (!frame || !frame->editor()->canEdit())
> > +        return;
> > +    frame->editor()->confirmComposition(compositionString);
> > +}
> > +
> > +void WebPage::setComposition(const String& compositionString, const Vector<WebCore::CompositionUnderline>& underlines, uint64_t cursorPosition)
> > +{
> > +    Frame* frame = m_page->focusController()->focusedOrMainFrame();
> > +    if (!frame || !frame->editor()->canEdit())
> > +        return;
> > +    frame->editor()->setComposition(compositionString, underlines, cursorPosition, 0);
> > +}
> > +
> > +void WebPage::firstRectForCharacterInSelectedRange(const uint64_t characterPosition, WebCore::IntRect& resultRect)
> > +{
> > +    Frame* frame = m_page->focusController()->focusedOrMainFrame();
> > +    IntRect rect;
> > +    if (RefPtr<Range> range = frame->editor()->hasComposition() ? frame->editor()->compositionRange() : frame->selection()->selection().toNormalizedRange()) {
> > +        ExceptionCode ec = 0;
> > +        RefPtr<Range> tempRange = range->cloneRange(ec);
> > +        tempRange->setStart(tempRange->startContainer(ec), tempRange->startOffset(ec) + characterPosition, ec);
> > +        rect = frame->editor()->firstRectForRange(tempRange.get());
> > +    }
> > +    resultRect = frame->view()->contentsToWindow(rect);
> > +}
> > +
> > +void WebPage::getSelectedText(String& text)
> > +{
> > +    Frame* frame = m_page->focusController()->focusedOrMainFrame();
> > +    RefPtr<Range> selectedRange = frame->selection()->toNormalizedRange();
> > +    text = selectedRange->text();
> > +}
> > +#endif
> 
> This should all go in WebPageWin.cpp.
> 
Ok.
> > WebKit2/WebProcess/WebPage/WebPage.messages.in:98
> > +	// This is a dummy message to avoid breaking the build for platforms that don't require
> > +	// synchronous messages.
> > +	Dummy() -> (bool dummyReturn)
> 
> This seems like a bug in our scripts. Ideally this wouldn't be necessary. Can you file a bug about it and reference the bug number here?
ok

[Adam Roben (:aroben)]

(In reply to comment #7)
> (In reply to comment #6)
> > Typo: followinf
> ok.
> > 
> > > WebKit2/UIProcess/PageClient.h:75
> > >      virtual void selectionChanged(bool, bool, bool, bool) = 0;
> > 
> > I still think it would be good to give names to these parameters. You could do that in a separate patch.
> Just followed the style used in this class. I will not change this now.

PageClient.h has parameter names for some of its functions (e.g., setFindIndicator has a "bool fadeOut" parameter). I'd argue that we should be adding more parameter names to that file, not less.

> > > WebKit2/UIProcess/WebPageProxy.h:182
> > > -    void didSelectionChange(bool, bool, bool, bool);
> > > +    void didChangeSelection(bool, bool, bool, bool);
> > 
> > Same comment here about parameter names.
> Again, I followed the existing style. I agree that using parameter names is better, I just don't like mixing different styles.

WebPageProxy.h also has plenty of functions with parameter names.

WebKit's defacto style (though it isn't written down anywhere) is to always use parameter names except where they don't add any value (e.g., "setString(const String& string)" is just as understandable without the "string"). We should be following that style in new code. Multiple boolean paramters in a row are especially opaque without parameter names.