Makes sure the keyboard typing isn't blocked when InputMethod::OnFocus() is not correctly called.

Makes sure the keyboard typing isn't blocked when InputMethod::OnFocus() is not correctly called.

BUG=569339, 505231
TEST=None

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

[Shu Chen, 2016-01-07 07:43:29]

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

[Shu Chen, 2016-01-07 07:43:29]

yukawa@ & sky@, can you please take a look? Thanks

[yukawa, 2016-01-07 09:23:56]

lgtm

https://codereview.chromium.org/1566083002/diff/20001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/1566083002/diff/20001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:388: // Refreshes the
focus info to IMF in case that IMF cached the old info
(optional) I'm fine with moving those logic, but can we keep these lines in the
original location for a while so that users in crbug.com/569339 can see if
LOG(ERROR) is really recorded or not?.

[yukawa, 2016-01-07 11:09:42]

https://codereview.chromium.org/1566083002/diff/20001/ui/base/ime/input_metho...
File ui/base/ime/input_method_win.cc (right):

https://codereview.chromium.org/1566083002/diff/20001/ui/base/ime/input_metho...
ui/base/ime/input_method_win.cc:267: "Char message got without the top-level
window being activated.";
(optional) We can also log |GetActiveWindow() == toplevel_window_handle_| as an
evidence that |system_toplevel_window_focused_| should have been true here.

[sky, 2016-01-07 16:24:42]

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

https://codereview.chromium.org/1566083002/diff/20001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux.cc:51: "Key event got without the top-level
window being activated.";
Under what conditions does this happen? It seems like you're working around a
bug in the rest of the code.

[Shu Chen, 2016-01-08 02:04:11]

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

https://codereview.chromium.org/1566083002/diff/20001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux.cc:51: "Key event got without the top-level
window being activated.";
On 2016/01/07 16:24:42, sky wrote:
> Under what conditions does this happen? It seems like you're working around a
> bug in the rest of the code.

Yes, exactly. crbug.com/569339 causes a lot of complaints, but it is hard to
repro.
The log here is help us to hunt down the root cause.
I'm trying to add more logs at multiple places and put those logs here (I will
upload a new patch soon).

The following OnFocus() call is to ease the pain for users (the bug can cause
user lose I think).

https://codereview.chromium.org/1566083002/diff/20001/ui/base/ime/input_metho...
File ui/base/ime/input_method_win.cc (right):

https://codereview.chromium.org/1566083002/diff/20001/ui/base/ime/input_metho...
ui/base/ime/input_method_win.cc:267: "Char message got without the top-level
window being activated.";
On 2016/01/07 11:09:42, yukawa wrote:
> (optional) We can also log |GetActiveWindow() == toplevel_window_handle_| as
an
> evidence that |system_toplevel_window_focused_| should have been true here.

I think ::GetActiveWindow() could return non-top-level window. Right?

https://codereview.chromium.org/1566083002/diff/20001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/1566083002/diff/20001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:388: // Refreshes the
focus info to IMF in case that IMF cached the old info
On 2016/01/07 09:23:56, yukawa wrote:
> (optional) I'm fine with moving those logic, but can we keep these lines in
the
> original location for a while so that users in crbug.com/569339 can see if
> LOG(ERROR) is really recorded or not?.

Done.

[yukawa, 2016-01-08 02:21:20]

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

https://codereview.chromium.org/1566083002/diff/20001/ui/base/ime/input_metho...
ui/base/ime/input_method_auralinux.cc:51: "Key event got without the top-level
window being activated.";
On 2016/01/08 02:04:11, Shu Chen wrote:
> On 2016/01/07 16:24:42, sky wrote:
> > Under what conditions does this happen? It seems like you're working around
a
> > bug in the rest of the code.
> 
> Yes, exactly. crbug.com/569339 causes a lot of complaints, but it is hard to
> repro.
> The log here is help us to hunt down the root cause.
> I'm trying to add more logs at multiple places and put those logs here (I will
> upload a new patch soon).
> 
> The following OnFocus() call is to ease the pain for users (the bug can cause
> user lose I think).

