[webkit-reviews] review granted: [Bug 51049] IME support in WebKit2 on Windows : [Attachment 76600] Patch2

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted Enrica Casucci
<enrica at apple.com>'s request for review:
Bug 51049: IME support in WebKit2 on Windows
https://bugs.webkit.org/show_bug.cgi?id=51049

Attachment 76600: Patch2
https://bugs.webkit.org/attachment.cgi?id=76600&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
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->edito
r()->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?