Created attachment 144954 [details] Comparison of candidate window position Some IME(input method editor, text inputting tool for CJK people) shows candidate window to select desired words. There are two types of candidate window in Japanese, one is shown just under cursor position and the other is shown aligning with composition text. In the former case, Chrome already support by using WebKit::WebWidget::selectionBounds API. However in the latter case, Chrome cannot support because WebKit does not support composition character boundary rectangles retrieval. In the case of Windows, this is corresponding to IMR_QUERYCHARPOSITION(ref 1) and we need to add API to retrieve composition character rectangles for supporting this message. This feature is not only Windows but also MacOSX(ref 2) and Chrome OS(ref 3). In addition, this feature is already implemented in WebKit/win as WebView::onIMERequestCharPosition, so Windows safari already support this feature(please see attached screen shot). Please let me add new API into WebKit/chromium. Reference: 1. http://msdn.microsoft.com/en-us/library/windows/desktop/dd318634(v=vs.85).aspx 2. https://developer.apple.com/library/mac/#documentation/Cocoa/Reference/NSTextInputClient_Protocol/Reference/Reference.html 3. http://code.google.com/p/chromium/issues/detail?id=120597
Created attachment 144959 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Created attachment 144961 [details] Fixed compile error
Created attachment 144974 [details] Refine
Comment on attachment 144974 [details] Refine View in context: https://bugs.webkit.org/attachment.cgi?id=144974&action=review Just nit picking comments > Source/WebKit/chromium/ChangeLog:12 > + Introduce new API to retrieve composition character boundary rectangles. > + This API is necessary for Japanese IME to show IME related window on > + the right position. In the case of Windows, this message is used to > + implement IMR_QUERYCHARPOSITION. > + I do not understand what this change is, but you might want to add a LayoutTest if possible. > Source/WebKit/chromium/public/WebWidget.h:188 > + virtual bool compositionCharacterBounds(size_t index, WebRect& rect) const { return false; } WebKit does not write obvious argument names. This can be: virtual bool compositionCharacterBounds(size_t index, WebRect&) > Source/WebKit/chromium/src/WebViewImpl.cpp:2012 > + return false; 4 spaces indentation please. > Source/WebKit/chromium/src/WebViewImpl.cpp:2018 > + return false; 4 spaces indentation please. > Source/WebKit/chromium/src/WebViewImpl.cpp:2023 > + tempRange->setStart(compositionRange->startContainer(ec), > + compositionRange->startOffset(ec) + index, > + ec); You do not need wrap the code by 80 characters. You can write like this: tempRange->setStart(compositionRange->startContainer(ec), compositionRange->startOffset(ec) + index, ec); > Source/WebKit/chromium/src/WebViewImpl.cpp:2026 > + tempRange->setEnd(compositionRange->startContainer(ec), > + compositionRange->startOffset(ec) + index + 1, > + ec); Ditto.
Created attachment 145566 [details] WIP
Comment on attachment 144974 [details] Refine View in context: https://bugs.webkit.org/attachment.cgi?id=144974&action=review >> Source/WebKit/chromium/ChangeLog:12 >> + > > I do not understand what this change is, but you might want to add a LayoutTest if possible. I revised the description. And I think LayoutTest is not for WebKit/chromium. I think webkit_unit_tests is for it, but I don't know how to... Anyway, I will add tests for above API, thanks. >> Source/WebKit/chromium/public/WebWidget.h:188 >> + virtual bool compositionCharacterBounds(size_t index, WebRect& rect) const { return false; } > > WebKit does not write obvious argument names. This can be: > > virtual bool compositionCharacterBounds(size_t index, WebRect&) Done, Thanks. >> Source/WebKit/chromium/src/WebViewImpl.cpp:2012 >> + return false; > > 4 spaces indentation please. Done, Thanks. >> Source/WebKit/chromium/src/WebViewImpl.cpp:2018 >> + return false; > > 4 spaces indentation please. Done, Thanks. >> Source/WebKit/chromium/src/WebViewImpl.cpp:2023 >> + ec); > > You do not need wrap the code by 80 characters. You can write like this: > > tempRange->setStart(compositionRange->startContainer(ec), compositionRange->startOffset(ec) + index, ec); Done, Thanks. >> Source/WebKit/chromium/src/WebViewImpl.cpp:2026 >> + ec); > > Ditto. Done, Thanks.
Comment on attachment 145566 [details] WIP This seems reasonable on the surface, but I don't feel like I know enough about editing. Maybe I'll say that the API change LGTM, but someone who understands editing should do the official review.
(In reply to comment #8) > (From update of attachment 145566 [details]) > This seems reasonable on the surface, but I don't feel like I know enough about editing. Maybe I'll say that the API change LGTM, but someone who understands editing should do the official review. rniwa: Are you familiar with this area? (or would you recommend another reviewer?)
Created attachment 145786 [details] Add tests
Comment on attachment 145786 [details] Add tests Looks reasonable.
Comment on attachment 145786 [details] Add tests Thank you for quick review!
Comment on attachment 145786 [details] Add tests Clearing flags on attachment: 145786 Committed r119494: <http://trac.webkit.org/changeset/119494>
All reviewed patches have been landed. Closing bug.
Comment on attachment 145786 [details] Add tests View in context: https://bugs.webkit.org/attachment.cgi?id=145786&action=review > Source/WebKit/chromium/src/WebViewImpl.cpp:2043 > + tempRange->setStart(compositionRange->startContainer(ec), compositionRange->startOffset(ec) + index, ec); > + tempRange->setEnd(compositionRange->startContainer(ec), compositionRange->startOffset(ec) + index + 1, ec); On the second thought, this code assumes that the entire composition text is in a single node, which may not be the case. You should probably take care of the case when they belong in different nodes. A better way way to do this might be using TextIterator.
Sure, I will fix as soon as possible. Thanks.
Comment on attachment 145786 [details] Add tests View in context: https://bugs.webkit.org/attachment.cgi?id=145786&action=review > Source/WebKit/chromium/src/WebViewImpl.cpp:2029 > +bool WebViewImpl::compositionCharacterBounds(size_t index, WebRect& rect) const I cannot figure out the clear differences (in terms of functionality) between this function and WebFrameImpl::firstRectForCharacterRange() <http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebKit/chromium/src/WebFrameImpl.cpp&exact_package=chromium&q=firstrectforcharacte&type=cs&l=1209>. Would it be possible to give me why you need a new function?
Reverted r119494 for reason: We found similar APIs are already implemented Committed r119553: <http://trac.webkit.org/changeset/119553>