Refactoring for InputMethodAuraLinux.

Refactoring for InputMethodAuraLinux.

This basically brings back the original implementation of InputMethodGtk long time ago, which covered most of cases that haven't been covered by the current implementation (e.g. generate keypress event for the accent/dead keys).

Due to limitations of Aura framework, it is hard to get the native window (e.g. XWindow). Therefore, gtk_im_context_set_cursor_position cannot be called at the appropriate time. The current InputMethodAuraLinux implementation workarounds it to leverage the key event, which carries the information about native window, and call set_client_window in DispatchKeyEvent(). The basic scenarios work but some special scenario cannot work. For example, iBus Pinyin IME tries to show the status-bar-UI at the cursor position without typing any keys.
This cl keep the current logics for that and won't fix the issue.

And this cl removes the keyup of 229 keycode, which is unnecessary. Therefore, for 229 keycode, the IMF will not try to pair the keydown and keyup.

This cl adds some tests to avoid future regressions on basic scenarios which consider sync/async modes, ibus hacky behaviors, dead keys, multi-commits, etc.
Note that the tests don't cover all possible scenarios as the worldwide IMEs with different IM modules (xim, ibus, etc.).

TBR=sky@chromium.org
BUG=463491, 474828, 125161, 458179
TEST=Verified on local linux workstation.

