Bug 87911 – [chromium] There is no way to retrieve composition character rectangle in WebKit/chromium

• Component: Text
• Status: RESOLVED WONTFIX

[Seigo Nonaka]

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

[Seigo Nonaka]

Created attachment 144959 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Seigo Nonaka]

Created attachment 144961 [details]
Fixed compile error

[Seigo Nonaka]

Created attachment 144974 [details]
Refine

[Kentaro Hara]

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.

[Seigo Nonaka]

Created attachment 145566 [details]
WIP

[Seigo Nonaka]

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.

[Adam Barth]

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.

[Kentaro Hara]

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

[Seigo Nonaka]

Created attachment 145786 [details]
Add tests

[Ryosuke Niwa]

Comment on attachment 145786 [details]
Add tests

Looks reasonable.

[Seigo Nonaka]

Comment on attachment 145786 [details]
Add tests

Thank you for quick review!

[WebKit Review Bot]

Comment on attachment 145786 [details]
Add tests

Clearing flags on attachment: 145786

Committed r119494: <http://trac.webkit.org/changeset/119494>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

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.

[Seigo Nonaka]

Sure, I will fix as soon as possible.
Thanks.

[Hironori Bono]

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?

[Kentaro Hara]

Reverted r119494 for reason:

We found similar APIs are already implemented

Committed r119553: <http://trac.webkit.org/changeset/119553>