Fix candidate window position issue.

Fix candidate window position issue.

This CL contains:
1. Extend SetCursorLocation message to propagate composition head character rectangle.
2. Do not send SetCursorLocation dbus message, which is not require any more.
3. Show candidate window at the composition head location when the candidate window category is SUGGESTION.


BUG=120597
TEST=unit_tests and manually check by alex


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=142103

[Yusuke Sato, 2012-06-13 14:31:26]

style-nits only.

http://codereview.chromium.org/10534134/diff/4002/chrome/browser/chromeos/inp...
File chrome/browser/chromeos/input_method/candidate_window.cc (right):

http://codereview.chromium.org/10534134/diff/4002/chrome/browser/chromeos/inp...
chrome/browser/chromeos/input_method/candidate_window.cc:537: virtual void
OnSetCursorLocation(int x, int y, int width, int height,
any reason not to use gfx::Rect instead of the first four arguments?

http://codereview.chromium.org/10534134/diff/4002/chrome/browser/chromeos/inp...
File chrome/browser/chromeos/input_method/candidate_window_view_unittest.cc
(right):

http://codereview.chromium.org/10534134/diff/4002/chrome/browser/chromeos/inp...
chrome/browser/chromeos/input_method/candidate_window_view_unittest.cc:39:
std::string value) {
const reference

http://codereview.chromium.org/10534134/diff/4002/chrome/browser/chromeos/inp...
File chrome/browser/chromeos/input_method/ibus_ui_controller.cc (left):

http://codereview.chromium.org/10534134/diff/4002/chrome/browser/chromeos/inp...
chrome/browser/chromeos/input_method/ibus_ui_controller.cc:302: // The list of
input method IDs for Mozc Japanese IMEs.
super-nice :)
I'm very happy to see the hack will be gone soon.

http://codereview.chromium.org/10534134/diff/4002/chrome/browser/chromeos/inp...
File chrome/browser/chromeos/input_method/ibus_ui_controller.cc (right):

http://codereview.chromium.org/10534134/diff/4002/chrome/browser/chromeos/inp...
chrome/browser/chromeos/input_method/ibus_ui_controller.cc:298: int32 x,
line 298-301: const gfx::Rect&

http://codereview.chromium.org/10534134/diff/4002/chrome/browser/chromeos/inp...
File chrome/browser/chromeos/input_method/ibus_ui_controller.h (right):

http://codereview.chromium.org/10534134/diff/4002/chrome/browser/chromeos/inp...
chrome/browser/chromeos/input_method/ibus_ui_controller.h:19: #include
"ui/gfx/rect.h"
replace it with a forward decl.

http://codereview.chromium.org/10534134/diff/4002/ui/base/ime/ibus_client.h
File ui/base/ime/ibus_client.h (right):

http://codereview.chromium.org/10534134/diff/4002/ui/base/ime/ibus_client.h#n...
ui/base/ime/ibus_client.h:16: #include "ui/gfx/rect.h"
forward decl.

http://codereview.chromium.org/10534134/diff/4002/ui/base/ime/ibus_client.h#n...
ui/base/ime/ibus_client.h:101: int32 x,
line 101-104: const gfx::Rect&

http://codereview.chromium.org/10534134/diff/4002/ui/base/ime/input_method_ib...
File ui/base/ime/input_method_ibus.cc (right):

http://codereview.chromium.org/10534134/diff/4002/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.cc:338:
GetTextInputClient()->GetCompositionCharacterBounds(0, &composition_head);
I guess when it returns false, composition_head is untouched, correct? If this
is correct, is the behavior well-documented?

[Seigo Nonaka, 2012-06-13 15:07:17]

http://codereview.chromium.org/10534134/diff/4002/chrome/browser/chromeos/inp...
File chrome/browser/chromeos/input_method/candidate_window.cc (right):