My understanding is the same to what Shu explained.  Given that we have
sufficient reporters in crbug.com/569339 who can relatively easily reproduce
this issue, having only logging code in this CL is also fine to me.

https://codereview.chromium.org/1566083002/diff/20001/ui/base/ime/input_metho...
File ui/base/ime/input_method_win.cc (right):

https://codereview.chromium.org/1566083002/diff/20001/ui/base/ime/input_metho...
ui/base/ime/input_method_win.cc:267: "Char message got without the top-level
window being activated.";
On 2016/01/08 02:04:11, Shu Chen wrote:
> On 2016/01/07 11:09:42, yukawa wrote:
> > (optional) We can also log |GetActiveWindow() == toplevel_window_handle_| as
> an
> > evidence that |system_toplevel_window_focused_| should have been true here.
> 
> I think ::GetActiveWindow() could return non-top-level window. Right?

(just FYI) AFAIK it always returns a top-level window, or NULL.  (We have
actually relied on that API in InputMethodWin::IsWindowFocused()).

[Shu Chen, 2016-01-08 09:00:16]

https://codereview.chromium.org/1566083002/diff/20001/ui/base/ime/input_metho...
File ui/base/ime/input_method_win.cc (right):

https://codereview.chromium.org/1566083002/diff/20001/ui/base/ime/input_metho...
ui/base/ime/input_method_win.cc:267: "Char message got without the top-level
window being activated.";
On 2016/01/08 02:21:20, yukawa wrote:
> On 2016/01/08 02:04:11, Shu Chen wrote:
> > On 2016/01/07 11:09:42, yukawa wrote:
> > > (optional) We can also log |GetActiveWindow() == toplevel_window_handle_|
as
> > an
> > > evidence that |system_toplevel_window_focused_| should have been true
here.
> > 
> > I think ::GetActiveWindow() could return non-top-level window. Right?
> 
> (just FYI) AFAIK it always returns a top-level window, or NULL.  (We have
> actually relied on that API in InputMethodWin::IsWindowFocused()).

Done.

[Shu Chen, 2016-01-08 09:25:50]

I've added some logs. Please review the latest patch. Thanks

[sky, 2016-01-08 16:51:58]

Do you actually have customers that are handing you logs? I know we have a way
to log what looks like a crash dump without actually crashing, it might be
better to do that here if you can include the extra information. I also suspect
you aren't including enough information. You aren't logging hwnds to tie the
data together.

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

