Bug 87911 - [chromium] There is no way to retrieve composition character rectangle in WebKit/chromium
Summary: [chromium] There is no way to retrieve composition character rectangle in Web...
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-30 17:38 PDT by Seigo Nonaka
Modified: 2013-04-15 07:27 PDT (History)
10 users (show)

See Also:


Attachments
Comparison of candidate window position (45.97 KB, image/png)
2012-05-30 17:38 PDT, Seigo Nonaka
no flags Details
Patch (4.64 KB, patch)
2012-05-30 17:51 PDT, Seigo Nonaka
no flags Details | Formatted Diff | Diff
Fixed compile error (4.64 KB, patch)
2012-05-30 17:56 PDT, Seigo Nonaka
no flags Details | Formatted Diff | Diff
Refine (4.46 KB, patch)
2012-05-30 19:57 PDT, Seigo Nonaka
no flags Details | Formatted Diff | Diff
WIP (4.33 KB, patch)
2012-06-04 05:30 PDT, Seigo Nonaka
no flags Details | Formatted Diff | Diff
Add tests (5.99 KB, patch)
2012-06-05 07:25 PDT, Seigo Nonaka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Seigo Nonaka 2012-05-30 17:38:25 PDT
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
Comment 1 Seigo Nonaka 2012-05-30 17:51:35 PDT
Created attachment 144959 [details]
Patch
Comment 2 WebKit Review Bot 2012-05-30 17:54:13 PDT
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.
Comment 3 Seigo Nonaka 2012-05-30 17:56:06 PDT
Created attachment 144961 [details]
Fixed compile error
Comment 4 Seigo Nonaka 2012-05-30 19:57:25 PDT
Created attachment 144974 [details]
Refine
Comment 5 Kentaro Hara 2012-06-04 04:07:28 PDT
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.
Comment 6 Seigo Nonaka 2012-06-04 05:30:54 PDT
Created attachment 145566 [details]
WIP
Comment 7 Seigo Nonaka 2012-06-04 05:40:44 PDT
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 8 Adam Barth 2012-06-04 11:15:33 PDT
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.
Comment 9 Kentaro Hara 2012-06-04 22:06:03 PDT
(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?)
Comment 10 Seigo Nonaka 2012-06-05 07:25:59 PDT
Created attachment 145786 [details]
Add tests
Comment 11 Ryosuke Niwa 2012-06-05 07:31:49 PDT
Comment on attachment 145786 [details]
Add tests

Looks reasonable.
Comment 12 Seigo Nonaka 2012-06-05 07:36:26 PDT
Comment on attachment 145786 [details]
Add tests

Thank you for quick review!
Comment 13 WebKit Review Bot 2012-06-05 09:05:06 PDT
Comment on attachment 145786 [details]
Add tests

Clearing flags on attachment: 145786

Committed r119494: <http://trac.webkit.org/changeset/119494>
Comment 14 WebKit Review Bot 2012-06-05 09:05:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Ryosuke Niwa 2012-06-05 09:23:30 PDT
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.
Comment 16 Seigo Nonaka 2012-06-05 09:40:17 PDT
Sure, I will fix as soon as possible.
Thanks.
Comment 17 Hironori Bono 2012-06-05 16:34:36 PDT
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?
Comment 18 Kentaro Hara 2012-06-05 20:44:21 PDT
Reverted r119494 for reason:

We found similar APIs are already implemented

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