Use input method's context for tracking focus states in OnGrabNotify.

Use input method's context for tracking focus states in OnGrabNotify.

Current implementation of RenderWidgetViewHostGtk::OnGrabNotify()
records the focus state obtained by gtk_widget_is_focus() when
other window get a grab, and set the stored state to the
input context on ungrab notification.

This CL changes to store the focus state of input context rather
than gtk_widget_is_focus(), and resolves the inconsistency issue.

BUG=chromium-os:15773
TEST=http://crosbug.com/15773#c20

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

[kinaba, 2011-08-02 10:25:20]

Hi Evan, can you share some time to review this patch?

The change solves an issue in ChromeOS, but I'm not quite sure that this doesn't
affect other part of Chrome in a bad way. If the _is_focus rather than
_has_focus was an intended behavior, I'll withdraw the current patch and try
another fix.

[Evan Stade, 2011-08-02 18:13:17]

even after some investigation, I'm not entirely sure. It looks like the use in
GrabNotify needs to be is_focus, but you can change just that instance to
is_focus. I suspect the change will otherwise be ok, but I don't know how to be
sure it hasn't broken something.

[kinaba, 2011-08-03 03:44:23]

On 2011/08/02 18:13:17, Evan Stade wrote:
> even after some investigation, I'm not entirely sure. It looks like the use in
> GrabNotify needs to be is_focus, but you can change just that instance to
> is_focus. I suspect the change will otherwise be ok, but I don't know how to
be
> sure it hasn't broken something.

Thanks for the reply! In fact, GrabNotify is the place that I want to change to
<has_focus> (because calling host_view->im_context_->OnFocusIn() when the
renderer window NOT having focus (which happens if we use <is_focus>) is causing
some inconsitency of the im state). Could you tell me the reason that it needs
to be <is_focus>?

[Evan Stade, 2011-08-03 16:22:25]

because (I think) by the time you get the grab notify event, is_focus will
always be false.

The ::grab-notify signal is emitted when a widget becomes shadowed by a GTK+
grab (not a pointer or keyboard grab) on another widget, or when it becomes
unshadowed due to a grab being removed.

i.e. it is called when the widget has (already) lost focus due to a grab. I
could be wrong though.

[kinaba, 2011-08-03 17:20:25]

On 2011/08/03 16:22:25, Evan Stade wrote:
> i.e. it is called when the widget has (already) lost focus due to a grab.

Not really. As the comment above OnGrabNotify says, 
> Popup windows do not take focus, so we never get a focus out
by a grab.
so is_focus and has_focus are not affected by the grab.

I've inserted log outputs here and seen a problematic behavior like:
1. RenderWidgetGtk focused (is_focus=TRUE, has_focus=TRUE,
imcontext->is_focused_=TRUE)
2. Open and focus another browser window (is_focus=TRUE, has_focus=FALSE,
imcontext->is_focused_=FALSE)
3. Show a context menu on the omnibox of the new window (is_focus=TRUE,
has_focus=FALSE, imcontext->is_focused_=FALSE)
4. The menu takes a grab (was_focused_before_grab_ := is_focus)
5. and releases the grab (imcontext->is_focused_ := was_focused_before_grab_)
6. (is_focus=TRUE, has_focus=FALSE, imcontext->focus=TRUE)
The step 6 should have the same state (T,F,F) as the step 3 but it isn't.

[kinaba, 2011-08-03 17:24:51]

Ah, but, well, now I began to think that the best way is to introduce a method
to get the state of the flag imcontext->is_focused_ and do
 was_focused_before_grab_ = imcontext->is_focused_; // not is_focus, nor
has_focus
in OnGrabNotify.

If you don't have objection, I will take this approach.
I.e., don't change HasFocus() implementation with gtk_widget_is_focus(), and
just replace the instance of HasFocus() call in OnGrabNotify with
imcontext->is_focused_.

[Evan Stade, 2011-08-03 19:48:24]

ok

[kinaba, 2011-08-04 03:58:47]

On 2011/08/03 19:48:24, Evan Stade wrote:
> ok

Done. Could you please review it?

[Evan Stade, 2011-08-04 04:00:47]

just nits

http://codereview.chromium.org/7548008/diff/8001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/gtk_im_context_wrapper.h (right):

http://codereview.chromium.org/7548008/diff/8001/chrome/browser/renderer_host...
chrome/browser/renderer_host/gtk_im_context_wrapper.h:56: bool IsFocused()
const;
simple getters should look_like_this() and can be defined inline in the header
file

http://codereview.chromium.org/7548008/diff/8001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/7548008/diff/8001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_widget_host_view_gtk.cc:237:
host_view->was_focused_before_grab_ = host_view->im_context_->IsFocused();
seems like was_focused_before_grab_ is not really the most accurate name for
this variable any more

[kinaba, 2011-08-04 04:24:37]

http://codereview.chromium.org/7548008/diff/8001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/gtk_im_context_wrapper.h (right):

http://codereview.chromium.org/7548008/diff/8001/chrome/browser/renderer_host...
chrome/browser/renderer_host/gtk_im_context_wrapper.h:56: bool IsFocused()
const;
On 2011/08/04 04:00:47, Evan Stade wrote:
> simple getters should look_like_this() and can be defined inline in the header
> file

Done.

http://codereview.chromium.org/7548008/diff/8001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/7548008/diff/8001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_widget_host_view_gtk.cc:237:
host_view->was_focused_before_grab_ = host_view->im_context_->IsFocused();
On 2011/08/04 04:00:47, Evan Stade wrote:
> seems like was_focused_before_grab_ is not really the most accurate name for
> this variable any more

Done.

[kinaba, 2011-08-04 04:29:37]

Changed the summary and description text of the issue.

[Evan Stade, 2011-08-04 16:47:28]

lgtm

http://codereview.chromium.org/7548008/diff/12001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_gtk.h (right):

http://codereview.chromium.org/7548008/diff/12001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_gtk.h:214: // Whether or
not this widget's input context was focused before shadowed by
before being shadowed

[kinaba, 2011-08-05 03:35:33]

http://codereview.chromium.org/7548008/diff/12001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_gtk.h (right):

http://codereview.chromium.org/7548008/diff/12001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_gtk.h:214: // Whether or
not this widget's input context was focused before shadowed by
On 2011/08/04 16:47:28, Evan Stade wrote:
> before being shadowed

Done.

[commit-bot: I haz the power, 2011-08-05 04:08:32]

Change committed as 95573