http://codereview.chromium.org/10534134/diff/4002/chrome/browser/chromeos/inp...
chrome/browser/chromeos/input_method/candidate_window.cc:537: virtual void
OnSetCursorLocation(int x, int y, int width, int height,
Okay remove now.
On 2012/06/13 14:31:26, Yusuke Sato wrote:
> any reason not to use gfx::Rect instead of the first four arguments?

http://codereview.chromium.org/10534134/diff/4002/chrome/browser/chromeos/inp...
File chrome/browser/chromeos/input_method/candidate_window_view_unittest.cc
(right):

http://codereview.chromium.org/10534134/diff/4002/chrome/browser/chromeos/inp...
chrome/browser/chromeos/input_method/candidate_window_view_unittest.cc:39:
std::string value) {
On 2012/06/13 14:31:26, Yusuke Sato wrote:
> const reference

Done.

http://codereview.chromium.org/10534134/diff/4002/chrome/browser/chromeos/inp...
File chrome/browser/chromeos/input_method/ibus_ui_controller.cc (right):

http://codereview.chromium.org/10534134/diff/4002/chrome/browser/chromeos/inp...
chrome/browser/chromeos/input_method/ibus_ui_controller.cc:298: int32 x,
On 2012/06/13 14:31:26, Yusuke Sato wrote:
> line 298-301: const gfx::Rect&

Done.

http://codereview.chromium.org/10534134/diff/4002/chrome/browser/chromeos/inp...
File chrome/browser/chromeos/input_method/ibus_ui_controller.h (right):

http://codereview.chromium.org/10534134/diff/4002/chrome/browser/chromeos/inp...
chrome/browser/chromeos/input_method/ibus_ui_controller.h:19: #include
"ui/gfx/rect.h"
On 2012/06/13 14:31:26, Yusuke Sato wrote:
> replace it with a forward decl.

Done.

http://codereview.chromium.org/10534134/diff/4002/ui/base/ime/ibus_client.h
File ui/base/ime/ibus_client.h (right):

http://codereview.chromium.org/10534134/diff/4002/ui/base/ime/ibus_client.h#n...
ui/base/ime/ibus_client.h:16: #include "ui/gfx/rect.h"
On 2012/06/13 14:31:26, Yusuke Sato wrote:
> forward decl.

Done.

http://codereview.chromium.org/10534134/diff/4002/ui/base/ime/ibus_client.h#n...
ui/base/ime/ibus_client.h:101: int32 x,
On 2012/06/13 14:31:26, Yusuke Sato wrote:
> line 101-104: const gfx::Rect&

Done.

http://codereview.chromium.org/10534134/diff/4002/ui/base/ime/input_method_ib...
File ui/base/ime/input_method_ibus.cc (right):

http://codereview.chromium.org/10534134/diff/4002/ui/base/ime/input_method_ib...
ui/base/ime/input_method_ibus.cc:338:
GetTextInputClient()->GetCompositionCharacterBounds(0, &composition_head);
It is correct but I didn't documented well about the behavior. I add comment for
TextInputClient. Also I add clear code just in case.
On 2012/06/13 14:31:26, Yusuke Sato wrote:
> I guess when it returns false, composition_head is untouched, correct? If this
> is correct, is the behavior well-documented?

[Yusuke Sato, 2012-06-13 15:17:38]

ui/base/ime/ LGTM.

Please ask horo@ for chrome/browser/chromeos/input_method/ review.

http://codereview.chromium.org/10534134/diff/10003/ui/base/ime/input_method_i...
File ui/base/ime/input_method_ibus.cc (right):

http://codereview.chromium.org/10534134/diff/10003/ui/base/ime/input_method_i...
ui/base/ime/input_method_ibus.cc:339: &composition_head))
{} is needed for a multi-line if.

[Seigo Nonaka, 2012-06-14 01:04:57]

yusukes: Thanks.

+horo:
Hi horo, could you take a look this CL mainly for
chrome/browser/chromeos/input_method/?
Thanks.

http://codereview.chromium.org/10534134/diff/10003/ui/base/ime/input_method_i...
File ui/base/ime/input_method_ibus.cc (right):

http://codereview.chromium.org/10534134/diff/10003/ui/base/ime/input_method_i...
ui/base/ime/input_method_ibus.cc:339: &composition_head))
On 2012/06/13 15:17:38, Yusuke Sato wrote:
> {} is needed for a multi-line if.

Done.

[horo, 2012-06-14 03:08:24]

lgtm

[Seigo Nonaka, 2012-06-14 03:10:43]

On 2012/06/14 03:08:24, horo wrote:
> lgtm

Thank you for your quick review!

[commit-bot: I haz the power, 2012-06-14 03:21:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nona@chromium.org/10534134/8003

[commit-bot: I haz the power, 2012-06-14 05:46:39]

Change committed as 142103