https://codereview.chromium.org/1566083002/diff/160001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:49: if (!system_toplevel_window_focused())
{
I think you should see if the toplevel is actually active when we get a key
event like this.

https://codereview.chromium.org/1566083002/diff/160001/ui/base/ime/input_meth...
File ui/base/ime/input_method_log_collector.cc (right):

https://codereview.chromium.org/1566083002/diff/160001/ui/base/ime/input_meth...
ui/base/ime/input_method_log_collector.cc:1: // Copyright (c) 2016 The Chromium
Authors. All rights reserved.
or here.

https://codereview.chromium.org/1566083002/diff/160001/ui/base/ime/input_meth...
File ui/base/ime/input_method_log_collector.h (right):

https://codereview.chromium.org/1566083002/diff/160001/ui/base/ime/input_meth...
ui/base/ime/input_method_log_collector.h:1: // Copyright (c) 2016 The Chromium
Authors. All rights reserved.
don't use (c) anymore.

https://codereview.chromium.org/1566083002/diff/160001/ui/views/widget/deskto...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/1566083002/diff/160001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:393: "Missing OnFocus
call when activating.");
What about the early return on 374 too?

[Shu Chen, 2016-01-09 01:45:53]

On 2016/01/08 16:51:58, sky wrote:
> Do you actually have customers that are handing you logs? I know we have a way
> to log what looks like a crash dump without actually crashing, it might be
> better to do that here if you can include the extra information. I also
suspect
> you aren't including enough information. You aren't logging hwnds to tie the
> data together.

I was thinking to ask customers to try and gather the logs.
If there is a way to collect the logs like this on a server, it would be great!
Can you please share code samples of how to do it? And where to get those logs?

For lacking of hwnds, can you please point out the specific logs need the hwnds
info?
I am not sure whether hwnds can help to analyze the issue because the logs are
within
InputMethod instance scope, and each InputMethod maps a top-level HWND.

[Shu Chen, 2016-01-09 01:46:02]

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

https://codereview.chromium.org/1566083002/diff/160001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:49: if (!system_toplevel_window_focused())
{
On 2016/01/08 16:51:57, sky wrote:
> I think you should see if the toplevel is actually active when we get a key
> event like this.

Line 1530 of desktop_window_tree_host_x11.cc makes sure DispatchKeyEvent is
called only when Widget::IsActive() returns true.

To check the window state at X11 level, do you know the XWindow API to do that?

https://codereview.chromium.org/1566083002/diff/160001/ui/base/ime/input_meth...
File ui/base/ime/input_method_log_collector.cc (right):

https://codereview.chromium.org/1566083002/diff/160001/ui/base/ime/input_meth...
ui/base/ime/input_method_log_collector.cc:1: // Copyright (c) 2016 The Chromium
Authors. All rights reserved.
On 2016/01/08 16:51:57, sky wrote:
> or here.

Done.

https://codereview.chromium.org/1566083002/diff/160001/ui/base/ime/input_meth...
File ui/base/ime/input_method_log_collector.h (right):

https://codereview.chromium.org/1566083002/diff/160001/ui/base/ime/input_meth...
ui/base/ime/input_method_log_collector.h:1: // Copyright (c) 2016 The Chromium
Authors. All rights reserved.
On 2016/01/08 16:51:57, sky wrote:
> don't use (c) anymore.

Done.

https://codereview.chromium.org/1566083002/diff/160001/ui/views/widget/deskto...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/1566083002/diff/160001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:393: "Missing OnFocus
call when activating.");
On 2016/01/08 16:51:58, sky wrote:
> What about the early return on 374 too?

Thanks for catching it. Log is added.

[Shu Chen, 2016-01-09 12:33:31]

I'm thinking it may be better to split this cl into 2.
One to actually avoid blocking keyboard typing, and another one to add logs. So
that the first cl can be merged to M48 to ease the user pains.

[sky, 2016-01-11 16:25:51]

What I think should happen is if you hit the conditional in
InputMethodWin::OnChar() that causes you to call OnFocus() you log
information that can help diagnose the problem. Talk with ericroman to
see if he has suggestions on how to go about that.

  -Scott

On Sat, Jan 9, 2016 at 4:33 AM,  <shuchen@chromium.org> wrote:
> I'm thinking it may be better to split this cl into 2.
> One to actually avoid blocking keyboard typing, and another one to add logs.
> So
> that the first cl can be merged to M48 to ease the user pains.
>
>
> https://codereview.chromium.org/1566083002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Shu Chen, 2016-01-13 04:03:33]

shuchen@chromium.org changed reviewers:
+ eroman@chromium.org

[Shu Chen, 2016-01-13 04:03:33]

I've now use dump instead of logging so that the logs can be collected without
users' help.

+eroman@ for the use of DumpWithoutCrashing().

Please take another look. Thanks!

[eroman, 2016-01-13 22:27:08]

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

https://codereview.chromium.org/1566083002/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:50: LOG(ERROR) << "Key event got without
the top-level window being activated.";
Do you want to add this to the logs dumped?

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

https://codereview.chromium.org/1566083002/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_log_collector.cc:23: if (logs_.empty())
Is knowing that the logs_ was empty useful in itself? If so might want to copy
logs_.size() to a stack variable and dump anyway in this case.

https://codereview.chromium.org/1566083002/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_log_collector.cc:26: char logs_copy[4096];
See suggestion for using string literals (const char*).

If you go that approach, should be sufficient to instead copy a const char*
logs_copy[<max-logs>], without needing to copy the string content itself to the
stack

https://codereview.chromium.org/1566083002/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_log_collector.cc:28: for (size_t i = 0; i <
logs_.size() && copy_from < sizeof(logs_copy); ++i) {
I didn't review this yet (in case you change to the other approach).

https://codereview.chromium.org/1566083002/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_log_collector.cc:40:
base::debug::DumpWithoutCrashing();
How often do you expect this to be hit? Do you have an estimate for how many
crash dumps it will generate for users?

If this is called often, it will be a problem for users (as
DumpWithoutCrashing() is expensive and can stall the application).
I would recommend adding some sort of safety net: either a maximum number of
times that DumpWithoutCrashing() will be called here (i.e. counter), or use
randomness to call this only on some percentage of times reached.

https://codereview.chromium.org/1566083002/diff/200001/ui/base/ime/input_meth...
File ui/base/ime/input_method_log_collector.h (right):

https://codereview.chromium.org/1566083002/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_log_collector.h:24: std::vector<std::string> logs_;
A couple design comments:

(1) How about making this a vector of string literals instead:
   std::vector<const char*> logs_;

From reading the callers of AddLog(), they all just pass string literals (except
for 2 callers which stringize a boolean, but that is easily changed into one of
two string literals).

The advantage of making them string literals is less memory overhead for
maintaining this log, and for copying the data to the stack (will be sufficient
to copy the literal pointers, as the literal content will be part of the binary
and easily recoverable).

(2) How large can logs_ grow? I am not very familiar with the surrounding code,
but I suggest putting in some defensive code to prevent the logs_ vector from
growing beyond a certain amount (maybe 10 or 20 is sufficient)? Provided the
content is string literals rather than std::string it is easy to predict the
worst case memory usage now.

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

https://codereview.chromium.org/1566083002/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_win.cc:266: LOG(ERROR)
Are these messages you want dumped?

[Shu Chen, 2016-01-14 07:44:47]

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

https://codereview.chromium.org/1566083002/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_auralinux.cc:50: LOG(ERROR) << "Key event got without
the top-level window being activated.";
On 2016/01/13 22:27:08, eroman wrote:
> Do you want to add this to the logs dumped?

Done.

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

https://codereview.chromium.org/1566083002/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_log_collector.cc:23: if (logs_.empty())
On 2016/01/13 22:27:08, eroman wrote:
> Is knowing that the logs_ was empty useful in itself? If so might want to copy
> logs_.size() to a stack variable and dump anyway in this case.

Done.

https://codereview.chromium.org/1566083002/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_log_collector.cc:26: char logs_copy[4096];
On 2016/01/13 22:27:08, eroman wrote:
> See suggestion for using string literals (const char*).
> 
> If you go that approach, should be sufficient to instead copy a const char*
> logs_copy[<max-logs>], without needing to copy the string content itself to
the
> stack

Done.

https://codereview.chromium.org/1566083002/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_log_collector.cc:28: for (size_t i = 0; i <
logs_.size() && copy_from < sizeof(logs_copy); ++i) {
On 2016/01/13 22:27:08, eroman wrote:
> I didn't review this yet (in case you change to the other approach).

Done.

https://codereview.chromium.org/1566083002/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_log_collector.cc:40:
base::debug::DumpWithoutCrashing();
On 2016/01/13 22:27:08, eroman wrote:
> How often do you expect this to be hit? Do you have an estimate for how many
> crash dumps it will generate for users?
> 
> If this is called often, it will be a problem for users (as
> DumpWithoutCrashing() is expensive and can stall the application).
> I would recommend adding some sort of safety net: either a maximum number of
> times that DumpWithoutCrashing() will be called here (i.e. counter), or use
> randomness to call this only on some percentage of times reached.

Done. I've made it only dump 5 times at maximum.

https://codereview.chromium.org/1566083002/diff/200001/ui/base/ime/input_meth...
File ui/base/ime/input_method_log_collector.h (right):

https://codereview.chromium.org/1566083002/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_log_collector.h:24: std::vector<std::string> logs_;
On 2016/01/13 22:27:08, eroman wrote:
> A couple design comments:
> 
> (1) How about making this a vector of string literals instead:
>    std::vector<const char*> logs_;
> 
> From reading the callers of AddLog(), they all just pass string literals
(except
> for 2 callers which stringize a boolean, but that is easily changed into one
of
> two string literals).
> 
> The advantage of making them string literals is less memory overhead for
> maintaining this log, and for copying the data to the stack (will be
sufficient
> to copy the literal pointers, as the literal content will be part of the
binary
> and easily recoverable).
> 
> (2) How large can logs_ grow? I am not very familiar with the surrounding
code,
> but I suggest putting in some defensive code to prevent the logs_ vector from
> growing beyond a certain amount (maybe 10 or 20 is sufficient)? Provided the
> content is string literals rather than std::string it is easy to predict the
> worst case memory usage now.

Makes sense. I've followed your suggestions.

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

https://codereview.chromium.org/1566083002/diff/200001/ui/base/ime/input_meth...
ui/base/ime/input_method_win.cc:266: LOG(ERROR)
On 2016/01/13 22:27:08, eroman wrote:
> Are these messages you want dumped?

Done.

[eroman, 2016-01-14 19:44:13]

the dumping portion looks good once comments around AddNumber() are address
(i.e. remove it).

https://codereview.chromium.org/1566083002/diff/240001/ui/base/ime/input_meth...
File ui/base/ime/input_method_log_collector.cc (right):

https://codereview.chromium.org/1566083002/diff/240001/ui/base/ime/input_meth...
ui/base/ime/input_method_log_collector.cc:20: if (logs_.size() >= 50)
I suggest making 50 a constant, since it is re-used lower down (and needs to
match for correctness).

https://codereview.chromium.org/1566083002/diff/240001/ui/base/ime/input_meth...
ui/base/ime/input_method_log_collector.cc:29: void
InputMethodLogCollector::AddNumber(int num_val) {
AddNumber() is unused as far as I can tell, why include it?

https://codereview.chromium.org/1566083002/diff/240001/ui/base/ime/input_meth...
ui/base/ime/input_method_log_collector.cc:31:
AddString(number_strings_.back().c_str());
This wouldn't really work since the backing memory for this string is not kept
alive in the dump. If you want the numbers can instead just keep a vector of
ints, and copy that to the stack as well when dumping.

https://codereview.chromium.org/1566083002/diff/240001/ui/base/ime/input_meth...
ui/base/ime/input_method_log_collector.cc:42: for (size_t i = 0; i < log_count;
++i)
To be extra safe I suggest writing it as "i < log_count &&  i <
arraysize(logs_copy)". Since otherwise if the maximum size is ever changed
beyond 50 could lead to memory error.

[Shu Chen, 2016-01-16 04:15:46]

https://codereview.chromium.org/1566083002/diff/240001/ui/base/ime/input_meth...
File ui/base/ime/input_method_log_collector.cc (right):

https://codereview.chromium.org/1566083002/diff/240001/ui/base/ime/input_meth...
ui/base/ime/input_method_log_collector.cc:20: if (logs_.size() >= 50)
On 2016/01/14 19:44:13, eroman wrote:
> I suggest making 50 a constant, since it is re-used lower down (and needs to
> match for correctness).

Done.

https://codereview.chromium.org/1566083002/diff/240001/ui/base/ime/input_meth...
ui/base/ime/input_method_log_collector.cc:29: void
InputMethodLogCollector::AddNumber(int num_val) {
On 2016/01/14 19:44:13, eroman wrote:
> AddNumber() is unused as far as I can tell, why include it?

Done. I've removed it.

https://codereview.chromium.org/1566083002/diff/240001/ui/base/ime/input_meth...
ui/base/ime/input_method_log_collector.cc:31:
AddString(number_strings_.back().c_str());
On 2016/01/14 19:44:13, eroman wrote:
> This wouldn't really work since the backing memory for this string is not kept
> alive in the dump. If you want the numbers can instead just keep a vector of
> ints, and copy that to the stack as well when dumping.

Thanks. I've removed the AddNumber method.

https://codereview.chromium.org/1566083002/diff/240001/ui/base/ime/input_meth...
ui/base/ime/input_method_log_collector.cc:42: for (size_t i = 0; i < log_count;
++i)
On 2016/01/14 19:44:13, eroman wrote:
> To be extra safe I suggest writing it as "i < log_count &&  i <
> arraysize(logs_copy)". Since otherwise if the maximum size is ever changed
> beyond 50 could lead to memory error.

Done.

[Shu Chen, 2016-01-16 04:24:12]

I'm not sure whether this can catch Jan 18 (Mon) M48 stable cut.

sky@, do you have any more comments besides eroman@'s comments?

[Shu Chen, 2016-01-19 04:28:00]

The CQ bit was checked by shuchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-19 04:28:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1566083002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1566083002/280001

[commit-bot: I haz the power, 2016-01-19 05:03:09]

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

[commit-bot: I haz the power, 2016-01-19 05:03:10]

Dry run: Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[Shu Chen, 2016-01-19 05:36:31]

The CQ bit was checked by shuchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-19 05:36:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1566083002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1566083002/300001

[commit-bot: I haz the power, 2016-01-19 06:15:43]

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

[commit-bot: I haz the power, 2016-01-19 06:15:44]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Shu Chen, 2016-01-19 06:28:57]

The CQ bit was checked by shuchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-19 06:29:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1566083002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1566083002/320001

[commit-bot: I haz the power, 2016-01-19 07:43:56]

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

[commit-bot: I haz the power, 2016-01-19 07:43:56]

Dry run: This issue passed the CQ dry run.

[sky, 2016-01-19 16:59:21]

LGTM - I was out Friday, sorry.

https://codereview.chromium.org/1566083002/diff/320001/ui/base/ime/input_meth...
File ui/base/ime/input_method_log_collector.h (right):

https://codereview.chromium.org/1566083002/diff/320001/ui/base/ime/input_meth...
ui/base/ime/input_method_log_collector.h:15: class UI_BASE_IME_EXPORT
InputMethodLogCollector {
Add a description as to what this is for, and make sure to include the bug id.

https://codereview.chromium.org/1566083002/diff/320001/ui/base/ime/input_meth...
ui/base/ime/input_method_log_collector.h:18: virtual ~InputMethodLogCollector();
You don't need virtual here, do you?

https://codereview.chromium.org/1566083002/diff/320001/ui/base/ime/input_meth...
ui/base/ime/input_method_log_collector.h:26: };
DISALLOW...

[Shu Chen, 2016-01-20 00:31:49]

eroman@, can you please take another look? Thanks

https://codereview.chromium.org/1566083002/diff/320001/ui/base/ime/input_meth...
File ui/base/ime/input_method_log_collector.h (right):

https://codereview.chromium.org/1566083002/diff/320001/ui/base/ime/input_meth...
ui/base/ime/input_method_log_collector.h:15: class UI_BASE_IME_EXPORT
InputMethodLogCollector {
On 2016/01/19 16:59:21, sky wrote:
> Add a description as to what this is for, and make sure to include the bug id.

Done.

https://codereview.chromium.org/1566083002/diff/320001/ui/base/ime/input_meth...
ui/base/ime/input_method_log_collector.h:18: virtual ~InputMethodLogCollector();
On 2016/01/19 16:59:21, sky wrote:
> You don't need virtual here, do you?

Done.

https://codereview.chromium.org/1566083002/diff/320001/ui/base/ime/input_meth...
ui/base/ime/input_method_log_collector.h:26: };
On 2016/01/19 16:59:20, sky wrote:
> DISALLOW...

Done.

[eroman, 2016-01-20 00:46:01]

lgtm

[Shu Chen, 2016-01-20 01:43:30]

The CQ bit was checked by shuchen@chromium.org

[Shu Chen, 2016-01-20 01:43:30]

The patchset sent to the CQ was uploaded after l-g-t-m from yukawa@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1566083002/#ps340001
(title: "addressed sky@'s comments.")

[commit-bot: I haz the power, 2016-01-20 01:45:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1566083002/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1566083002/340001

[commit-bot: I haz the power, 2016-01-20 01:55:39]

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

[commit-bot: I haz the power, 2016-01-20 01:55:40]

Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Shu Chen, 2016-01-20 02:17:47]

The CQ bit was checked by shuchen@chromium.org

[Shu Chen, 2016-01-20 02:17:47]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org,
yukawa@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1566083002/#ps360001
(title: "compiled.")

[commit-bot: I haz the power, 2016-01-20 02:20:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1566083002/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1566083002/360001

[commit-bot: I haz the power, 2016-01-20 02:42:40]

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

[commit-bot: I haz the power, 2016-01-20 02:42:41]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[Shu Chen, 2016-01-20 03:04:50]

The CQ bit was checked by shuchen@chromium.org

[commit-bot: I haz the power, 2016-01-20 03:06:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1566083002/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1566083002/360001

[Shu Chen, 2016-01-20 03:53:03]

The CQ bit was checked by shuchen@chromium.org

[Shu Chen, 2016-01-20 03:53:03]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org,
yukawa@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1566083002/#ps380001
(title: "remote_input_method_win.cc is back.")

[commit-bot: I haz the power, 2016-01-20 03:53:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1566083002/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1566083002/380001

[commit-bot: I haz the power, 2016-01-20 04:19:15]

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

[commit-bot: I haz the power, 2016-01-20 04:19:16]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Shu Chen, 2016-01-20 04:57:51]

The CQ bit was checked by shuchen@chromium.org

[Shu Chen, 2016-01-20 04:57:51]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org,
yukawa@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1566083002/#ps400001
(title: "remote_input_method_win.cc is gone again.")

[commit-bot: I haz the power, 2016-01-20 04:59:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1566083002/400001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1566083002/400001

[commit-bot: I haz the power, 2016-01-20 05:43:52]

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

[commit-bot: I haz the power, 2016-01-20 05:43:53]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Shu Chen, 2016-01-20 06:26:49]

The CQ bit was checked by shuchen@chromium.org

[commit-bot: I haz the power, 2016-01-20 06:27:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1566083002/400001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1566083002/400001

[commit-bot: I haz the power, 2016-01-20 08:46:39]

Committed patchset #21 (id:400001)

[commit-bot: I haz the power, 2016-01-20 08:47:49]

Description was changed from

==========
Makes sure the keyboard typing isn't blocked when InputMethod::OnFocus() is not
correctly called.

BUG=569339,505231
TEST=None
==========

to

==========
Makes sure the keyboard typing isn't blocked when InputMethod::OnFocus() is not
correctly called.

BUG=569339,505231
TEST=None

Committed: https://crrev.com/d6b55d458f5d3ebeba2826520f14b4bf0e571396
Cr-Commit-Position: refs/heads/master@{#370340}
==========

[commit-bot: I haz the power, 2016-01-20 08:47:50]

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

[ramant (doing other things), 2016-01-21 22:19:03]

A revert of this CL (patchset #21 id:400001) has been created in
https://codereview.chromium.org/1615073005/ by rtenneti@chromium.org.

The reason for reverting is: Very high crash rate .