[webkit-reviews] review denied: [Bug 51049] IME support in WebKit2 on Windows : [Attachment 76553] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Dec 14 12:44:10 PST 2010
Adam Roben (aroben) <aroben at apple.com> has denied 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 76553: Patch
https://bugs.webkit.org/attachment.cgi?id=76553&action=review
------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
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(characterPosi
tion), 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?
More information about the webkit-reviews
mailing list