Committed: https://crrev.com/fad763a26eb994725004358ffdb622f62b744812
Cr-Commit-Position: refs/heads/master@{#325372}

[Shu Chen, 2015-04-08 00:23:00]

shuchen@chromium.org changed reviewers:
+ sky@chromium.org, suzhe@chromium.org, yukawa@chromium.org

[Shu Chen, 2015-04-08 00:23:01]

Guys, can you please review this cl? Thanks

[Seigo Nonaka, 2015-04-08 07:14:57]

nona@chromium.org changed reviewers:
+ nona@chromium.org

[Seigo Nonaka, 2015-04-08 07:14:58]

drive-by.

mostly nit-pick.
Thank you for your cleaning up!

https://codereview.chromium.org/1068093002/diff/40001/ui/base/ime/input_metho...
File ui/base/ime/input_method_auralinux.cc (right):

https://codereview.chromium.org/1068093002/diff/40001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux.cc:66: handling_key_event_ = true;
Please remove.
Optional: probably using a RAII object for handling_key_event_ might be less
error-prone.

like

class ScopedBooleanFlipper {
 public:
  ScopedBooleanFlipper(bool* v) : v_(v) { DCHECK(!*v_); *v_ = true; }
  ~ScopedBooleanFlipper() { *v_ = false; }
};

And

bool filtered = false;
{
  ScopedBooleanFlipper(&handling_key_event_);
  if (context_focused_)
    filtered = ...
  else
    filtered = ...
}

https://codereview.chromium.org/1068093002/diff/40001/ui/base/ime/input_metho...
File ui/base/ime/input_method_auralinux_unittest.cc (right):

https://codereview.chromium.org/1068093002/diff/40001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux_unittest.cc:249: TextInputClientForTesting*
client = new TextInputClientForTesting(
Optional: I recommend you to use scoped_ptr for this, instead of deleting
manually.

https://codereview.chromium.org/1068093002/diff/40001/ui/base/ime/linux/linux...
File ui/base/ime/linux/linux_input_method_context.h (right):

https://codereview.chromium.org/1068093002/diff/40001/ui/base/ime/linux/linux...
ui/base/ime/linux/linux_input_method_context.h:13: typedef unsigned long Window;
Who use this?

https://codereview.chromium.org/1068093002/diff/40001/ui/base/ime/linux/linux...
ui/base/ime/linux/linux_input_method_context.h:43: // Focus the context.
nit: Focuses

[Shu Chen, 2015-04-08 08:24:13]

Thanks for the review, nona!

https://codereview.chromium.org/1068093002/diff/40001/ui/base/ime/input_metho...
File ui/base/ime/input_method_auralinux.cc (right):

https://codereview.chromium.org/1068093002/diff/40001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux.cc:66: handling_key_event_ = true;
On 2015/04/08 07:14:58, Seigo Nonaka wrote:
> Please remove.
> Optional: probably using a RAII object for handling_key_event_ might be less
> error-prone.
> 
> like
> 
> class ScopedBooleanFlipper {
>  public:
>   ScopedBooleanFlipper(bool* v) : v_(v) { DCHECK(!*v_); *v_ = true; }
>   ~ScopedBooleanFlipper() { *v_ = false; }
> };
> 
> And
> 
> bool filtered = false;
> {
>   ScopedBooleanFlipper(&handling_key_event_);
>   if (context_focused_)
>     filtered = ...
>   else
>     filtered = ...
> }

Done.

https://codereview.chromium.org/1068093002/diff/40001/ui/base/ime/input_metho...
File ui/base/ime/input_method_auralinux_unittest.cc (right):

https://codereview.chromium.org/1068093002/diff/40001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux_unittest.cc:249: TextInputClientForTesting*
client = new TextInputClientForTesting(
On 2015/04/08 07:14:58, Seigo Nonaka wrote:
> Optional: I recommend you to use scoped_ptr for this, instead of deleting
> manually.

Done.

https://codereview.chromium.org/1068093002/diff/40001/ui/base/ime/linux/linux...
File ui/base/ime/linux/linux_input_method_context.h (right):

https://codereview.chromium.org/1068093002/diff/40001/ui/base/ime/linux/linux...
ui/base/ime/linux/linux_input_method_context.h:13: typedef unsigned long Window;
On 2015/04/08 07:14:58, Seigo Nonaka wrote:
> Who use this?

That is useless, removed.

https://codereview.chromium.org/1068093002/diff/40001/ui/base/ime/linux/linux...
ui/base/ime/linux/linux_input_method_context.h:43: // Focus the context.
On 2015/04/08 07:14:58, Seigo Nonaka wrote:
> nit: Focuses

Done.

[Seigo Nonaka, 2015-04-08 17:15:41]

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
File ui/base/ime/input_method_auralinux.cc (right):

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux.cc:117: case ui::TEXT_INPUT_TYPE_PASSWORD:
Probably off the topic but I just noticed the issue about GtkContextSimple for
password field.
https://code.google.com/p/chromium/issues/detail?id=125161

If you have any idea, please also consider to fix it.

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux.cc:163: DCHECK(!handling_key_event_);
ditto?

[yukawa, 2015-04-08 22:26:49]

Mostly looks good.  Can you (or someone) check if this change has
positive/no/negative impact on crbug.com/351991 or not?

https://codereview.chromium.org/1068093002/diff/60001/chrome/browser/ui/libgt...
File chrome/browser/ui/libgtk2ui/x11_input_method_context_impl_gtk2.h (right):

https://codereview.chromium.org/1068093002/diff/60001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtk2ui/x11_input_method_context_impl_gtk2.h:29: explicit
X11InputMethodContextImplGtk2(
nit: we can drop "explicit".

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
File ui/base/ime/input_method_auralinux.cc (right):

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux.cc:16: class ScopedBooleanFlipper {
Can we use base::AutoReset in //src/base/auto_reset.h ?

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux.cc:90: if (result_text_.length() ||
composition_changed_)
I slightly prefer string::empty(). How about this?
  if (!result_text_.empty() || composition_changed_)

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux.cc:117: case ui::TEXT_INPUT_TYPE_PASSWORD:
On 2015/04/08 17:15:40, Seigo Nonaka wrote:
> Probably off the topic but I just noticed the issue about GtkContextSimple for
> password field.
> https://code.google.com/p/chromium/issues/detail?id=125161
> 
> If you have any idea, please also consider to fix it.

Sounds good.  But I think it's OK to try it later so that we can keep the diff
in this CL as small as possible in case we need to do bisect.

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux.cc:213: if (!composing_text_ &&
!text.length())
I slightly prefer string::empty(). How about this?
  if (!composing_text_ && text.empty())

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux.cc:240: if (!composition_.text.length() &&
!composition_text.text.length())
I slightly prefer string::empty(). How about this?
  if (composition_.text.empty() && composition_text.text.empty())

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux.cc:245: if (composition_.text.length())
I slightly prefer string::empty(). How about this?
  if (!composition_.text.empty())

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux.cc:307: if (result_text_.length()) {
I slightly prefer string::empty(). How about this?
  if (!result_text_.empty()) {

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux.cc:309: for (base::string16::const_iterator i
= result_text_.begin();
How about using range-based for?
  for (const auto c : result_text_)
    client->InsertChar(c, key.flags());

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux.cc:320: if (composition_.text.length()) {
I slightly prefer string::empty(). How about this?
    if (!composition_.text.empty()) {

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
File ui/base/ime/input_method_auralinux_unittest.cc (right):

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux_unittest.cc:46: for (size_t i = 0; i <
actions_.size(); i++) {
  for (const auto& action : actions_) {

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux_unittest.cc:111:
DISALLOW_COPY_AND_ASSIGN(LinuxInputMethodContextFactoryForTesting);
Wrong indent?

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux_unittest.cc:116:
InputMethodDelegateForTesting()
Wrong indent offset?

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux_unittest.cc:144: :
DummyTextInputClient(text_input_type),
Wrong indent offset?

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux_unittest.cc:200: context_(NULL),
context_simple_(NULL) {
Wrong indent offset?

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux_unittest.cc:335: // iBus mutes the key down.
nit: I heard IBus is an official name.  Ditto for others.

[Shu Chen, 2015-04-09 01:16:33]

https://codereview.chromium.org/1068093002/diff/60001/chrome/browser/ui/libgt...
File chrome/browser/ui/libgtk2ui/x11_input_method_context_impl_gtk2.h (right):

https://codereview.chromium.org/1068093002/diff/60001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtk2ui/x11_input_method_context_impl_gtk2.h:29: explicit
X11InputMethodContextImplGtk2(
On 2015/04/08 22:26:48, yukawa wrote:
> nit: we can drop "explicit".

Done.

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
File ui/base/ime/input_method_auralinux.cc (right):

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux.cc:16: class ScopedBooleanFlipper {
On 2015/04/08 22:26:48, yukawa wrote:
> Can we use base::AutoReset in //src/base/auto_reset.h ?

Done.

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux.cc:90: if (result_text_.length() ||
composition_changed_)
On 2015/04/08 22:26:49, yukawa wrote:
> I slightly prefer string::empty(). How about this?
>   if (!result_text_.empty() || composition_changed_)

Done.

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux.cc:117: case ui::TEXT_INPUT_TYPE_PASSWORD:
On 2015/04/08 22:26:49, yukawa wrote:
> On 2015/04/08 17:15:40, Seigo Nonaka wrote:
> > Probably off the topic but I just noticed the issue about GtkContextSimple
for
> > password field.
> > https://code.google.com/p/chromium/issues/detail?id=125161
> > 
> > If you have any idea, please also consider to fix it.
> 
> Sounds good.  But I think it's OK to try it later so that we can keep the diff
> in this CL as small as possible in case we need to do bisect.

I would also prefer to consider it in a separated cl.

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux.cc:163: DCHECK(!handling_key_event_);
On 2015/04/08 17:15:40, Seigo Nonaka wrote:
> ditto?

Done.

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux.cc:213: if (!composing_text_ &&
!text.length())
On 2015/04/08 22:26:48, yukawa wrote:
> I slightly prefer string::empty(). How about this?
>   if (!composing_text_ && text.empty())

Done.

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux.cc:240: if (!composition_.text.length() &&
!composition_text.text.length())
On 2015/04/08 22:26:48, yukawa wrote:
> I slightly prefer string::empty(). How about this?
>   if (composition_.text.empty() && composition_text.text.empty())

Done.

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux.cc:245: if (composition_.text.length())
On 2015/04/08 22:26:48, yukawa wrote:
> I slightly prefer string::empty(). How about this?
>   if (!composition_.text.empty())

Done.

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux.cc:307: if (result_text_.length()) {
On 2015/04/08 22:26:48, yukawa wrote:
> I slightly prefer string::empty(). How about this?
>   if (!result_text_.empty()) {

Done.

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux.cc:309: for (base::string16::const_iterator i
= result_text_.begin();
On 2015/04/08 22:26:48, yukawa wrote:
> How about using range-based for?
>   for (const auto c : result_text_)
>     client->InsertChar(c, key.flags());

Done.

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux.cc:320: if (composition_.text.length()) {
On 2015/04/08 22:26:49, yukawa wrote:
> I slightly prefer string::empty(). How about this?
>     if (!composition_.text.empty()) {

Done.

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
File ui/base/ime/input_method_auralinux_unittest.cc (right):

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux_unittest.cc:46: for (size_t i = 0; i <
actions_.size(); i++) {
On 2015/04/08 22:26:49, yukawa wrote:
>   for (const auto& action : actions_) {

Done.

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux_unittest.cc:111:
DISALLOW_COPY_AND_ASSIGN(LinuxInputMethodContextFactoryForTesting);
On 2015/04/08 22:26:49, yukawa wrote:
> Wrong indent?

Done.

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux_unittest.cc:116:
InputMethodDelegateForTesting()
On 2015/04/08 22:26:49, yukawa wrote:
> Wrong indent offset?

Done.

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux_unittest.cc:144: :
DummyTextInputClient(text_input_type),
On 2015/04/08 22:26:49, yukawa wrote:
> Wrong indent offset?

Done. I've done git cl format.

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux_unittest.cc:200: context_(NULL),
context_simple_(NULL) {
On 2015/04/08 22:26:49, yukawa wrote:
> Wrong indent offset?

Done.

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux_unittest.cc:335: // iBus mutes the key down.
On 2015/04/08 22:26:49, yukawa wrote:
> nit: I heard IBus is an official name.  Ditto for others.

Done.

[yukawa, 2015-04-09 05:22:54]

yukawa@chromium.org changed reviewers:
+ yukishiino@chromium.org

[yukawa, 2015-04-09 05:22:55]

Thanks.  Looks good to me.

I have only limited knowledge/experience of GTK. I want to hear comments from
James, Seigo, and Yuki as for changes related to GTK.

[Yuki, 2015-04-09 05:58:52]

Does this CL produce 229-down when 'a' key is pressed even when an IME is not
activated?
My dirty hack was introduced to prevent such a behavior.

How do you stop producing VKEY_PROCESS in case of IBus + English US keyboard?

[Yuki, 2015-04-09 06:21:55]

On 2015/04/09 05:58:52, Yuki wrote:
> Does this CL produce 229-down when 'a' key is pressed even when an IME is not
> activated?
> My dirty hack was introduced to prevent such a behavior.
> 
> How do you stop producing VKEY_PROCESS in case of IBus + English US keyboard?

One more question.

With this CL, it seems we can enable an IME at password text field.  If so, it
would be controversial.  I think not everyone agrees with the change.  You may
want to have a separate discussion if it's really okay to allow to enable an IME
at password text field.

[Shu Chen, 2015-04-09 06:45:45]

On 2015/04/09 06:21:55, Yuki wrote:
> On 2015/04/09 05:58:52, Yuki wrote:
> > Does this CL produce 229-down when 'a' key is pressed even when an IME is
not
> > activated?
> > My dirty hack was introduced to prevent such a behavior.
> > 
> > How do you stop producing VKEY_PROCESS in case of IBus + English US
keyboard?
> 
> One more question.
> 
> With this CL, it seems we can enable an IME at password text field.  If so, it
> would be controversial.  I think not everyone agrees with the change.  You may
> want to have a separate discussion if it's really okay to allow to enable an
IME
> at password text field.

The cl doesn't try to workaround it. And I think we should not workaround it,
because IMF doesn't know the standalone OnCommit() must be triggered by the
previous keydown.
For example, assuming there is an ibus-based IME tries to simulate the dead key
behavior, e.g. `e -> è.
The current implementation will generate events: `(up), e(down), è(press), e(up)
With my cl, it will be: 229(down), `(up), 229(down), e(down), è(press), e(up)
So the `(up) has 229(down) to be a pair, which sounds more reasonable. WDYT?

As for the password concern, please notice that my cl won't enable IME on
password field, it only notify the IME about a password field is
focused/blurred, and it won't forward physical keyboard events to IMEs.
The same logic is used in ChromeOS IMF to make Virtual Keyboard can work in
password field.

[Shu Chen, 2015-04-09 07:02:42]

On 2015/04/09 06:45:45, Shu Chen wrote:
> On 2015/04/09 06:21:55, Yuki wrote:
> > On 2015/04/09 05:58:52, Yuki wrote:
> > > Does this CL produce 229-down when 'a' key is pressed even when an IME is
> not
> > > activated?
> > > My dirty hack was introduced to prevent such a behavior.
> > > 
> > > How do you stop producing VKEY_PROCESS in case of IBus + English US
> keyboard?
> > 
> > One more question.
> > 
> > With this CL, it seems we can enable an IME at password text field.  If so,
it
> > would be controversial.  I think not everyone agrees with the change.  You
may
> > want to have a separate discussion if it's really okay to allow to enable an
> IME
> > at password text field.
> 
> The cl doesn't try to workaround it. And I think we should not workaround it,
> because IMF doesn't know the standalone OnCommit() must be triggered by the
> previous keydown.
> For example, assuming there is an ibus-based IME tries to simulate the dead
key
> behavior, e.g. `e -> è.
> The current implementation will generate events: `(up), e(down), è(press),
e(up)
> With my cl, it will be: 229(down), `(up), 229(down), e(down), è(press), e(up)
> So the `(up) has 229(down) to be a pair, which sounds more reasonable. WDYT?
> 
> As for the password concern, please notice that my cl won't enable IME on
> password field, it only notify the IME about a password field is
> focused/blurred, and it won't forward physical keyboard events to IMEs.
> The same logic is used in ChromeOS IMF to make Virtual Keyboard can work in
> password field.

My previous dead key case was not a good case.
A better case is off-spot IME (like Google Pinyin IME on Windows), which doesn't
set composition in the input field, instead, it show the composition text in its
popup editor UI.
So, when user typing "ni" and then mouse click on the candidate "你", the new cl
generated events are: 229(down), n(up), 229(down), i(up), 229(down),
textInput(你)
Not sure how the current implement behaves for above case, can you please
analyze?

[Yuki, 2015-04-09 07:03:28]

On 2015/04/09 06:45:45, Shu Chen wrote:
> On 2015/04/09 06:21:55, Yuki wrote:
> > On 2015/04/09 05:58:52, Yuki wrote:
> > > Does this CL produce 229-down when 'a' key is pressed even when an IME is
> not
> > > activated?
> > > My dirty hack was introduced to prevent such a behavior.
> > > 
> > > How do you stop producing VKEY_PROCESS in case of IBus + English US
> keyboard?
> > 
> > One more question.
> > 
> > With this CL, it seems we can enable an IME at password text field.  If so,
it
> > would be controversial.  I think not everyone agrees with the change.  You
may
> > want to have a separate discussion if it's really okay to allow to enable an
> IME
> > at password text field.
> 
> The cl doesn't try to workaround it. And I think we should not workaround it,
> because IMF doesn't know the standalone OnCommit() must be triggered by the
> previous keydown.
> For example, assuming there is an ibus-based IME tries to simulate the dead
key
> behavior, e.g. `e -> è.
> The current implementation will generate events: `(up), e(down), è(press),
e(up)
> With my cl, it will be: 229(down), `(up), 229(down), e(down), è(press), e(up)
> So the `(up) has 229(down) to be a pair, which sounds more reasonable. WDYT?
> 
> As for the password concern, please notice that my cl won't enable IME on
> password field, it only notify the IME about a password field is
> focused/blurred, and it won't forward physical keyboard events to IMEs.
> The same logic is used in ChromeOS IMF to make Virtual Keyboard can work in
> password field.

The fix looks very good, but I'm very concerned to produce VKEY_PROCESS down for
a simple 'a' key down.  IBus is installed on many Debian/Ubuntu based system by
default, and even non-IME users (non intentional IME users) use it.  And not few
web pages do not expect VKEY_PROCESS events, especially game-like-interactive
web pages written in JS will be broken.

In short, I'm afraid that this change may cause site-compatibility issues, and
need to be very careful.

[Yuki, 2015-04-09 07:17:47]

On 2015/04/09 07:03:28, Yuki wrote:
> On 2015/04/09 06:45:45, Shu Chen wrote:
> > On 2015/04/09 06:21:55, Yuki wrote:
> > > On 2015/04/09 05:58:52, Yuki wrote:
> > > > Does this CL produce 229-down when 'a' key is pressed even when an IME
is
> > not
> > > > activated?
> > > > My dirty hack was introduced to prevent such a behavior.
> > > > 
> > > > How do you stop producing VKEY_PROCESS in case of IBus + English US
> > keyboard?
> > > 
> > > One more question.
> > > 
> > > With this CL, it seems we can enable an IME at password text field.  If
so,
> it
> > > would be controversial.  I think not everyone agrees with the change.  You
> may
> > > want to have a separate discussion if it's really okay to allow to enable
an
> > IME
> > > at password text field.
> > 
> > The cl doesn't try to workaround it. And I think we should not workaround
it,
> > because IMF doesn't know the standalone OnCommit() must be triggered by the
> > previous keydown.
> > For example, assuming there is an ibus-based IME tries to simulate the dead
> key
> > behavior, e.g. `e -> è.
> > The current implementation will generate events: `(up), e(down), è(press),
> e(up)
> > With my cl, it will be: 229(down), `(up), 229(down), e(down), è(press),
e(up)
> > So the `(up) has 229(down) to be a pair, which sounds more reasonable. WDYT?
> > 
> > As for the password concern, please notice that my cl won't enable IME on
> > password field, it only notify the IME about a password field is
> > focused/blurred, and it won't forward physical keyboard events to IMEs.
> > The same logic is used in ChromeOS IMF to make Virtual Keyboard can work in
> > password field.
> 
> The fix looks very good, but I'm very concerned to produce VKEY_PROCESS down
for
> a simple 'a' key down.  IBus is installed on many Debian/Ubuntu based system
by
> default, and even non-IME users (non intentional IME users) use it.  And not
few
> web pages do not expect VKEY_PROCESS events, especially game-like-interactive
> web pages written in JS will be broken.
> 
> In short, I'm afraid that this change may cause site-compatibility issues, and
> need to be very careful.

A little more: # of non-IME users (typically ASCII only) is greater than # of
IME users.  So, it's important to not break non-IME world (or, more important
than fixing IME world :(

[Shu Chen, 2015-04-09 07:22:38]

On 2015/04/09 07:17:47, Yuki wrote:
> On 2015/04/09 07:03:28, Yuki wrote:
> > On 2015/04/09 06:45:45, Shu Chen wrote:
> > > On 2015/04/09 06:21:55, Yuki wrote:
> > > > On 2015/04/09 05:58:52, Yuki wrote:
> > > > > Does this CL produce 229-down when 'a' key is pressed even when an IME
> is
> > > not
> > > > > activated?
> > > > > My dirty hack was introduced to prevent such a behavior.
> > > > > 
> > > > > How do you stop producing VKEY_PROCESS in case of IBus + English US
> > > keyboard?
> > > > 
> > > > One more question.
> > > > 
> > > > With this CL, it seems we can enable an IME at password text field.  If
> so,
> > it
> > > > would be controversial.  I think not everyone agrees with the change. 
You
> > may
> > > > want to have a separate discussion if it's really okay to allow to
enable
> an
> > > IME
> > > > at password text field.
> > > 
> > > The cl doesn't try to workaround it. And I think we should not workaround
> it,
> > > because IMF doesn't know the standalone OnCommit() must be triggered by
the
> > > previous keydown.
> > > For example, assuming there is an ibus-based IME tries to simulate the
dead
> > key
> > > behavior, e.g. `e -> è.
> > > The current implementation will generate events: `(up), e(down), è(press),
> > e(up)
> > > With my cl, it will be: 229(down), `(up), 229(down), e(down), è(press),
> e(up)
> > > So the `(up) has 229(down) to be a pair, which sounds more reasonable.
WDYT?
> > > 
> > > As for the password concern, please notice that my cl won't enable IME on
> > > password field, it only notify the IME about a password field is
> > > focused/blurred, and it won't forward physical keyboard events to IMEs.
> > > The same logic is used in ChromeOS IMF to make Virtual Keyboard can work
in
> > > password field.
> > 
> > The fix looks very good, but I'm very concerned to produce VKEY_PROCESS down
> for
> > a simple 'a' key down.  IBus is installed on many Debian/Ubuntu based system
> by
> > default, and even non-IME users (non intentional IME users) use it.  And not
> few
> > web pages do not expect VKEY_PROCESS events, especially
game-like-interactive
> > web pages written in JS will be broken.
> > 
> > In short, I'm afraid that this change may cause site-compatibility issues,
and
> > need to be very careful.
> 
> A little more: # of non-IME users (typically ASCII only) is greater than # of
> IME users.  So, it's important to not break non-IME world (or, more important
> than fixing IME world :(

Yuki, I just discussed with James offline on this issue.

As mentioned in my previous post, the only justification we can think of is to
support e(229).preventDefault() with off-spot IME.
That case is narrow enough to make us to drop it.

Therefore, we agree with you on the issue, and I will make a new patchset with
the fix to mute the unexpected 229 keydown for non-IME users.

Thanks for your review!

[Shu Chen, 2015-04-09 07:43:38]

Yuki, can you please take another look at the latest patchset #7?

[Yuki, 2015-04-09 07:56:34]

On 2015/04/09 07:43:38, Shu Chen wrote:
> Yuki, can you please take another look at the latest patchset #7?

I vaguely understand that you stop emitting VKEY_PROCESS event in
InputMethodAuraLinux.  However, I believe it's really hard to confirm
IME-related behaviors by code review (without patching + testing).  I'm okay to
go with this CL as long as 1) you're aware of the issues, and 2) willing to
address them.  I don't think 10 min test is enough.  Please take your time to
test various aspects of changes until you get convinced.

I don't think you need my rubber stamp, but if so, I'm happy to do.

[Shu Chen, 2015-04-09 08:36:07]

On 2015/04/09 07:56:34, Yuki wrote:
> On 2015/04/09 07:43:38, Shu Chen wrote:
> > Yuki, can you please take another look at the latest patchset #7?
> 
> I vaguely understand that you stop emitting VKEY_PROCESS event in
> InputMethodAuraLinux.  However, I believe it's really hard to confirm
> IME-related behaviors by code review (without patching + testing).  I'm okay
to
> go with this CL as long as 1) you're aware of the issues, and 2) willing to
> address them.  I don't think 10 min test is enough.  Please take your time to
> test various aspects of changes until you get convinced.
> 
> I don't think you need my rubber stamp, but if so, I'm happy to do.

Thanks, Yuki. It would be nice to get your rubber stamp.
For testing, I manually verified it my ibus environment with US keyboard and
Pinyin IME (both Chinese and English modes) in
textarea/body/password/native-fields and they work as expected.
And all the unit tests pass with the latest patchset. Do you think more tests
should be added?

[Yuki, 2015-04-09 08:38:31]

lgtm

[James Su, 2015-04-09 08:40:02]

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
File ui/base/ime/input_method_auralinux.cc (right):

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux.cc:107: }
add a comment somewhere to clarify that filtered key release events will be
swallowed and not generate 229 key up event.

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/linux/linux...
File ui/base/ime/linux/linux_input_method_context_factory.h (right):

https://codereview.chromium.org/1068093002/diff/60001/ui/base/ime/linux/linux...
ui/base/ime/linux/linux_input_method_context_factory.h:33:
LinuxInputMethodContextDelegate* delegate, bool is_simple) const = 0;
add document for is_simple parameter.

https://codereview.chromium.org/1068093002/diff/120001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/x11_input_method_context_impl_gtk2.cc (right):

https://codereview.chromium.org/1068093002/diff/120001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/x11_input_method_context_impl_gtk2.cc:40:
gtk_context_ = gtk_im_multicontext_new();
nit: gtk_context_ = is_simple ? gtk_im_context_simple_new() :
gtk_im_multicontext_new();

https://codereview.chromium.org/1068093002/diff/120001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/x11_input_method_context_impl_gtk2.cc:49: //
TODO(yukishiino): Handle operations on surrounding text.
change this to TODO(shuchen) ?

https://codereview.chromium.org/1068093002/diff/120001/ui/base/ime/input_meth...
File ui/base/ime/input_method_auralinux.cc (right):

https://codereview.chromium.org/1068093002/diff/120001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:19: composing_text_(false),
I think this variable can be eliminated. Let's discuss it face to face.

https://codereview.chromium.org/1068093002/diff/120001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:102: 
How about add comment to explain why we don't send key press 229 if the key
press is filtered without result.

https://codereview.chromium.org/1068093002/diff/120001/ui/base/ime/linux/linu...
File ui/base/ime/linux/linux_input_method_context.h (right):

https://codereview.chromium.org/1068093002/diff/120001/ui/base/ime/linux/linu...
ui/base/ime/linux/linux_input_method_context.h:30: virtual bool
DispatchKeyEvent(const ui::KeyEvent& key_event) = 0;
This class should accept NativeEvent rather than ui::KeyEvent.

[James Su, 2015-04-09 09:11:40]

https://codereview.chromium.org/1068093002/diff/120001/ui/base/ime/input_meth...
File ui/base/ime/input_method_auralinux_unittest.cc (right):

https://codereview.chromium.org/1068093002/diff/120001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux_unittest.cc:23: const base::char16
kActionCompositionEnd = L'E';
Cover this case in sync mode.

https://codereview.chromium.org/1068093002/diff/120001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux_unittest.cc:38: void AddAction(const
std::string& action) {
How about introduce separated methods for different actions, e.g.
AddCommitAction, etc. So that the calling code can be easier to understand.

https://codereview.chromium.org/1068093002/diff/120001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux_unittest.cc:477: context_->AddAction("U:d");
cover composition end action here?

[Seigo Nonaka, 2015-04-09 17:10:37]

looks good to me, defer to James and Yuki.

[Shu Chen, 2015-04-10 05:26:52]

https://codereview.chromium.org/1068093002/diff/120001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/x11_input_method_context_impl_gtk2.cc (right):

https://codereview.chromium.org/1068093002/diff/120001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/x11_input_method_context_impl_gtk2.cc:40:
gtk_context_ = gtk_im_multicontext_new();
On 2015/04/09 08:40:02, James Su wrote:
> nit: gtk_context_ = is_simple ? gtk_im_context_simple_new() :
> gtk_im_multicontext_new();

Done.

https://codereview.chromium.org/1068093002/diff/120001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/x11_input_method_context_impl_gtk2.cc:49: //
TODO(yukishiino): Handle operations on surrounding text.
On 2015/04/09 08:40:02, James Su wrote:
> change this to TODO(shuchen) ?

Done.

https://codereview.chromium.org/1068093002/diff/120001/ui/base/ime/input_meth...
File ui/base/ime/input_method_auralinux.cc (right):

https://codereview.chromium.org/1068093002/diff/120001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:19: composing_text_(false),
On 2015/04/09 08:40:02, James Su wrote:
> I think this variable can be eliminated. Let's discuss it face to face.

Done.

https://codereview.chromium.org/1068093002/diff/120001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:102: 
On 2015/04/09 08:40:02, James Su wrote:
> How about add comment to explain why we don't send key press 229 if the key
> press is filtered without result.

Done.

https://codereview.chromium.org/1068093002/diff/120001/ui/base/ime/input_meth...
File ui/base/ime/input_method_auralinux_unittest.cc (right):

https://codereview.chromium.org/1068093002/diff/120001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux_unittest.cc:23: const base::char16
kActionCompositionEnd = L'E';
On 2015/04/09 09:11:40, James Su wrote:
> Cover this case in sync mode.

Done.

https://codereview.chromium.org/1068093002/diff/120001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux_unittest.cc:38: void AddAction(const
std::string& action) {
On 2015/04/09 09:11:40, James Su wrote:
> How about introduce separated methods for different actions, e.g.
> AddCommitAction, etc. So that the calling code can be easier to understand.

Done.

https://codereview.chromium.org/1068093002/diff/120001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux_unittest.cc:477: context_->AddAction("U:d");
On 2015/04/09 09:11:40, James Su wrote:
> cover composition end action here?

Done.

https://codereview.chromium.org/1068093002/diff/120001/ui/base/ime/linux/linu...
File ui/base/ime/linux/linux_input_method_context.h (right):

https://codereview.chromium.org/1068093002/diff/120001/ui/base/ime/linux/linu...
ui/base/ime/linux/linux_input_method_context.h:30: virtual bool
DispatchKeyEvent(const ui::KeyEvent& key_event) = 0;
On 2015/04/09 08:40:02, James Su wrote:
> This class should accept NativeEvent rather than ui::KeyEvent. 

Done.

[James Su, 2015-04-10 09:25:17]

https://codereview.chromium.org/1068093002/diff/180001/ui/base/ime/input_meth...
File ui/base/ime/input_method_auralinux.cc (right):

https://codereview.chromium.org/1068093002/diff/180001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:62: result_text_.clear();
clear composition_ here.

https://codereview.chromium.org/1068093002/diff/180001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:82: // composition. This is to workaround
the wierd behavior of IBus with US
wierd -> weird

https://codereview.chromium.org/1068093002/diff/180001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:94: client->InsertText(result_text_);
add comment to explain the logic of this block. Especially why we cannot insert
char here, if the key event is not filtered.

And I'm not sure if your test covers this case.

https://codereview.chromium.org/1068093002/diff/180001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:102: client->ClearCompositionText();
add comment to explain this part of logic. Especially why no need to clear
composition if result text is not empty.

https://codereview.chromium.org/1068093002/diff/180001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:106: composition_.Clear();
I don't understand this if block. I don't think this if check is necessary.

I think you can simply call composition_.Clear() here or at the beginning of
DispatchKeyEvent() method.

https://codereview.chromium.org/1068093002/diff/180001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:107: 
I'm wondering if it's more clear to return true here if filtered is true. So
that we don't need to check !filtered in below if else if blocks.

https://codereview.chromium.org/1068093002/diff/180001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:210: if (suppress_next_result_ ||
!GetTextInputClient()) {
Why not check IsTextInputTypeNone() instead?

https://codereview.chromium.org/1068093002/diff/180001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:233: composition_ = composition_text;
move this line into if (is_sync_mode_) block.

https://codereview.chromium.org/1068093002/diff/180001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:257: composition_.Clear();
remove this line.

[Shu Chen, 2015-04-10 16:17:39]

https://codereview.chromium.org/1068093002/diff/180001/ui/base/ime/input_meth...
File ui/base/ime/input_method_auralinux.cc (right):

https://codereview.chromium.org/1068093002/diff/180001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:62: result_text_.clear();
On 2015/04/10 09:25:16, James Su wrote:
> clear composition_ here.

should not do this per offline discussion.

https://codereview.chromium.org/1068093002/diff/180001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:82: // composition. This is to workaround
the wierd behavior of IBus with US
On 2015/04/10 09:25:16, James Su wrote:
> wierd -> weird

Done.

https://codereview.chromium.org/1068093002/diff/180001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:94: client->InsertText(result_text_);
On 2015/04/10 09:25:17, James Su wrote:
> add comment to explain the logic of this block. Especially why we cannot
insert
> char here, if the key event is not filtered.
> 
> And I'm not sure if your test covers this case.

Done.

https://codereview.chromium.org/1068093002/diff/180001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:102: client->ClearCompositionText();
On 2015/04/10 09:25:17, James Su wrote:
> add comment to explain this part of logic. Especially why no need to clear
> composition if result text is not empty.

Done.

https://codereview.chromium.org/1068093002/diff/180001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:106: composition_.Clear();
On 2015/04/10 09:25:17, James Su wrote:
> I don't understand this if block. I don't think this if check is necessary.
> 
> I think you can simply call composition_.Clear() here or at the beginning of
> DispatchKeyEvent() method.

Comment is added.

https://codereview.chromium.org/1068093002/diff/180001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:107: 
On 2015/04/10 09:25:16, James Su wrote:
> I'm wondering if it's more clear to return true here if filtered is true. So
> that we don't need to check !filtered in below if else if blocks.

I would prefer to avoid returning in the middle of a long method.

https://codereview.chromium.org/1068093002/diff/180001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:210: if (suppress_next_result_ ||
!GetTextInputClient()) {
On 2015/04/10 09:25:16, James Su wrote:
> Why not check IsTextInputTypeNone() instead?

When IsTextInputTypeNone() returns true, here cannot just return, because it
needs to append the result text for later InsertChar.

https://codereview.chromium.org/1068093002/diff/180001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:233: composition_ = composition_text;
On 2015/04/10 09:25:16, James Su wrote:
> move this line into if (is_sync_mode_) block.

Invalid comment per our offline discussion.

https://codereview.chromium.org/1068093002/diff/180001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:257: composition_.Clear();
On 2015/04/10 09:25:16, James Su wrote:
> remove this line.

Invalid comment per our offline discussion.

[James Su, 2015-04-12 14:50:33]

https://codereview.chromium.org/1068093002/diff/200001/ui/base/ime/input_meth...
File ui/base/ime/input_method_auralinux.cc (right):

https://codereview.chromium.org/1068093002/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:116: composition_.Clear();
According to our discussion, we need to keep composition_ in sync with the
actual composition text in the text input client. So I think it's not correct to
clear composition_ here.
Instead, we should only clear it if result_text_ is not empty and
composition_changed_ is false.

[Shu Chen, 2015-04-13 01:48:44]

https://codereview.chromium.org/1068093002/diff/200001/ui/base/ime/input_meth...
File ui/base/ime/input_method_auralinux.cc (right):

https://codereview.chromium.org/1068093002/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:116: composition_.Clear();
On 2015/04/12 14:50:33, James Su wrote:
> According to our discussion, we need to keep composition_ in sync with the
> actual composition text in the text input client. So I think it's not correct
to
> clear composition_ here.
> Instead, we should only clear it if result_text_ is not empty and
> composition_changed_ is false.

Done. Test also updated.

[James Su, 2015-04-13 14:38:33]

On 2015/04/13 01:48:44, Shu Chen wrote:
>
https://codereview.chromium.org/1068093002/diff/200001/ui/base/ime/input_meth...
> File ui/base/ime/input_method_auralinux.cc (right):
> 
>
https://codereview.chromium.org/1068093002/diff/200001/ui/base/ime/input_meth...
> ui/base/ime/input_method_auralinux.cc:116: composition_.Clear();
> On 2015/04/12 14:50:33, James Su wrote:
> > According to our discussion, we need to keep composition_ in sync with the
> > actual composition text in the text input client. So I think it's not
correct
> to
> > clear composition_ here.
> > Instead, we should only clear it if result_text_ is not empty and
> > composition_changed_ is false.
> 
> Done. Test also updated.

LGTM.

[Shu Chen, 2015-04-13 15:12:46]

sky@, can you please review this cl?

The changes under chrome/browser/ui/ need your approval.

Thanks,
Shu

[sky, 2015-04-13 19:41:26]

sky@chromium.org changed reviewers:
+ erg@chromium.org
- sky@chromium.org

[sky, 2015-04-13 19:41:27]

As the changes are all in chrome/browser/ui/libgtk2ui, erg is a better reviewer.
sky->erg

[Elliot Glaysher, 2015-04-13 20:44:06]

lgtm

[Shu Chen, 2015-04-14 00:56:52]

sky@, your approval is needed for some structural changes for:
    chrome/test/BUILD.gn
    ui/base/BUILD.gn
    ui/base/ui_base_tests.gyp

Thanks.

[Shu Chen, 2015-04-15 04:52:24]

On 2015/04/14 00:56:52, Shu Chen wrote:
> sky@, your approval is needed for some structural changes for:
>     chrome/test/BUILD.gn
>     ui/base/BUILD.gn
>     ui/base/ui_base_tests.gyp
> 
> Thanks.

TBR'ing sky@ for the rest structural changes.

[Shu Chen, 2015-04-15 04:52:24]

On 2015/04/14 00:56:52, Shu Chen wrote:
> sky@, your approval is needed for some structural changes for:
>     chrome/test/BUILD.gn
>     ui/base/BUILD.gn
>     ui/base/ui_base_tests.gyp
> 
> Thanks.

TBR'ing sky@ for the rest structural changes.

[Shu Chen, 2015-04-15 04:55:55]

The CQ bit was checked by shuchen@chromium.org

[Shu Chen, 2015-04-15 04:55:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
yukishiino@chromium.org, suzhe@chromium.org, erg@chromium.org
Link to the patchset: https://codereview.chromium.org/1068093002/#ps240001
(title: "rebased.")

[commit-bot: I haz the power, 2015-04-15 04:56:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1068093002/240001

[commit-bot: I haz the power, 2015-04-15 05:52:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-15 05:52:20]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Shu Chen, 2015-04-15 06:11:19]

The CQ bit was checked by shuchen@chromium.org

[Shu Chen, 2015-04-15 06:11:19]

The patchset sent to the CQ was uploaded after l-g-t-m from suzhe@chromium.org,
erg@chromium.org, yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/1068093002/#ps260001
(title: " ")

[commit-bot: I haz the power, 2015-04-15 06:11:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1068093002/260001

[commit-bot: I haz the power, 2015-04-15 08:36:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-15 08:36:59]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Shu Chen, 2015-04-16 00:48:19]

The CQ bit was checked by shuchen@chromium.org

[Shu Chen, 2015-04-16 00:48:19]

The patchset sent to the CQ was uploaded after l-g-t-m from suzhe@chromium.org,
erg@chromium.org, yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/1068093002/#ps280001
(title: "fixed interactive_ui_tests failure.")

[commit-bot: I haz the power, 2015-04-16 00:48:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1068093002/280001

[commit-bot: I haz the power, 2015-04-16 01:43:52]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-04-16 01:44:51]

Patchset 15 (id:??) landed as
https://crrev.com/fad763a26eb994725004358ffdb622f62b744812
Cr-Commit-Position: refs/heads/master@{#325372}