Browser Side Text Input State Tracking for OOPIF.

Browser Side Text Input State Tracking for OOPIF.

Currently, the TextInputStateChanged method is not implemented in RenderWidgetHostViewChildFrame. Conequently, the IME
will not work until this feature is fixed.

This CL implements the text input state tracking through refactoring most of the TextInputStateChanged code to the top-level RenderWidgetHostViewBase.

This CL should also fix a previously reported race condition in the palindrome crbug.com/546645 caused by RenderWidgetHostViewGuest's way of handling TextInputStateChanged.

BUG=578168, 546645
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/2bd4a2f0c0f114979d47f4498ea1bce9b091591e
Cr-Commit-Position: refs/heads/master@{#385537}

[EhsanK, 2016-01-29 21:08:19]

Description was changed from

==========
Implementing TextInputStateChanged for OOPIF.
Implementing TextInputStateChanged for OOPIF.

Currently, the TextInputStateChanged method is not implemented in
RenderWidgetHostViewChildFrame. Conequently, the IME
will not work until this feature is fixed. Furthermore, for non-OOPIF-based
<webview>, the method is implemented in
RenderWidgetHostViewGuest but current implementation is palindromically racy
(crbug.com/546645).

This CL implements the TextInputStateChanged function through refactoring most
of the code to the top-level RenderWidgetHostViewBase. The implementation is
based on FrameTree::FocusedFrame() and hecne does not apply to non-OOPIF
<webview> (RenderWidgetHostViewGuest). Therefore, temporarily, a separate code
path is
added to support the feauture for current implementation of <webview> based on
BrowserPlugin and will be removed later.

BUG=546645,578168
==========

to

==========
Implementing TextInputStateChanged for OOPIF.
Implementing TextInputStateChanged for OOPIF.

Currently, the TextInputStateChanged method is not implemented in
RenderWidgetHostViewChildFrame. Conequently, the IME
will not work until this feature is fixed. Furthermore, for non-OOPIF-based
<webview>, the method is implemented in
RenderWidgetHostViewGuest but current implementation is palindromically racy
(crbug.com/546645).

This CL implements the TextInputStateChanged function through refactoring most
of the code to the top-level RenderWidgetHostViewBase. The implementation is
based on FrameTree::FocusedFrame() and hecne does not apply to non-OOPIF
<webview> (RenderWidgetHostViewGuest). Therefore, temporarily, a separate code
path is
added to support the feauture for current implementation of <webview> based on
BrowserPlugin and will be removed later.

BUG=546645,578168
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[EhsanK, 2016-01-29 21:09:07]

ekaramad@chromium.org changed reviewers:
+ kenrb@chromium.org

[EhsanK, 2016-01-29 21:09:08]

PTAL.

[Charlie Reis, 2016-01-30 00:10:20]

[+site-isolation-reviews]

[EhsanK, 2016-02-01 14:57:36]

Description was changed from

==========
Implementing TextInputStateChanged for OOPIF.
Implementing TextInputStateChanged for OOPIF.

Currently, the TextInputStateChanged method is not implemented in
RenderWidgetHostViewChildFrame. Conequently, the IME
will not work until this feature is fixed. Furthermore, for non-OOPIF-based
<webview>, the method is implemented in
RenderWidgetHostViewGuest but current implementation is palindromically racy
(crbug.com/546645).

This CL implements the TextInputStateChanged function through refactoring most
of the code to the top-level RenderWidgetHostViewBase. The implementation is
based on FrameTree::FocusedFrame() and hecne does not apply to non-OOPIF
<webview> (RenderWidgetHostViewGuest). Therefore, temporarily, a separate code
path is
added to support the feauture for current implementation of <webview> based on
BrowserPlugin and will be removed later.

BUG=546645,578168
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Implementing TextInputStateChanged for OOPIF.

Currently, the TextInputStateChanged method is not implemented in
RenderWidgetHostViewChildFrame. Conequently, the IME
will not work until this feature is fixed. Furthermore, for non-OOPIF-based
<webview>, the method is implemented in
RenderWidgetHostViewGuest but current implementation is palindromically racy
(crbug.com/546645).

This CL implements the TextInputStateChanged function through refactoring most
of the code to the top-level RenderWidgetHostViewBase. The implementation is
based on FrameTree::FocusedFrame() and hecne does not apply to non-OOPIF
<webview> (RenderWidgetHostViewGuest). Therefore, temporarily, a separate code
path is
added to support the feauture for current implementation of <webview> based on
BrowserPlugin and will be removed later.

BUG=546645,578168
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[kenrb, 2016-02-01 20:12:43]

A few comments. It would be really nice if we didn't have to expand the
exceptions for including frame_host/ files from renderer_host/ code.

https://codereview.chromium.org/1652483002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/1652483002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_widget_host_view_child_frame.h:88: bool
ShouldObtainTextInputStateFromSubFrameViews() const override;
nit: SubFrame should be ChildFrame, to keep terminology consistent.

https://codereview.chromium.org/1652483002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/1652483002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_widget_host_view_guest.cc:369: //
InterstitialPages are not WebContents so we cannot interceptG
nit: typo

https://codereview.chromium.org/1652483002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/DEPS (right):

https://codereview.chromium.org/1652483002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/DEPS:49: "render_widget_host_view_aura\.cc":[
The space character shouldn't be removed.

https://codereview.chromium.org/1652483002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/DEPS:55: ],
I see that this is mostly replicating the exception above it (originally added
by jam@ when he added GetFocusedFrame()), since you are moving code from
RWHVAura to RWHVBase, but this list is getting long. I wonder if Charlie or
Nasko has an idea how to untangle this. It seems difficult since we don't even
have access to the WebContents.

https://codereview.chromium.org/1652483002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_base.cc (right):

https://codereview.chromium.org/1652483002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_base.cc:741: if
(!ShouldObtainTextInputStateFromSubFrameViews())
Why is this needed? Can't you just compare GetFocusedFrame()->GetView() to
|this| and recurse on GetFocusedFrame() if they are different? Or is this method
only required because of the BrowserPlugin alternate code path?

https://codereview.chromium.org/1652483002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_base.cc:778: void
RenderWidgetHostViewBase::NotifyTextInputStateChanged() {
Does this need to be called when the focused frame changes?

https://codereview.chromium.org/1652483002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/1652483002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_base.h:130: // Notifies
the top level page RWHVBase about a change in text input state in
It suffices to say RenderWidgetHostView instead of RWHVBase.

https://codereview.chromium.org/1652483002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_base.h:249: // Updates the
state of the input method attached to the view.
These comments need to be expanded. It appears that each RenderWidgetHostView
now keeps its own state (which is attached to RenderWidgetHostViewBase), and we
reference the 'current' text input state as being the state attached to the
focused frame. This doesn't really come across from the comments as written.

https://codereview.chromium.org/1652483002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/1652483002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_mac.mm:1031:
!GetCurrentTextInputState()->can_compose_inline_);
Wrong method name? (Related, you might want to run a try job)

[EhsanK, 2016-02-02 20:02:29]

Patchset #2 (id:20001) has been deleted

[EhsanK, 2016-02-11 21:02:36]

Description was changed from

==========
Implementing TextInputStateChanged for OOPIF.

Currently, the TextInputStateChanged method is not implemented in
RenderWidgetHostViewChildFrame. Conequently, the IME
will not work until this feature is fixed. Furthermore, for non-OOPIF-based
<webview>, the method is implemented in
RenderWidgetHostViewGuest but current implementation is palindromically racy
(crbug.com/546645).

This CL implements the TextInputStateChanged function through refactoring most
of the code to the top-level RenderWidgetHostViewBase. The implementation is
based on FrameTree::FocusedFrame() and hecne does not apply to non-OOPIF
<webview> (RenderWidgetHostViewGuest). Therefore, temporarily, a separate code
path is
added to support the feauture for current implementation of <webview> based on
BrowserPlugin and will be removed later.

BUG=546645,578168
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Implementing TextInputStateChanged for OOPIF.

Currently, the TextInputStateChanged method is not implemented in
RenderWidgetHostViewChildFrame. Conequently, the IME
will not work until this feature is fixed. 

This CL implements the text input state tracking through refactoring most of the
TextInputStateChanged code to the top-level RenderWidgetHostViewBase. The
implementation is
based on FrameTree::GetFocusedFrame() and hence does not apply to non-OOPIF
<webview> (RenderWidgetHostViewGuest). 

BUG=578168

CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[EhsanK, 2016-02-11 21:03:22]

Patchset #3 (id:60001) has been deleted

[EhsanK, 2016-02-11 21:11:13]

Patchset #2 (id:40001) has been deleted

[EhsanK, 2016-02-12 15:46:59]

ekaramad@chromium.org changed reviewers:
+ creis@chromium.org

[EhsanK, 2016-02-12 15:47:00]

PTAL. 

I removed all the Non-OOPIF-<webview> related code and hence, the removal of
bug=546645 from the issue description.

Adding creis@ for content/ reviews.

https://codereview.chromium.org/1652483002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/1652483002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_widget_host_view_child_frame.h:88: bool
ShouldObtainTextInputStateFromSubFrameViews() const override;
On 2016/02/01 20:12:43, kenrb wrote:
> nit: SubFrame should be ChildFrame, to keep terminology consistent.

Done.

https://codereview.chromium.org/1652483002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/1652483002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_widget_host_view_guest.cc:369: //
InterstitialPages are not WebContents so we cannot interceptG
On 2016/02/01 20:12:43, kenrb wrote:
> nit: typo

Done.

https://codereview.chromium.org/1652483002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/DEPS (right):

https://codereview.chromium.org/1652483002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/DEPS:49: "render_widget_host_view_aura\.cc":[
On 2016/02/01 20:12:43, kenrb wrote:
> The space character shouldn't be removed.

Done.

https://codereview.chromium.org/1652483002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/DEPS:55: ],
On 2016/02/01 20:12:43, kenrb wrote:
> I see that this is mostly replicating the exception above it (originally added
> by jam@ when he added GetFocusedFrame()), since you are moving code from
> RWHVAura to RWHVBase, but this list is getting long. I wonder if Charlie or
> Nasko has an idea how to untangle this. It seems difficult since we don't even
> have access to the WebContents.

Based on this comment, I decided to add the required function to the
RenderWidgetHostDelegate, and, moved the GetFocusedFrame back to RWHVAura to be
dealt with later.

https://codereview.chromium.org/1652483002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_base.cc (right):

https://codereview.chromium.org/1652483002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_base.cc:741: if
(!ShouldObtainTextInputStateFromSubFrameViews())
On 2016/02/01 20:12:43, kenrb wrote:
> Why is this needed? Can't you just compare GetFocusedFrame()->GetView() to
> |this| and recurse on GetFocusedFrame() if they are different? Or is this
method
> only required because of the BrowserPlugin alternate code path?

Good point! I removed this part and all other code related to Non-OOPIF
<webview> which were introduced to fix crbug.com/546645. I will deal with that
later.

I also renamed the NotifyText... to UpdateText..

https://codereview.chromium.org/1652483002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_base.cc:778: void
RenderWidgetHostViewBase::NotifyTextInputStateChanged() {
On 2016/02/01 20:12:43, kenrb wrote:
> Does this need to be called when the focused frame changes?
This is called whenever after a ViewHostMsg_TextInputStateChanged, which is
either:
1- An input on the same frame,
2- An input on a child frame, which in turn called this (top-level) RWHV.

The text input IPC is tied with focus, I believe it is sent just after the focus
related IPC. However, if the input fields are within the same frame, then there
is no focus IPC.

On a related note, there was a way to not use this method and keep overriding
the TextInputStateChanged for all RWHV*. However, in such a case we would not be
informed of a state change in a child frame and hence, everytime there was a
need to the state (e.g., at the Aura) we had to search for the focused frame.
Now by notifying and caching the last state we only search for focused frame
when there is a change.

https://codereview.chromium.org/1652483002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/1652483002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_base.h:130: // Notifies
the top level page RWHVBase about a change in text input state in
On 2016/02/01 20:12:43, kenrb wrote:
> It suffices to say RenderWidgetHostView instead of RWHVBase.

Done.

https://codereview.chromium.org/1652483002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_base.h:249: // Updates the
state of the input method attached to the view.
On 2016/02/01 20:12:43, kenrb wrote:
> These comments need to be expanded. It appears that each RenderWidgetHostView
> now keeps its own state (which is attached to RenderWidgetHostViewBase), and
we
> reference the 'current' text input state as being the state attached to the
> focused frame. This doesn't really come across from the comments as written.

Done.

https://codereview.chromium.org/1652483002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/1652483002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_mac.mm:1031:
!GetCurrentTextInputState()->can_compose_inline_);
On 2016/02/01 20:12:43, kenrb wrote:
> Wrong method name? (Related, you might want to run a try job)

Done. Sorry for this. I did run this on all _rel_ try bots.

[Charlie Reis, 2016-02-17 06:22:14]

Ken knows this much better than I do, so I'll defer to him and just provide an
owner's review.  (Ken, do you know it well enough or should we get an IME person
to look as well?)

A few thoughts in the meantime.

https://codereview.chromium.org/1652483002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/1652483002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:73: using
TextInputState = ViewHostMsg_TextInputState_Params;
In cases that params are used widely, we tend to go the other way around: define
a struct and then wrap it in the IPC messages file with
IPC_STRUCT_TRAITS_BEGIN/END.  Is that a big change in this case?

https://codereview.chromium.org/1652483002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:131: //
text_input_state_ if the view is for a main frame.
I'm confused.  Why would these two differ for the main frame?  (Maybe this needs
a clearer comment for what it represents, if it's not something specific to this
view.)

https://codereview.chromium.org/1652483002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:190: // Informs
that the  DOM node has changed.
This doesn't seem like a good change to the comment, given that the method is
named FocusedNodeChanged.  Is the actual behavior not about the focused node
anymore?

https://codereview.chromium.org/1652483002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:246: // Also, this
change does not necessarily reflect the current state the input
Typo?  (the current state the input state)

https://codereview.chromium.org/1652483002/diff/80001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1652483002/diff/80001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:5323: LOG(INFO) << "Sending a
mouse click to top level page's <input>.";
nit: We generally avoid new LOG(INFO)s, unless they make a test failure much
easier to diagnose.  Is this needed?

https://codereview.chromium.org/1652483002/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1652483002/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1682: else if
(rfhi->GetRenderViewHost())
nit: No else needed after return.

Also, this looks wrong according to the comment on GetRenderWidgetHost.  That
already returns the RVH's RWH for the main frame (making this branch
unnecessary), but it returns null for subframes that don't own their own widget
(which aren't handled here).

[kenrb, 2016-02-17 23:05:53]

It might be a good idea to have somebody more familiar with IME mechanics have a
look.

https://codereview.chromium.org/1652483002/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1652483002/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1682: else if
(rfhi->GetRenderViewHost())
On 2016/02/17 06:22:14, Charlie Reis wrote:
> nit: No else needed after return.
> 
> Also, this looks wrong according to the comment on GetRenderWidgetHost.  That
> already returns the RVH's RWH for the main frame (making this branch
> unnecessary), but it returns null for subframes that don't own their own
widget
> (which aren't handled here).

I hadn't noticed this, but right. I think everything from line 1679 to the end
of the method can be replaced by
return focused_frame->current_frame_host()->GetView();

[EhsanK, 2016-02-18 05:48:01]

PTAL. Thanks!

https://codereview.chromium.org/1652483002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/1652483002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:73: using
TextInputState = ViewHostMsg_TextInputState_Params;
On 2016/02/17 06:22:14, Charlie Reis wrote:
> In cases that params are used widely, we tend to go the other way around:
define
> a struct and then wrap it in the IPC messages file with
> IPC_STRUCT_TRAITS_BEGIN/END.  Is that a big change in this case?

Done. Hopefully I got it right.

https://codereview.chromium.org/1652483002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:131: //
text_input_state_ if the view is for a main frame.
On 2016/02/17 06:22:14, Charlie Reis (catching up) wrote:
> I'm confused.  Why would these two differ for the main frame?  (Maybe this
needs
> a clearer comment for what it represents, if it's not something specific to
this
> view.)

|text_input_state_| : the text input state corresponding to a text input field
in this frame (not child frames).

|current_text_input_state()|: |text_input_state_| of the focused frame or child
frame.

For the main frame, we might have to obtain the input state from one of the OOP
child frames.

Whenever a change in input state of a child frame occurs, the top level RWHV
receives a NotifyTextInputStateChanged() and then it will look for the focused
frame to cache its value into |cached_text_input_state_|.

I could remove NotifyTextInputStateChanged() and |cached_text_input_state_| to
make the code simpler to read. But the every time we need to find the text input
state, we might have to traverse the whole frame tree to find the focused frame
and ask for its input state.

https://codereview.chromium.org/1652483002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:190: // Informs
that the  DOM node has changed.
On 2016/02/17 06:22:14, Charlie Reis (catching up) wrote:
> This doesn't seem like a good change to the comment, given that the method is
> named FocusedNodeChanged.  Is the actual behavior not about the focused node
> anymore?

Hmmm. I do not have any recollection of changing this. Sorry. Modified by
mistake.

https://codereview.chromium.org/1652483002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:246: // Also, this
change does not necessarily reflect the current state the input
On 2016/02/17 06:22:14, Charlie Reis (catching up) wrote:
> Typo?  (the current state the input state)

Done.

https://codereview.chromium.org/1652483002/diff/80001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1652483002/diff/80001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:5323: LOG(INFO) << "Sending a
mouse click to top level page's <input>.";
On 2016/02/17 06:22:14, Charlie Reis (catching up) wrote:
> nit: We generally avoid new LOG(INFO)s, unless they make a test failure much
> easier to diagnose.  Is this needed?
No. Sorry. It is there because I forgot to remove it in the final patch. Thanks!

https://codereview.chromium.org/1652483002/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1652483002/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1682: else if
(rfhi->GetRenderViewHost())
On 2016/02/17 06:22:14, Charlie Reis (catching up) wrote:
> nit: No else needed after return.
> 
> Also, this looks wrong according to the comment on GetRenderWidgetHost.  That
> already returns the RVH's RWH for the main frame (making this branch
> unnecessary), but it returns null for subframes that don't own their own
widget
> (which aren't handled here).

I guess using RFH::GetView() resolves all the issues.
Done.

https://codereview.chromium.org/1652483002/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1682: else if
(rfhi->GetRenderViewHost())
On 2016/02/17 23:05:53, kenrb wrote:
> On 2016/02/17 06:22:14, Charlie Reis wrote:
> > nit: No else needed after return.
> > 
> > Also, this looks wrong according to the comment on GetRenderWidgetHost. 
That
> > already returns the RVH's RWH for the main frame (making this branch
> > unnecessary), but it returns null for subframes that don't own their own
> widget
> > (which aren't handled here).
> 
> I hadn't noticed this, but right. I think everything from line 1679 to the end
> of the method can be replaced by
> return focused_frame->current_frame_host()->GetView();

Yes this is the correct way. Thanks!

[Charlie Reis, 2016-02-27 00:27:11]

On 2016/02/18 05:48:01, ehsaaan wrote:
> PTAL. Thanks!
> 

Sorry that I didn't get a chance to take another look; this week was busy with a
summit.  I would suggest having an IME owner review (as Ken suggested), and then
possibly getting Nasko to finish up the content/ review if that's done before I
get back.  Thanks for working on this!

[EhsanK, 2016-02-27 03:14:50]

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

[EhsanK, 2016-02-27 03:15:31]

On 2016/02/27 00:27:11, Charlie Reis (OOO til Mar 7) wrote:
> On 2016/02/18 05:48:01, ehsaaan wrote:
> > PTAL. Thanks!
> > 
> 
> Sorry that I didn't get a chance to take another look; this week was busy with
a
> summit.  I would suggest having an IME owner review (as Ken suggested), and
then
> possibly getting Nasko to finish up the content/ review if that's done before
I
> get back.  Thanks for working on this!

Thanks Charlie. Added shuchen@ for IME input and reviews.

[EhsanK, 2016-02-27 03:16:46]

PTAL.

Adding shuchen@ for IME reviews.

Thanks!

[Shu Chen, 2016-03-04 02:30:30]

https://codereview.chromium.org/1652483002/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1652483002/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:1012:
GetInputMethod()->OnTextInputTypeChanged(this);
The original logic notifies InputMethod only when there are state changes, while
this logics doesn't check the state changes. Is that intentional?

[EhsanK, 2016-03-04 09:31:50]

Thanks for the reviews. PTAL.

https://codereview.chromium.org/1652483002/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1652483002/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:1012:
GetInputMethod()->OnTextInputTypeChanged(this);
On 2016/03/04 02:30:29, Shu Chen wrote:
> The original logic notifies InputMethod only when there are state changes,
while
> this logics doesn't check the state changes. Is that intentional?

I actually removed that part due to this comment:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
Which is technically true, but as I see now we only care about certain changes
in Aura and Mac.

I reverted the logic back to how it used to be but the change is in
RenderWidgetHostViewBase::TextInputStateChanged. We now early return if there
are no useful changes and this method never gets called.

[Shu Chen, 2016-03-04 09:57:45]

https://codereview.chromium.org/1652483002/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1652483002/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:1012:
GetInputMethod()->OnTextInputTypeChanged(this);
On 2016/03/04 09:31:49, ehsaaan wrote:
> On 2016/03/04 02:30:29, Shu Chen wrote:
> > The original logic notifies InputMethod only when there are state changes,
> while
> > this logics doesn't check the state changes. Is that intentional?
> 
> I actually removed that part due to this comment:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
> Which is technically true, but as I see now we only care about certain changes
> in Aura and Mac.
> 
> I reverted the logic back to how it used to be but the change is in
> RenderWidgetHostViewBase::TextInputStateChanged. We now early return if there
> are no useful changes and this method never gets called.

The original logic is:
  if (anything changed) {
    call OnTextInputTypeChanged(...);
  }
  if (show_ime_if_needed) {
    call ShowImeIfNeeded();
  }

Now your latest logic is:
  if (anything changed) {
    call OnTextInputTypeChanged(...);
    if (show_ime_if_needed) {
      call ShowImeIfNeeded();
    }
  }

Can you please revert to the original logic? Thanks

[EhsanK, 2016-03-08 04:09:35]

Patchset #7 (id:180001) has been deleted

[EhsanK, 2016-03-08 04:47:28]

Patchset #7 (id:200001) has been deleted

[EhsanK, 2016-03-08 05:18:10]

Patchset #7 (id:220001) has been deleted

[EhsanK, 2016-03-08 05:21:03]

shuchen@: Thanks for the prompt feedback! PTAL.

https://codereview.chromium.org/1652483002/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1652483002/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:1012:
GetInputMethod()->OnTextInputTypeChanged(this);
On 2016/03/04 09:57:45, Shu Chen wrote:
> On 2016/03/04 09:31:49, ehsaaan wrote:
> > On 2016/03/04 02:30:29, Shu Chen wrote:
> > > The original logic notifies InputMethod only when there are state changes,
> > while
> > > this logics doesn't check the state changes. Is that intentional?
> > 
> > I actually removed that part due to this comment:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
> > Which is technically true, but as I see now we only care about certain
changes
> > in Aura and Mac.
> > 
> > I reverted the logic back to how it used to be but the change is in
> > RenderWidgetHostViewBase::TextInputStateChanged. We now early return if
there
> > are no useful changes and this method never gets called.
> 
> The original logic is:
>   if (anything changed) {
>     call OnTextInputTypeChanged(...);
>   }
>   if (show_ime_if_needed) {
>     call ShowImeIfNeeded();
>   }
> 
> Now your latest logic is:
>   if (anything changed) {
>     call OnTextInputTypeChanged(...);
>     if (show_ime_if_needed) {
>       call ShowImeIfNeeded();
>     }
>   }
> 
> Can you please revert to the original logic? Thanks

Thanks for the detailed explanation. I realize the error now. I made the fix
(hopefully correct and readable).

https://codereview.chromium.org/1652483002/diff/240001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1652483002/diff/240001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:1016: if
(focused_view->ShouldProcessTextInputState()) {
This function must only return true, and the code inside will only get called if
the received update in text input state in the corresponding RWHV is new
according to the old logic (RenderWidgetHostViewBase::TextInputStateChanged sets
that).

https://codereview.chromium.org/1652483002/diff/240001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:1021: if
(state->show_ime_if_needed && state->type != ui::TEXT_INPUT_TYPE_NONE)
This one also will be called regardless of whether the state changed has been
deemed as new.

[EhsanK, 2016-03-08 05:21:05]

shuchen@: Thanks for the prompt feedback! PTAL.

[Shu Chen, 2016-03-08 06:17:06]

lgtm

[Charlie Reis, 2016-03-09 00:09:33]

Thanks.  Some thoughts below, and a few nits.

https://codereview.chromium.org/1652483002/diff/240001/content/browser/render...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/1652483002/diff/240001/content/browser/render...
content/browser/renderer_host/render_widget_host_delegate.h:179: // Called when
the widget has sent a compositor proto.  This is used in Btlimp
I noticed the "Btlimp" typo here from an earlier patch set, but it looks like
you landed that in https://codereview.chromium.org/1592573002.  Mind fixing it
here?

https://codereview.chromium.org/1652483002/diff/240001/content/browser/render...
content/browser/renderer_host/render_widget_host_delegate.h:184: // Called when
the visibility of the RenderFrameProxyHost in outter
nit: Same here, with "outter"

https://codereview.chromium.org/1652483002/diff/240001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/1652483002/diff/240001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:139: // root
RenderWidget or the RenderWidget of one of the child frames.
This seems like a confusing method to use correctly.  If I call it on the root
RWHV, then it gives me the current TextInputState for the whole tab.  If I call
it on a child RWHV, it gives me the RWHV's own TextInputState, even if that's
not the current one.

That means all call sites would have to check whether they're calling it on the
root RWHV or not (or somehow verify that they aren't calling it for children). 
That seems quite likely to cause bugs.

Maybe there's another way to structure this?  Maybe the cached value should be
stored on another class (WebContentsImpl?) if it's a tab level concept?  Or this
method could assert or return null if called on a child RWHV?

https://codereview.chromium.org/1652483002/diff/240001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:451: // Returns the
currently focused RWHV.
Maybe it's worth mentioning how this relates to current_text_input_state() and
when people should call one vs the other?

https://codereview.chromium.org/1652483002/diff/240001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:492: // calls to
GetFocusedView() whenever the text input state is required.
Maybe mention whether this is valid on anything but the main RWHV?

https://codereview.chromium.org/1652483002/diff/240001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1652483002/diff/240001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:5553:
base::TimeDelta::FromMicroseconds(500));
Is there any way to avoid this polling?  It's generally not a good strategy to
use in tests.

If we need it, maybe we should use a similar pattern as other cases, like 
SurfaceHitTestReadyNotifier::WaitForSurfaceReady().

https://codereview.chromium.org/1652483002/diff/240001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:5598: ui::TEXT_INPUT_TYPE_NONE);
nit: Reverse the parameters, since the contract is EXPECT_EQ(expected, actual). 
This matters for error messages.

(Same below.)

https://codereview.chromium.org/1652483002/diff/240001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:5599: // Focus the root
(main-frame). This would focus the text input field.
nit: Add blank line before.

https://codereview.chromium.org/1652483002/diff/240001/content/common/text_in...
File content/common/text_input_state.cc (right):

https://codereview.chromium.org/1652483002/diff/240001/content/common/text_in...
content/common/text_input_state.cc:1: // Copyright 2015 The Chromium Authors.
All rights reserved.
nit: 2016

https://codereview.chromium.org/1652483002/diff/240001/content/common/text_in...
File content/common/text_input_state.h (right):

https://codereview.chromium.org/1652483002/diff/240001/content/common/text_in...
content/common/text_input_state.h:1: // Copyright 2015 The Chromium Authors. All
rights reserved.
nit: 2016

https://codereview.chromium.org/1652483002/diff/240001/content/common/text_in...
content/common/text_input_state.h:53: // Whether this change is originated from
none-IME (e.g., Javascript,
non-IME

https://codereview.chromium.org/1652483002/diff/240001/content/common/text_in...
content/common/text_input_state.h:60: #endif
nit: #endif  // CONTENT_COMMON_TEXT_INPUT_STATE_H_

[EhsanK, 2016-03-10 01:34:29]

Patchset #8 (id:260001) has been deleted

[EhsanK, 2016-03-10 05:24:34]

Patchset #8 (id:280001) has been deleted

[EhsanK, 2016-03-10 05:36:50]

Patchset #8 (id:300001) has been deleted

[EhsanK, 2016-03-14 19:57:47]

On 2016/03/09 00:09:33, Charlie Reis (slow til 3-15) wrote:
> Thanks.  Some thoughts below, and a few nits.
> 
>
https://codereview.chromium.org/1652483002/diff/240001/content/browser/render...
> File content/browser/renderer_host/render_widget_host_delegate.h (right):
> 
>
https://codereview.chromium.org/1652483002/diff/240001/content/browser/render...
> content/browser/renderer_host/render_widget_host_delegate.h:179: // Called
when
> the widget has sent a compositor proto.  This is used in Btlimp
> I noticed the "Btlimp" typo here from an earlier patch set, but it looks like
> you landed that in https://codereview.chromium.org/1592573002.  Mind fixing it
> here?
> 
>
https://codereview.chromium.org/1652483002/diff/240001/content/browser/render...
> content/browser/renderer_host/render_widget_host_delegate.h:184: // Called
when
> the visibility of the RenderFrameProxyHost in outter
> nit: Same here, with "outter"
> 
>
https://codereview.chromium.org/1652483002/diff/240001/content/browser/render...
> File content/browser/renderer_host/render_widget_host_view_base.h (right):
> 
>
https://codereview.chromium.org/1652483002/diff/240001/content/browser/render...
> content/browser/renderer_host/render_widget_host_view_base.h:139: // root
> RenderWidget or the RenderWidget of one of the child frames.
> This seems like a confusing method to use correctly.  If I call it on the root
> RWHV, then it gives me the current TextInputState for the whole tab.  If I
call
> it on a child RWHV, it gives me the RWHV's own TextInputState, even if that's
> not the current one.
> 
> That means all call sites would have to check whether they're calling it on
the
> root RWHV or not (or somehow verify that they aren't calling it for children).

> That seems quite likely to cause bugs.
> 
> Maybe there's another way to structure this?  Maybe the cached value should be
> stored on another class (WebContentsImpl?) if it's a tab level concept?  Or
this
> method could assert or return null if called on a child RWHV?
> 
>
https://codereview.chromium.org/1652483002/diff/240001/content/browser/render...
> content/browser/renderer_host/render_widget_host_view_base.h:451: // Returns
the
> currently focused RWHV.
> Maybe it's worth mentioning how this relates to current_text_input_state() and
> when people should call one vs the other?
> 
>
https://codereview.chromium.org/1652483002/diff/240001/content/browser/render...
> content/browser/renderer_host/render_widget_host_view_base.h:492: // calls to
> GetFocusedView() whenever the text input state is required.
> Maybe mention whether this is valid on anything but the main RWHV?
> 
>
https://codereview.chromium.org/1652483002/diff/240001/content/browser/site_p...
> File content/browser/site_per_process_browsertest.cc (right):
> 
>
https://codereview.chromium.org/1652483002/diff/240001/content/browser/site_p...
> content/browser/site_per_process_browsertest.cc:5553:
> base::TimeDelta::FromMicroseconds(500));
> Is there any way to avoid this polling?  It's generally not a good strategy to
> use in tests.
> 
> If we need it, maybe we should use a similar pattern as other cases, like 
> SurfaceHitTestReadyNotifier::WaitForSurfaceReady().
> 
>
https://codereview.chromium.org/1652483002/diff/240001/content/browser/site_p...
> content/browser/site_per_process_browsertest.cc:5598:
ui::TEXT_INPUT_TYPE_NONE);
> nit: Reverse the parameters, since the contract is EXPECT_EQ(expected,
actual). 
> This matters for error messages.
> 
> (Same below.)
> 
>
https://codereview.chromium.org/1652483002/diff/240001/content/browser/site_p...
> content/browser/site_per_process_browsertest.cc:5599: // Focus the root
> (main-frame). This would focus the text input field.
> nit: Add blank line before.
> 
>
https://codereview.chromium.org/1652483002/diff/240001/content/common/text_in...
> File content/common/text_input_state.cc (right):
> 
>
https://codereview.chromium.org/1652483002/diff/240001/content/common/text_in...
> content/common/text_input_state.cc:1: // Copyright 2015 The Chromium Authors.
> All rights reserved.
> nit: 2016
> 
>
https://codereview.chromium.org/1652483002/diff/240001/content/common/text_in...
> File content/common/text_input_state.h (right):
> 
>
https://codereview.chromium.org/1652483002/diff/240001/content/common/text_in...
> content/common/text_input_state.h:1: // Copyright 2015 The Chromium Authors.
All
> rights reserved.
> nit: 2016
> 
>
https://codereview.chromium.org/1652483002/diff/240001/content/common/text_in...
> content/common/text_input_state.h:53: // Whether this change is originated
from
> none-IME (e.g., Javascript,
> non-IME
> 
>
https://codereview.chromium.org/1652483002/diff/240001/content/common/text_in...
> content/common/text_input_state.h:60: #endif
> nit: #endif  // CONTENT_COMMON_TEXT_INPUT_STATE_H_

creis@:

Thanks again for the reviews. I moved most of the logic for 'tracking' the state
to the 
RenderWidgetHostDelegate.

Also, this patch does not use GetFocusedFrame() logic anymore as now the RWHVs
call
the RWHDelegate directly for text input state access, both setting and getting.
If this change is
approved, I will go ahead and fix the CL description above.

Thanks!

[EhsanK, 2016-03-14 19:58:08]

PTAL.

https://codereview.chromium.org/1652483002/diff/240001/content/browser/render...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/1652483002/diff/240001/content/browser/render...
content/browser/renderer_host/render_widget_host_delegate.h:179: // Called when
the widget has sent a compositor proto.  This is used in Btlimp
On 2016/03/09 00:09:33, Charlie Reis wrote:
> I noticed the "Btlimp" typo here from an earlier patch set, but it looks like
> you landed that in https://codereview.chromium.org/1592573002.  Mind fixing it
> here?

Oops. Sorry. Thanks pointing this out! Done.

https://codereview.chromium.org/1652483002/diff/240001/content/browser/render...
content/browser/renderer_host/render_widget_host_delegate.h:184: // Called when
the visibility of the RenderFrameProxyHost in outter
On 2016/03/09 00:09:33, Charlie Reis wrote:
> nit: Same here, with "outter"

Done.

https://codereview.chromium.org/1652483002/diff/240001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/1652483002/diff/240001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:139: // root
RenderWidget or the RenderWidget of one of the child frames.
On 2016/03/09 00:09:33, Charlie Reis wrote:
> This seems like a confusing method to use correctly.  If I call it on the root
> RWHV, then it gives me the current TextInputState for the whole tab.  If I
call
> it on a child RWHV, it gives me the RWHV's own TextInputState, even if that's
> not the current one.
> 
> That means all call sites would have to check whether they're calling it on
the
> root RWHV or not (or somehow verify that they aren't calling it for children).

> That seems quite likely to cause bugs.
> 
> Maybe there's another way to structure this?  Maybe the cached value should be
> stored on another class (WebContentsImpl?) if it's a tab level concept?  Or
this
> method could assert or return null if called on a child RWHV?

Thanks for the suggestion. Your point is quite valid. I moved it up to the
WebContentsImpl which obviated the need to have
RenderWidgetHostViewBase::GetFocusedView(). Now there will be a
WebContentsImpl::GetTextInputState() which will return the cached value.

https://codereview.chromium.org/1652483002/diff/240001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:451: // Returns the
currently focused RWHV.
On 2016/03/09 00:09:33, Charlie Reis wrote:
> Maybe it's worth mentioning how this relates to current_text_input_state() and
> when people should call one vs the other?

It is removed now since the whole logic of getting the state from a focused
frame has moved up to the WebContentsImpl. I have added some comments there.
Done.

https://codereview.chromium.org/1652483002/diff/240001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:492: // calls to
GetFocusedView() whenever the text input state is required.
On 2016/03/09 00:09:33, Charlie Reis wrote:
> Maybe mention whether this is valid on anything but the main RWHV?

I guess removing it will make it simpler.

https://codereview.chromium.org/1652483002/diff/240001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1652483002/diff/240001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:5553:
base::TimeDelta::FromMicroseconds(500));
On 2016/03/09 00:09:33, Charlie Reis wrote:
> Is there any way to avoid this polling?  It's generally not a good strategy to
> use in tests.
> 
> If we need it, maybe we should use a similar pattern as other cases, like 
> SurfaceHitTestReadyNotifier::WaitForSurfaceReady().

Done.

https://codereview.chromium.org/1652483002/diff/240001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:5598: ui::TEXT_INPUT_TYPE_NONE);
On 2016/03/09 00:09:33, Charlie Reis wrote:
> nit: Reverse the parameters, since the contract is EXPECT_EQ(expected,
actual). 
> This matters for error messages.
> 
> (Same below.)

Done.

https://codereview.chromium.org/1652483002/diff/240001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:5599: // Focus the root
(main-frame). This would focus the text input field.
On 2016/03/09 00:09:33, Charlie Reis wrote:
> nit: Add blank line before.

Done.

https://codereview.chromium.org/1652483002/diff/240001/content/common/text_in...
File content/common/text_input_state.cc (right):

https://codereview.chromium.org/1652483002/diff/240001/content/common/text_in...
content/common/text_input_state.cc:1: // Copyright 2015 The Chromium Authors.
All rights reserved.
On 2016/03/09 00:09:33, Charlie Reis wrote:
> nit: 2016

Done.

https://codereview.chromium.org/1652483002/diff/240001/content/common/text_in...
File content/common/text_input_state.h (right):

https://codereview.chromium.org/1652483002/diff/240001/content/common/text_in...
content/common/text_input_state.h:1: // Copyright 2015 The Chromium Authors. All
rights reserved.
On 2016/03/09 00:09:33, Charlie Reis wrote:
> nit: 2016

Done.

https://codereview.chromium.org/1652483002/diff/240001/content/common/text_in...
content/common/text_input_state.h:53: // Whether this change is originated from
none-IME (e.g., Javascript,
On 2016/03/09 00:09:33, Charlie Reis wrote:
> non-IME

Done.

[Charlie Reis, 2016-03-15 18:32:01]

Thanks-- a few additional questions and nits below.

https://codereview.chromium.org/1652483002/diff/400001/content/browser/frame_...
File content/browser/frame_host/interstitial_page_impl.cc (right):

https://codereview.chromium.org/1652483002/diff/400001/content/browser/frame_...
content/browser/frame_host/interstitial_page_impl.cc:541: if (web_contents_)
Style nit: Body is more than one line, so this needs braces.  Same below.

https://codereview.chromium.org/1652483002/diff/400001/content/browser/frame_...
content/browser/frame_host/interstitial_page_impl.cc:544: return
TextInputState();
Is this a bug?  Looks like we assign a value to text_input_state_ but never use
it.  (What's the reason for caching it in InterstitialPageImpl?)

https://codereview.chromium.org/1652483002/diff/400001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/1652483002/diff/400001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:224:
TextInputStateChanged(TextInputState());
Why are we passing in an empty one rather than text_input_state()?

https://codereview.chromium.org/1652483002/diff/400001/content/browser/render...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/1652483002/diff/400001/content/browser/render...
content/browser/renderer_host/render_widget_host_delegate.h:139: // obtained
from the focused  RWHV.
nit: Remove extra space.

https://codereview.chromium.org/1652483002/diff/400001/content/browser/render...
content/browser/renderer_host/render_widget_host_delegate.h:143: // in the text
input state. The IPC could or could not have lead to a change
nit: s/could/might/g

https://codereview.chromium.org/1652483002/diff/400001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.cc (right):

https://codereview.chromium.org/1652483002/diff/400001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.cc:389:
text_input_state_(new TextInputState),
Minor nit: new TextInputState()

https://codereview.chromium.org/1652483002/diff/400001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.cc:760: bool
text_input_state_changed) {}
nit: You don't need this in the .cc file if it's empty.  Just put the braces in
the .h file.

https://codereview.chromium.org/1652483002/diff/400001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/1652483002/diff/400001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:131: const
TextInputState* text_input_state() const {
Let's include a comment to give context about when to use this vs the one on
WebContents.

https://codereview.chromium.org/1652483002/diff/400001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:248: // input since
the text input  could be due to a change in the focused
nit: Remove extra space.

https://codereview.chromium.org/1652483002/diff/400001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:256: //
RenderWidgetHostViewChildFrame.
I'm unclear on the last sentence.  Would it be clearer to say that it should
only be called on the main frame's RWHV?

https://codereview.chromium.org/1652483002/diff/400001/content/browser/site_p...
File content/browser/site_per_process_browsertest.h (right):

https://codereview.chromium.org/1652483002/diff/400001/content/browser/site_p...
content/browser/site_per_process_browsertest.h:30: WebContentsImpl*
GetWebContents();
nit: web_contents()

Thanks for adding this-- I like removing all those casts in the tests.

https://codereview.chromium.org/1652483002/diff/400001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1652483002/diff/400001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:2254:
GetOuterWebContents()->UpdateTextInputState(rwhv, text_input_state_changed);
Does this need an early return, or do we need to also do the things below? 
(Clarify in the comment if so, since it looks like a potential bug.)

https://codereview.chromium.org/1652483002/diff/400001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:2255: // If this RWHV has a
text input type of NONE, then there is nothing to
nit: Blank line before.

https://codereview.chromium.org/1652483002/diff/400001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1652483002/diff/400001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:1346: RenderWidgetHostViewBase*
view_with_active_text_input_;
This looks like it could get stale if the RWHV went away (e.g., after a
cross-process navigation, or maybe a process crash?).  That would lead to a UaF.
 Can we find a safe way to clear it if necessary?

[EhsanK, 2016-03-15 23:51:18]

Thanks for the reviews. PTAL.

https://codereview.chromium.org/1652483002/diff/400001/content/browser/frame_...
File content/browser/frame_host/interstitial_page_impl.cc (right):

https://codereview.chromium.org/1652483002/diff/400001/content/browser/frame_...
content/browser/frame_host/interstitial_page_impl.cc:541: if (web_contents_)
On 2016/03/15 18:32:00, Charlie Reis (slow til 3-15) wrote:
> Style nit: Body is more than one line, so this needs braces.  Same below.

Fit in one line after 'git cl format'. Thanks!

https://codereview.chromium.org/1652483002/diff/400001/content/browser/frame_...
content/browser/frame_host/interstitial_page_impl.cc:544: return
TextInputState();
On 2016/03/15 18:32:00, Charlie Reis (slow til 3-15) wrote:
> Is this a bug?  Looks like we assign a value to text_input_state_ but never
use
> it.  (What's the reason for caching it in InterstitialPageImpl?)

Definitely a bug and surprised why tests didn't fail. It was left over when I
patched and uploaded from a draft CL
(https://codereview.chromium.org/1663013005/).

Thanks for noticing this.

https://codereview.chromium.org/1652483002/diff/400001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/1652483002/diff/400001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:224:
TextInputStateChanged(TextInputState());
On 2016/03/15 18:32:00, Charlie Reis (slow til 3-15) wrote:
> Why are we passing in an empty one rather than text_input_state()?

This line will reset the WebContents knowledge of text input state, if it had
come from this RWHV. The WebContents currently holds a pointer the RWHV as well
as the last reported value of TextInputState. If this RWHV goes out of
existence, those information will be invalid and it will cause errors in IME
workings (also cursor stuff).

This line will hit when the renderer crashes.

However, as I see it now, it might be best to separate this task into another
method. I have done that in the new patch. Please let me know if it makes sense.

This new method is called NotifyHostDelegateAboutShutdown() and should be called
before host_->SetView(NULL). I think this method should also be called in
top-level RWHV, otherwise, the same issue might arise in cases with nested
WebContents.

https://codereview.chromium.org/1652483002/diff/400001/content/browser/render...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/1652483002/diff/400001/content/browser/render...
content/browser/renderer_host/render_widget_host_delegate.h:139: // obtained
from the focused  RWHV.
On 2016/03/15 18:32:00, Charlie Reis (slow til 3-15) wrote:
> nit: Remove extra space.

Done. Thanks!

https://codereview.chromium.org/1652483002/diff/400001/content/browser/render...
content/browser/renderer_host/render_widget_host_delegate.h:143: // in the text
input state. The IPC could or could not have lead to a change
On 2016/03/15 18:32:00, Charlie Reis (slow til 3-15) wrote:
> nit: s/could/might/g

Done. Thanks!

https://codereview.chromium.org/1652483002/diff/400001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.cc (right):

https://codereview.chromium.org/1652483002/diff/400001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.cc:389:
text_input_state_(new TextInputState),
On 2016/03/15 18:32:00, Charlie Reis (slow til 3-15) wrote:
> Minor nit: new TextInputState()

Done.

https://codereview.chromium.org/1652483002/diff/400001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.cc:760: bool
text_input_state_changed) {}
On 2016/03/15 18:32:00, Charlie Reis (slow til 3-15) wrote:
> nit: You don't need this in the .cc file if it's empty.  Just put the braces
in
> the .h file.

Done.

https://codereview.chromium.org/1652483002/diff/400001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/1652483002/diff/400001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:131: const
TextInputState* text_input_state() const {
On 2016/03/15 18:32:00, Charlie Reis (slow til 3-15) wrote:
> Let's include a comment to give context about when to use this vs the one on
> WebContents.

Done.

https://codereview.chromium.org/1652483002/diff/400001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:248: // input since
the text input  could be due to a change in the focused
On 2016/03/15 18:32:00, Charlie Reis (slow til 3-15) wrote:
> nit: Remove extra space.

Done. Thanks!

https://codereview.chromium.org/1652483002/diff/400001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:256: //
RenderWidgetHostViewChildFrame.
On 2016/03/15 18:32:00, Charlie Reis (slow til 3-15) wrote:
> I'm unclear on the last sentence.  Would it be clearer to say that it should
> only be called on the main frame's RWHV?

Yes. Revised. Thanks!

https://codereview.chromium.org/1652483002/diff/400001/content/browser/site_p...
File content/browser/site_per_process_browsertest.h (right):

https://codereview.chromium.org/1652483002/diff/400001/content/browser/site_p...
content/browser/site_per_process_browsertest.h:30: WebContentsImpl*
GetWebContents();
On 2016/03/15 18:32:00, Charlie Reis (slow til 3-15) wrote:
> nit: web_contents()
> 
> Thanks for adding this-- I like removing all those casts in the tests.

Thanks! Done.

https://codereview.chromium.org/1652483002/diff/400001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1652483002/diff/400001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:2254:
GetOuterWebContents()->UpdateTextInputState(rwhv, text_input_state_changed);
On 2016/03/15 18:32:01, Charlie Reis (slow til 3-15) wrote:
> Does this need an early return, or do we need to also do the things below? 
> (Clarify in the comment if so, since it looks like a potential bug.)

This too is definitely a bug. I messed this (and the earlier one on
interstitial_page_impl.cc) up during patch and upload. Sorry again.

https://codereview.chromium.org/1652483002/diff/400001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:2255: // If this RWHV has a
text input type of NONE, then there is nothing to
On 2016/03/15 18:32:01, Charlie Reis (slow til 3-15) wrote:
> nit: Blank line before.

Done.

https://codereview.chromium.org/1652483002/diff/400001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1652483002/diff/400001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:1346: RenderWidgetHostViewBase*
view_with_active_text_input_;
On 2016/03/15 18:32:01, Charlie Reis (slow til 3-15) wrote:
> This looks like it could get stale if the RWHV went away (e.g., after a
> cross-process navigation, or maybe a process crash?).  That would lead to a
UaF.
>  Can we find a safe way to clear it if necessary?

I added some notes above under your comment on
'render_widget_host_view_child_frame.cc'. There by calling
TextInputStateChanged(TextInputState()) in the destruction path for
RenderWidgetHostViewChildFrame I was trying to address this issue. Two notes:

1- If the destruction path is normal, i.e., by detaching the frame, then the
frame should lose focus and eventually the TextInputStateChanged called
organically. 

2- The line added in RWHVCF::Destroy was to address the crash scenario. It will
get called as long as RenderProcessGone is called. I tested this by manually
crashing the process and this line gets hit.

Is there a third (or fourth) scenario not reflected here? Please let me know.
Also, as explained above, I think it is best to have a method do this clean up
rather than the ambiguous line above. We might need to repeat the same process
for inner and outer WebContents since based on this design, the inner
WebContents reports/obtains the state from outer WebContents.

https://codereview.chromium.org/1652483002/diff/440001/content/browser/site_p...
File content/browser/site_per_process_browsertest.h (right):

https://codereview.chromium.org/1652483002/diff/440001/content/browser/site_p...
content/browser/site_per_process_browsertest.h:12: #include
"content/shell/browser/shell.h"
This was necessary for web_contents() to be defined here. If this is fine, I
will go ahead and remove the #include to this and web_contents_impl.h in the cc
file.

[Charlie Reis, 2016-03-16 16:55:13]

Thanks-- a few followup questions on the clearing logic.

https://codereview.chromium.org/1652483002/diff/400001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/1652483002/diff/400001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:224:
TextInputStateChanged(TextInputState());
On 2016/03/15 23:51:17, ehsaaan wrote:
> On 2016/03/15 18:32:00, Charlie Reis (slow til 3-15) wrote:
> > Why are we passing in an empty one rather than text_input_state()?
> 
> This line will reset the WebContents knowledge of text input state, if it had
> come from this RWHV. The WebContents currently holds a pointer the RWHV as
well
> as the last reported value of TextInputState. If this RWHV goes out of
> existence, those information will be invalid and it will cause errors in IME
> workings (also cursor stuff).
> 
> This line will hit when the renderer crashes.
> 
> However, as I see it now, it might be best to separate this task into another
> method. I have done that in the new patch. Please let me know if it makes
sense.
> 
> This new method is called NotifyHostDelegateAboutShutdown() and should be
called
> before host_->SetView(NULL). I think this method should also be called in
> top-level RWHV, otherwise, the same issue might arise in cases with nested
> WebContents.

I see-- I didn't notice that this was during Destroy before.  Yes, the new
approach makes sense.

https://codereview.chromium.org/1652483002/diff/400001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1652483002/diff/400001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:1346: RenderWidgetHostViewBase*
view_with_active_text_input_;
On 2016/03/15 23:51:18, ehsaaan wrote:
> On 2016/03/15 18:32:01, Charlie Reis (slow til 3-15) wrote:
> > This looks like it could get stale if the RWHV went away (e.g., after a
> > cross-process navigation, or maybe a process crash?).  That would lead to a
> UaF.
> >  Can we find a safe way to clear it if necessary?
> 
> I added some notes above under your comment on
> 'render_widget_host_view_child_frame.cc'. There by calling
> TextInputStateChanged(TextInputState()) in the destruction path for
> RenderWidgetHostViewChildFrame I was trying to address this issue. Two notes:
> 
> 1- If the destruction path is normal, i.e., by detaching the frame, then the
> frame should lose focus and eventually the TextInputStateChanged called
> organically. 

Makes sense, though "eventually" concerns me.  Is there a period of time where
the pointer will be stale, or do we clear it at the same time that the frame is
detached?  Let's confirm.


> 2- The line added in RWHVCF::Destroy was to address the crash scenario. It
will
> get called as long as RenderProcessGone is called. I tested this by manually
> crashing the process and this line gets hit.

Great.  Yes, I missed the Destroy behavior before.  That's what I was looking
for.

> Is there a third (or fourth) scenario not reflected here? Please let me know.

I'm curious about cross-process main frame navigations.  Suppose we navigate
from a.com to b.com in the omnibox and switch processes.  The RWHV will change,
and I'm not sure if we'll get an update to this member or not.  It's not a
RWHVCF, so I don't see anything that will clear the pointer to the old RWVH
here.


> Also, as explained above, I think it is best to have a method do this clean up
> rather than the ambiguous line above. We might need to repeat the same process
> for inner and outer WebContents since based on this design, the inner
> WebContents reports/obtains the state from outer WebContents.

Given this inner/outer case and cross-process main frame navigations, I'm
wondering if we need to add the clearing logic to all RWHVs, rather than just
RWHVCFs?

https://codereview.chromium.org/1652483002/diff/440001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.cc (right):

https://codereview.chromium.org/1652483002/diff/440001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.cc:422:
text_input_state_.reset(new TextInputState);
Same minor nit: new TextInputState()

https://codereview.chromium.org/1652483002/diff/440001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/1652483002/diff/440001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:259: //
WebContentsImp if the RenderWidgetHostView is a top-level view. The
nit: WebContentsImpl

https://codereview.chromium.org/1652483002/diff/440001/content/browser/site_p...
File content/browser/site_per_process_browsertest.h (right):

https://codereview.chromium.org/1652483002/diff/440001/content/browser/site_p...
content/browser/site_per_process_browsertest.h:12: #include
"content/shell/browser/shell.h"
On 2016/03/15 23:51:18, ehsaaan wrote:
> This was necessary for web_contents() to be defined here. If this is fine, I
> will go ahead and remove the #include to this and web_contents_impl.h in the
cc
> file.

Yep, that's fine.

https://codereview.chromium.org/1652483002/diff/440001/content/browser/site_p...
content/browser/site_per_process_browsertest.h:20: class WebContentsImpl;
nit: Don't need these 2 lines anymore.

[EhsanK, 2016-03-30 20:46:01]

ekaramad@chromium.org changed reviewers:
+ lazyboy@chromium.org

[EhsanK, 2016-03-30 20:46:04]

PTAL.

Adding lazyboy@ for the web_view_browsertests.cc permissions.

https://codereview.chromium.org/1652483002/diff/400001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/1652483002/diff/400001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:224:
TextInputStateChanged(TextInputState());
On 2016/03/16 16:55:13, Charlie Reis wrote:
> On 2016/03/15 23:51:17, ehsaaan wrote:
> > On 2016/03/15 18:32:00, Charlie Reis (slow til 3-15) wrote:
> > > Why are we passing in an empty one rather than text_input_state()?
> > 
> > This line will reset the WebContents knowledge of text input state, if it
had
> > come from this RWHV. The WebContents currently holds a pointer the RWHV as
> well
> > as the last reported value of TextInputState. If this RWHV goes out of
> > existence, those information will be invalid and it will cause errors in IME
> > workings (also cursor stuff).
> > 
> > This line will hit when the renderer crashes.
> > 
> > However, as I see it now, it might be best to separate this task into
another
> > method. I have done that in the new patch. Please let me know if it makes
> sense.
> > 
> > This new method is called NotifyHostDelegateAboutShutdown() and should be
> called
> > before host_->SetView(NULL). I think this method should also be called in
> > top-level RWHV, otherwise, the same issue might arise in cases with nested
> > WebContents.
> 
> I see-- I didn't notice that this was during Destroy before.  Yes, the new
> approach makes sense.

Done.

https://codereview.chromium.org/1652483002/diff/400001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1652483002/diff/400001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:1346: RenderWidgetHostViewBase*
view_with_active_text_input_;
> Makes sense, though "eventually" concerns me.  Is there a period of time where
> the pointer will be stale, or do we clear it at the same time that the frame
is
> detached?  Let's confirm.

I tested the following:

  - Detach: An IPC comes and sets the input to NONE (tested for RWHVCF).

  - Crash: NotifyHostDelegateAboutShutdown() fixes the issue. There is one new
test.

> I'm curious about cross-process main frame navigations.  Suppose we navigate
> from a.com to b.com in the omnibox and switch processes.  The RWHV will
change,
> and I'm not sure if we'll get an update to this member or not.  It's not a
> RWHVCF, so I don't see anything that will clear the pointer to the old RWVH

When navigation happens, RWHV is destroyed and it resets the input state.
> here.

https://codereview.chromium.org/1652483002/diff/440001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.cc (right):

https://codereview.chromium.org/1652483002/diff/440001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.cc:422:
text_input_state_.reset(new TextInputState);
On 2016/03/16 16:55:13, Charlie Reis wrote:
> Same minor nit: new TextInputState()

Thanks! Done.

https://codereview.chromium.org/1652483002/diff/440001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/1652483002/diff/440001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:259: //
WebContentsImp if the RenderWidgetHostView is a top-level view. The
On 2016/03/16 16:55:13, Charlie Reis wrote:
> nit: WebContentsImpl

Done.

https://codereview.chromium.org/1652483002/diff/440001/content/browser/site_p...
File content/browser/site_per_process_browsertest.h (right):

https://codereview.chromium.org/1652483002/diff/440001/content/browser/site_p...
content/browser/site_per_process_browsertest.h:20: class WebContentsImpl;
On 2016/03/16 16:55:13, Charlie Reis wrote:
> nit: Don't need these 2 lines anymore.

Done.

https://codereview.chromium.org/1652483002/diff/480001/content/DEPS
File content/DEPS (right):

https://codereview.chromium.org/1652483002/diff/480001/content/DEPS#newcode118
content/DEPS:118: ".*_browsertest[a-z_]*\.(cc|h)": [
I had to add this so that I can call shell->web_contents() in the
site_per_process_browsertest.h file. If this is bad, I guess I will have to
either undo the refactoring in the test file (WebContentsImpl* cast replaced by
web_contents()) or change the method name back to GetWebContents() and define it
in the cc file.

[Charlie Reis, 2016-03-30 21:52:47]

Thanks for getting back to this!  It seems fine apart from the RWHV pointer on
WebContents, which I'm still nervous about.  There's enough chances for that to
become a security bug that I'd like to avoid it if we can-- one idea below.

https://codereview.chromium.org/1652483002/diff/400001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1652483002/diff/400001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:1346: RenderWidgetHostViewBase*
view_with_active_text_input_;
On 2016/03/30 20:46:03, ehsaaan wrote:
> > Makes sense, though "eventually" concerns me.  Is there a period of time
where
> > the pointer will be stale, or do we clear it at the same time that the frame
> is
> > detached?  Let's confirm.
> 
> I tested the following:
> 
>   - Detach: An IPC comes and sets the input to NONE (tested for RWHVCF).
> 
>   - Crash: NotifyHostDelegateAboutShutdown() fixes the issue. There is one new
> test.
> 
> > I'm curious about cross-process main frame navigations.  Suppose we navigate
> > from a.com to b.com in the omnibox and switch processes.  The RWHV will
> change,
> > and I'm not sure if we'll get an update to this member or not.  It's not a
> > RWHVCF, so I don't see anything that will clear the pointer to the old RWVH
> 
> When navigation happens, RWHV is destroyed and it resets the input state.

What code makes this happen?  I can't follow it by reading the code, given how
complex it is.

https://codereview.chromium.org/1652483002/diff/480001/content/DEPS
File content/DEPS (right):

https://codereview.chromium.org/1652483002/diff/480001/content/DEPS#newcode118
content/DEPS:118: ".*_browsertest[a-z_]*\.(cc|h)": [
On 2016/03/30 20:46:04, ehsaaan wrote:
> I had to add this so that I can call shell->web_contents() in the
> site_per_process_browsertest.h file. If this is bad, I guess I will have to
> either undo the refactoring in the test file (WebContentsImpl* cast replaced
by
> web_contents()) or change the method name back to GetWebContents() and define
it
> in the cc file.

This seems fine-- it's in the same spirit as the rest of the browsertest
exception.  Thanks for checking.

https://codereview.chromium.org/1652483002/diff/480001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1652483002/diff/480001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:2260: if
(view_with_active_text_input_ != rwhv &&
Is this comparison the only reason we need view_with_active_text_input_?  I'm
wondering if there's another way to track this without risking a use-after-free.

For example, suppose we kept the RenderWidgetHost's routing ID and process ID
around instead?  That would let us compare rwhv->GetRenderWidgetHost() against
the pair, without keeping an extra pointer to the RenderWidgetHostView.

Sorry that I'm being really conservative on this, but there's just way too much
code that has to be right to avoid a UaF here.  And even if there aren't many
uses of view_with_active_text_input_ now, I don't want future code in
WebContentsImpl to assume that possibly stale pointer is safe to use.

[EhsanK, 2016-03-30 23:36:33]

Thanks for the reviews. PTAL.

https://codereview.chromium.org/1652483002/diff/400001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1652483002/diff/400001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:1346: RenderWidgetHostViewBase*
view_with_active_text_input_;
Sorry for the incomplete explanation. I meant to say that the method
RenderWidgetHostViewBase::NotifyHostDelegateAboutShutdown() is supposed to take
care of all such scenarios. This method is called from RWHV::Destroy() and what
it does is to
 1) Reset the text_input_state() to NONE.
 2) Notify the RenderWidgetHostDelegate (WebContentsImpl) that the state has
changed.
Since everything happens on the same thread, we should never have stale pointers
or incorrect state.

I also added a test which verifies this when the RWHV crashes by calling the
RPH::Shutdown().

I also did manually check and printf  the state for scenarios such as
removeChild and navigation. It seems like most corner cases are covered.

All in all, with having a WeakPtr instead of the raw pointer, we should be fine
with the stale pointer problem.

https://codereview.chromium.org/1652483002/diff/480001/content/DEPS
File content/DEPS (right):

https://codereview.chromium.org/1652483002/diff/480001/content/DEPS#newcode118
content/DEPS:118: ".*_browsertest[a-z_]*\.(cc|h)": [
On 2016/03/30 21:52:47, Charlie Reis wrote:
> On 2016/03/30 20:46:04, ehsaaan wrote:
> > I had to add this so that I can call shell->web_contents() in the
> > site_per_process_browsertest.h file. If this is bad, I guess I will have to
> > either undo the refactoring in the test file (WebContentsImpl* cast replaced
> by
> > web_contents()) or change the method name back to GetWebContents() and
define
> it
> > in the cc file.
> 
> This seems fine-- it's in the same spirit as the rest of the browsertest
> exception.  Thanks for checking.

Acknowledged.

https://codereview.chromium.org/1652483002/diff/480001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1652483002/diff/480001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:2260: if
(view_with_active_text_input_ != rwhv &&
On 2016/03/30 21:52:47, Charlie Reis wrote:
> Is this comparison the only reason we need view_with_active_text_input_?  I'm
> wondering if there's another way to track this without risking a
use-after-free.
> 
> For example, suppose we kept the RenderWidgetHost's routing ID and process ID
> around instead?  That would let us compare rwhv->GetRenderWidgetHost() against
> the pair, without keeping an extra pointer to the RenderWidgetHostView.
> 
> Sorry that I'm being really conservative on this, but there's just way too
much
> code that has to be right to avoid a UaF here.  And even if there aren't many
> uses of view_with_active_text_input_ now, I don't want future code in
> WebContentsImpl to assume that possibly stale pointer is safe to use.

This makes a lot of sense...thanks! Following our discussion over hangouts, I
will use a WeakPtr given that RenderWidgetHostViewBase already supports that.

[EhsanK, 2016-03-30 23:50:21]

Description was changed from

==========
Implementing TextInputStateChanged for OOPIF.

Currently, the TextInputStateChanged method is not implemented in
RenderWidgetHostViewChildFrame. Conequently, the IME
will not work until this feature is fixed. 

This CL implements the text input state tracking through refactoring most of the
TextInputStateChanged code to the top-level RenderWidgetHostViewBase. The
implementation is
based on FrameTree::GetFocusedFrame() and hence does not apply to non-OOPIF
<webview> (RenderWidgetHostViewGuest). 

BUG=578168

CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Implementing TextInputStateChanged for OOPIF.

Currently, the TextInputStateChanged method is not implemented in
RenderWidgetHostViewChildFrame. Conequently, the IME
will not work until this feature is fixed. 

This CL implements the text input state tracking through refactoring most of the
TextInputStateChanged code to the top-level RenderWidgetHostViewBase. The
implementation is
based on FrameTree::GetFocusedFrame() and hence does not apply to non-OOPIF
<webview> (RenderWidgetHostViewGuest). 

This CL should also fix a previously reported race condition in the palindrome
crbug.com/546645 caused by RenderWidgetHostViewGuest's way of handling
TextInputStateChanged.

BUG=578168, 546645

CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2016-03-31 05:59:17]

Great.  Almost there!  Can you run try jobs (including the linux_site_isolation
one) so that we can make sure it's passing the tests on each platform?  A few
minor things below.

https://codereview.chromium.org/1652483002/diff/400001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1652483002/diff/400001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:1346: RenderWidgetHostViewBase*
view_with_active_text_input_;
On 2016/03/30 23:36:33, ehsaaan wrote:
> Sorry for the incomplete explanation. I meant to say that the method
> RenderWidgetHostViewBase::NotifyHostDelegateAboutShutdown() is supposed to
take
> care of all such scenarios. This method is called from RWHV::Destroy() and
what
> it does is to
>  1) Reset the text_input_state() to NONE.
>  2) Notify the RenderWidgetHostDelegate (WebContentsImpl) that the state has
> changed.
> Since everything happens on the same thread, we should never have stale
pointers
> or incorrect state.
> 
> I also added a test which verifies this when the RWHV crashes by calling the
> RPH::Shutdown().
> 
> I also did manually check and printf  the state for scenarios such as
> removeChild and navigation. It seems like most corner cases are covered.
> 
> All in all, with having a WeakPtr instead of the raw pointer, we should be
fine
> with the stale pointer problem.

Thanks for clarifying, and I agree about the WeakPtr making it safer.

The one part I don't follow is how NotifyHostDelegateAboutShutdown is called
from RWHV::Destroy.  There are lots of implementations of Destroy, and I only
see it called from some of them (Android, Mac, ChildFrame, and indirectly in
Guest).  Aura calls it from RemovingFromRootWindow (and I can't see how that's
connected to Destroy), and Mus doesn't seem to call it at all.

That's why I was concerned about clearing it in a consistent way, so that we
don't end up with platform-specific bugs.  Out of curiosity, would it work to
call it from ~RenderWidgetHostViewBase, so that we don't have to worry about all
the platform subclasses getting it right in their own way?

https://codereview.chromium.org/1652483002/diff/500001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1652483002/diff/500001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:2246: // A RWHV should update
WebContentsImpl in its destruction path so that that
nit: s/that that/that its/

https://codereview.chromium.org/1652483002/diff/500001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:2249: text_input_state_.type
!= ui::TEXT_INPUT_TYPE_NONE);
This doesn't look right-- it's saying that we'll crash if we ever call
GetTextInputState() when view_with_active_text_input_ exists.  Does it pass your
tests?

I think you're trying to say "either there's a view, or the state type is NONE,"
right?  Something like:
DCHECK(view_with_active_text_input_ || text_input_state_.type ==
ui::TEXT_INPUT_TYPE_NONE);

[EhsanK, 2016-03-31 18:06:09]

Description was changed from

==========
Implementing TextInputStateChanged for OOPIF.

Currently, the TextInputStateChanged method is not implemented in
RenderWidgetHostViewChildFrame. Conequently, the IME
will not work until this feature is fixed. 

This CL implements the text input state tracking through refactoring most of the
TextInputStateChanged code to the top-level RenderWidgetHostViewBase. The
implementation is
based on FrameTree::GetFocusedFrame() and hence does not apply to non-OOPIF
<webview> (RenderWidgetHostViewGuest). 

This CL should also fix a previously reported race condition in the palindrome
crbug.com/546645 caused by RenderWidgetHostViewGuest's way of handling
TextInputStateChanged.

BUG=578168, 546645

CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Browser Side Text Input State Tracking for OOPIF.

Currently, the TextInputStateChanged method is not implemented in
RenderWidgetHostViewChildFrame. Conequently, the IME
will not work until this feature is fixed. 

This CL implements the text input state tracking through refactoring most of the
TextInputStateChanged code to the top-level RenderWidgetHostViewBase. The
implementation is
based on FrameTree::GetFocusedFrame() and hence does not apply to non-OOPIF
<webview> (RenderWidgetHostViewGuest). 

This CL should also fix a previously reported race condition in the palindrome
crbug.com/546645 caused by RenderWidgetHostViewGuest's way of handling
TextInputStateChanged.

BUG=578168, 546645

CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[EhsanK, 2016-04-01 00:59:17]

PTAL.

https://codereview.chromium.org/1652483002/diff/400001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1652483002/diff/400001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:1346: RenderWidgetHostViewBase*
view_with_active_text_input_;
On 2016/03/30 23:36:33, ehsaaan wrote:
> Sorry for the incomplete explanation. I meant to say that the method
> RenderWidgetHostViewBase::NotifyHostDelegateAboutShutdown() is supposed to
take
> care of all such scenarios. This method is called from RWHV::Destroy() and
what
> it does is to
>  1) Reset the text_input_state() to NONE.
>  2) Notify the RenderWidgetHostDelegate (WebContentsImpl) that the state has
> changed.
> Since everything happens on the same thread, we should never have stale
pointers
> or incorrect state.
> 
> I also added a test which verifies this when the RWHV crashes by calling the
> RPH::Shutdown().
> 
> I also did manually check and printf  the state for scenarios such as
> removeChild and navigation. It seems like most corner cases are covered.
> 
> All in all, with having a WeakPtr instead of the raw pointer, we should be
fine
> with the stale pointer problem.

It is more correct to say that NotifyHostDelegateAboutShutdown() is called after
NotifyObseversAboutShutdown() (rather than inside ::Destroy() due to the comment
inside RenderWidgetHostViewAura::Destroy()).

https://codereview.chromium.org/1652483002/diff/400001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:1346: RenderWidgetHostViewBase*
view_with_active_text_input_;
> The one part I don't follow is how NotifyHostDelegateAboutShutdown is called
> from RWHV::Destroy.  There are lots of implementations of Destroy, and I only
> see it called from some of them (Android, Mac, ChildFrame, and indirectly in
> Guest).

I meant to say (I think I did not publish previous draft) that this method
should be called after NotifyObserversAboutShutdown(), which happens in Destory
function. For Aura it is different. There are no explicit calls to
NotifyObserversAboutShutdown. Further, RWHV::Destroy is not always hit in the
destruction path (the note in the cc file says this happens if we close the
window).

> Aura calls it from RemovingFromRootWindow (and I can't see how that's
> connected to Destroy)..
I guess that was one scenario leading to destruction. I picked that location
becayse the next line notifies the IME. Taking another look at the code, I
believe it is safe/better to call it in the dtor of Aura now. That will
definitely be where ::Destroy. This will also address the comment in
RWHVAura::Destroy in my opinion.

>...and Mus doesn't seem to call it at all.
Most of the code for Mus was not implemented including how the IME is going to
work so I had left it untouched and for that reason, RWHV::TextInputStateChanged
remained virtual. Now I have made it a public method of RWHV and added an
UpdateInputMethodIfNecessary to Mus but it is still empty. I also added
NotifyHostDelegateAboutShutdown to the Destroy method of RWHVMus.

> That's why I was concerned about clearing it in a consistent way, so that we
> don't end up with platform-specific bugs.  Out of curiosity, would it work to
> call it from ~RenderWidgetHostViewBase, so that we don't have to worry about
all
> the platform subclasses getting it right in their own way?
The way I see it, unfortunately not. We lose the reference to
RenderWidgetHostImpl before the call to RenderWidgetHostViewBase dtor. That
reference is needed to get the WebContentsImpl.

https://codereview.chromium.org/1652483002/diff/500001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1652483002/diff/500001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:2246: // A RWHV should update
WebContentsImpl in its destruction path so that that
On 2016/03/31 05:59:16, Charlie Reis wrote:
> nit: s/that that/that its/

Done.

https://codereview.chromium.org/1652483002/diff/500001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:2248:
DCHECK(!view_with_active_text_input_ &&
Not sure if having a DCHECK like this a good practice. I think this should never
hit unless there is a destruction path which does not lead to a call to
RWHVBase::NotifyHostDelegateAboutShutdown().

https://codereview.chromium.org/1652483002/diff/500001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:2249: text_input_state_.type
!= ui::TEXT_INPUT_TYPE_NONE);
On 2016/03/31 05:59:17, Charlie Reis wrote:
> This doesn't look right-- it's saying that we'll crash if we ever call
> GetTextInputState() when view_with_active_text_input_ exists.  Does it pass
your
> tests?
> 
> I think you're trying to say "either there's a view, or the state type is
NONE,"
> right?  Something like:
> DCHECK(view_with_active_text_input_ || text_input_state_.type ==
> ui::TEXT_INPUT_TYPE_NONE);

You are right. I keep forgetting DCHECK(false) crashes not DCHECK(true). So I
meant to crash when we do not have a valid view but the text input type is not
none. I did not run tests after this but pretty sure it will crash all of them
:).

[Charlie Reis, 2016-04-01 21:24:10]

Ok, thanks.  That's the extent to which I can review this (since I'm mainly
deferring to shuchen@ for the details).  content/ LGTM with one concern below
about a possible merge conflict gone wrong.

(In the future, it helps to do unrelated things like the
SitePerProcessBrowserTest helper method in a separate CL, which can make it
easier to avoid or spot merge conflicts.  Also, it looks like you've only run a
handful of try jobs rather than the full set-- I'd recommend running "git cl
try" to get the full set before trying to commit, given the size of this
change.)

Thanks for wrapping it up!

https://codereview.chromium.org/1652483002/diff/500001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1652483002/diff/500001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:2248:
DCHECK(!view_with_active_text_input_ &&
On 2016/04/01 00:59:17, ehsaaan wrote:
> Not sure if having a DCHECK like this a good practice. I think this should
never
> hit unless there is a destruction path which does not lead to a call to
> RWHVBase::NotifyHostDelegateAboutShutdown().

Yes, seems like a good thing to have.

https://codereview.chromium.org/1652483002/diff/580001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1652483002/diff/580001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:4938: // Helper function to run
the CreateContextMenuTest in either normal
Why is this in your CL?  This seems unrelated.  Is it due to a merge conflict?

https://codereview.chromium.org/1652483002/diff/580001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:6267: // Verfy the input type is
none in the begning.
nit: beginning

https://codereview.chromium.org/1652483002/diff/580001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:6280: // Send focus to the first
input field
nit: End with period.

https://codereview.chromium.org/1652483002/diff/580001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1652483002/diff/580001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:1332:
base::WeakPtr<RenderWidgetHostViewBase> view_with_active_text_input_;
nit: List before text_input_state_, and maybe mention in the comment that
text_input_state_ is specific to this RWHV.

[EhsanK, 2016-04-02 03:10:01]

On 2016/04/01 21:24:10, Charlie Reis (slow til 4-8) wrote:
> Ok, thanks.  That's the extent to which I can review this (since I'm mainly
> deferring to shuchen@ for the details).  content/ LGTM with one concern below
> about a possible merge conflict gone wrong.
> 
> (In the future, it helps to do unrelated things like the
> SitePerProcessBrowserTest helper method in a separate CL, which can make it
> easier to avoid or spot merge conflicts.  Also, it looks like you've only run
a
> handful of try jobs rather than the full set-- I'd recommend running "git cl
> try" to get the full set before trying to commit, given the size of this
> change.)
> 
> Thanks for wrapping it up!
> 
>
https://codereview.chromium.org/1652483002/diff/500001/content/browser/web_co...
> File content/browser/web_contents/web_contents_impl.cc (right):
> 
>
https://codereview.chromium.org/1652483002/diff/500001/content/browser/web_co...
> content/browser/web_contents/web_contents_impl.cc:2248:
> DCHECK(!view_with_active_text_input_ &&
> On 2016/04/01 00:59:17, ehsaaan wrote:
> > Not sure if having a DCHECK like this a good practice. I think this should
> never
> > hit unless there is a destruction path which does not lead to a call to
> > RWHVBase::NotifyHostDelegateAboutShutdown().
> 
> Yes, seems like a good thing to have.
> 
>
https://codereview.chromium.org/1652483002/diff/580001/content/browser/site_p...
> File content/browser/site_per_process_browsertest.cc (right):
> 
>
https://codereview.chromium.org/1652483002/diff/580001/content/browser/site_p...
> content/browser/site_per_process_browsertest.cc:4938: // Helper function to
run
> the CreateContextMenuTest in either normal
> Why is this in your CL?  This seems unrelated.  Is it due to a merge conflict?
> 
>
https://codereview.chromium.org/1652483002/diff/580001/content/browser/site_p...
> content/browser/site_per_process_browsertest.cc:6267: // Verfy the input type
is
> none in the begning.
> nit: beginning
> 
>
https://codereview.chromium.org/1652483002/diff/580001/content/browser/site_p...
> content/browser/site_per_process_browsertest.cc:6280: // Send focus to the
first
> input field
> nit: End with period.
> 
>
https://codereview.chromium.org/1652483002/diff/580001/content/browser/web_co...
> File content/browser/web_contents/web_contents_impl.h (right):
> 
>
https://codereview.chromium.org/1652483002/diff/580001/content/browser/web_co...
> content/browser/web_contents/web_contents_impl.h:1332:
> base::WeakPtr<RenderWidgetHostViewBase> view_with_active_text_input_;
> nit: List before text_input_state_, and maybe mention in the comment that
> text_input_state_ is specific to this RWHV.

Thanks Charlie for the reviews. I guess I will refactor all those static casts
in a follow -up CL.

[EhsanK, 2016-04-02 03:14:03]

https://codereview.chromium.org/1652483002/diff/580001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1652483002/diff/580001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:4938: // Helper function to run
the CreateContextMenuTest in either normal
On 2016/04/01 21:24:09, Charlie Reis (slow til 4-8) wrote:
> Why is this in your CL?  This seems unrelated.  Is it due to a merge conflict?

I guess it is. Sorry about that. I reverted the refactor but kept the
web_contents() method. I will submit a followup CL after this one lands. This
makes more sense and easier to revert if necessary.

https://codereview.chromium.org/1652483002/diff/580001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:6267: // Verfy the input type is
none in the begning.
On 2016/04/01 21:24:10, Charlie Reis (slow til 4-8) wrote:
> nit: beginning

Done. Thanks!

https://codereview.chromium.org/1652483002/diff/580001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:6280: // Send focus to the first
input field
On 2016/04/01 21:24:09, Charlie Reis (slow til 4-8) wrote:
> nit: End with period.

Done. Thanks!

https://codereview.chromium.org/1652483002/diff/580001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1652483002/diff/580001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:1332:
base::WeakPtr<RenderWidgetHostViewBase> view_with_active_text_input_;
On 2016/04/01 21:24:10, Charlie Reis (slow til 4-8) wrote:
> nit: List before text_input_state_, and maybe mention in the comment that
> text_input_state_ is specific to this RWHV.

Done.

[lazyboy, 2016-04-04 19:17:59]

One general concern in the test.

Please follow up with TextInputState being copied around issue if you're not
handling it in this CL.

https://codereview.chromium.org/1652483002/diff/600001/chrome/browser/apps/gu...
File chrome/browser/apps/guest_view/web_view_browsertest.cc (right):

https://codereview.chromium.org/1652483002/diff/600001/chrome/browser/apps/gu...
chrome/browser/apps/guest_view/web_view_browsertest.cc:3185:
TextInputStateHelper::WaitForDesiredState(
Generally, queuing tasks to wait for a change is discouraged.

How about you have a "focus" handler in the embedder's 2016 text box and send a
message to the test and then verify TextInputStateTestExport's state is the
correct one? That way you don't have to do the busy wait thingy:

in embedder:
inputElement.onfocus = function() { chrome.testSendMessage('FOCUSED'); }
ExtensionTestMessageListener listener("FOCUSED");
press_tab();
listener.WaitUntilSatisfied();

If this works, then do the same for other steps below.

https://codereview.chromium.org/1652483002/diff/600001/content/browser/browse...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/1652483002/diff/600001/content/browser/browse...
content/browser/browser_plugin/browser_plugin_guest.cc:986:
SendTextInputTypeChangedToView(
(As discussed offline): maybe we can go to web_contents() directly instead of
through view. Sadly, we might still need to store |last_text_input_state_|

[kenrb, 2016-04-04 19:25:15]

lgtm ipc/

[EhsanK, 2016-04-06 00:37:58]

@lazyboy: PTAL. I turned the struct return type to pointer in this CL. Thanks!

https://codereview.chromium.org/1652483002/diff/600001/chrome/browser/apps/gu...
File chrome/browser/apps/guest_view/web_view_browsertest.cc (right):

https://codereview.chromium.org/1652483002/diff/600001/chrome/browser/apps/gu...
chrome/browser/apps/guest_view/web_view_browsertest.cc:3185:
TextInputStateHelper::WaitForDesiredState(
On 2016/04/04 19:17:59, lazyboy wrote:
> Generally, queuing tasks to wait for a change is discouraged.
> 
> How about you have a "focus" handler in the embedder's 2016 text box and send
a
> message to the test and then verify TextInputStateTestExport's state is the
> correct one? That way you don't have to do the busy wait thingy:
> 
> in embedder:
> inputElement.onfocus = function() { chrome.testSendMessage('FOCUSED'); }
> ExtensionTestMessageListener listener("FOCUSED");
> press_tab();
> listener.WaitUntilSatisfied();
> 
> If this works, then do the same for other steps below.

(As discussed offline) I now listen to 'onfocus()' for inputs and then start
polling the text input state.

(As discussed offline 2) The test now lives in
web_view_interactive_browsertest.cc (due to tab key simulation issues on Mac).

https://codereview.chromium.org/1652483002/diff/600001/content/browser/browse...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/1652483002/diff/600001/content/browser/browse...
content/browser/browser_plugin/browser_plugin_guest.cc:986:
SendTextInputTypeChangedToView(
On 2016/04/04 19:17:59, lazyboy wrote:
> (As discussed offline): maybe we can go to web_contents() directly instead of
> through view. Sadly, we might still need to store |last_text_input_state_|

Yes. I will try to fix it in a follow-up CL.

[lazyboy, 2016-04-06 02:39:54]

tests lgtm once you address the comments, thanks!

https://codereview.chromium.org/1652483002/diff/680001/chrome/browser/apps/gu...
File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right):

https://codereview.chromium.org/1652483002/diff/680001/chrome/browser/apps/gu...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:528:
testing::Bool());
testing::Values(false), since all the tests in this test class
(WebViewTextInputStateInteractiveTest) is disabled under OOPIF, then you could
remove the early return on lines 1392, 1433 below.

Pull the TODO to fix this from below to here. And possibly refer to a bug.

https://codereview.chromium.org/1652483002/diff/680001/chrome/browser/apps/gu...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1356: class
TextInputStateHelper {
Move this class to a anonymous namespace and put it at the very beginning of
this file.

https://codereview.chromium.org/1652483002/diff/680001/chrome/browser/apps/gu...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1370:
base::TimeDelta::FromMilliseconds(1LL));
1ms is too agressive, I'd do ~100ms, put the value in a constant
kInputStateMonitorRetryInterval or sth.

https://codereview.chromium.org/1652483002/diff/680001/chrome/test/data/exten...
File
chrome/test/data/extensions/platform_apps/web_view/text_input_state/window.html
(right):

https://codereview.chromium.org/1652483002/diff/680001/chrome/test/data/exten...
chrome/test/data/extensions/platform_apps/web_view/text_input_state/window.html:13:
<div id="webview-embedder"></div>
nit: webview-container or sth, both webview and embedder in one id is a bit
confusing :)

[EhsanK, 2016-04-06 16:04:50]

Thanks for the reviews!

https://codereview.chromium.org/1652483002/diff/680001/chrome/browser/apps/gu...
File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right):

https://codereview.chromium.org/1652483002/diff/680001/chrome/browser/apps/gu...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:528:
testing::Bool());
On 2016/04/06 02:39:53, lazyboy wrote:
> testing::Values(false), since all the tests in this test class
> (WebViewTextInputStateInteractiveTest) is disabled under OOPIF, then you could
> remove the early return on lines 1392, 1433 below.
> 
> Pull the TODO to fix this from below to here. And possibly refer to a bug.

Done.

https://codereview.chromium.org/1652483002/diff/680001/chrome/browser/apps/gu...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1356: class
TextInputStateHelper {
On 2016/04/06 02:39:54, lazyboy wrote:
> Move this class to a anonymous namespace and put it at the very beginning of
> this file.

Done.

https://codereview.chromium.org/1652483002/diff/680001/chrome/browser/apps/gu...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1370:
base::TimeDelta::FromMilliseconds(1LL));
On 2016/04/06 02:39:53, lazyboy wrote:
> 1ms is too agressive, I'd do ~100ms, put the value in a constant
> kInputStateMonitorRetryInterval or sth.

Done.

https://codereview.chromium.org/1652483002/diff/680001/chrome/test/data/exten...
File
chrome/test/data/extensions/platform_apps/web_view/text_input_state/window.html
(right):

https://codereview.chromium.org/1652483002/diff/680001/chrome/test/data/exten...
chrome/test/data/extensions/platform_apps/web_view/text_input_state/window.html:13:
<div id="webview-embedder"></div>
On 2016/04/06 02:39:54, lazyboy wrote:
> nit: webview-container or sth, both webview and embedder in one id is a bit
> confusing :)

:). Done.

[EhsanK, 2016-04-06 16:05:07]

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

[commit-bot: I haz the power, 2016-04-06 16:05:34]

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

[commit-bot: I haz the power, 2016-04-06 16:08:28]

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

[commit-bot: I haz the power, 2016-04-06 16:08:31]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  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_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[EhsanK, 2016-04-06 18:28:53]

The CQ bit was checked by ekaramad@chromium.org

[EhsanK, 2016-04-06 18:28:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
shuchen@chromium.org, creis@chromium.org, kenrb@chromium.org,
lazyboy@chromium.org
Link to the patchset: https://codereview.chromium.org/1652483002/#ps720001
(title: "Merged")

[commit-bot: I haz the power, 2016-04-06 18:29:30]

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

[commit-bot: I haz the power, 2016-04-06 20:12:58]

Description was changed from

==========
Browser Side Text Input State Tracking for OOPIF.

Currently, the TextInputStateChanged method is not implemented in
RenderWidgetHostViewChildFrame. Conequently, the IME
will not work until this feature is fixed. 

This CL implements the text input state tracking through refactoring most of the
TextInputStateChanged code to the top-level RenderWidgetHostViewBase. The
implementation is
based on FrameTree::GetFocusedFrame() and hence does not apply to non-OOPIF
<webview> (RenderWidgetHostViewGuest). 

This CL should also fix a previously reported race condition in the palindrome
crbug.com/546645 caused by RenderWidgetHostViewGuest's way of handling
TextInputStateChanged.

BUG=578168, 546645

CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Browser Side Text Input State Tracking for OOPIF.

Currently, the TextInputStateChanged method is not implemented in
RenderWidgetHostViewChildFrame. Conequently, the IME
will not work until this feature is fixed. 

This CL implements the text input state tracking through refactoring most of the
TextInputStateChanged code to the top-level RenderWidgetHostViewBase. The
implementation is
based on FrameTree::GetFocusedFrame() and hence does not apply to non-OOPIF
<webview> (RenderWidgetHostViewGuest). 

This CL should also fix a previously reported race condition in the palindrome
crbug.com/546645 caused by RenderWidgetHostViewGuest's way of handling
TextInputStateChanged.

BUG=578168, 546645

CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-04-06 20:13:01]

Committed patchset #28 (id:720001)

[commit-bot: I haz the power, 2016-04-06 20:14:10]

Description was changed from

==========
Browser Side Text Input State Tracking for OOPIF.

Currently, the TextInputStateChanged method is not implemented in
RenderWidgetHostViewChildFrame. Conequently, the IME
will not work until this feature is fixed. 

This CL implements the text input state tracking through refactoring most of the
TextInputStateChanged code to the top-level RenderWidgetHostViewBase. The
implementation is
based on FrameTree::GetFocusedFrame() and hence does not apply to non-OOPIF
<webview> (RenderWidgetHostViewGuest). 

This CL should also fix a previously reported race condition in the palindrome
crbug.com/546645 caused by RenderWidgetHostViewGuest's way of handling
TextInputStateChanged.

BUG=578168, 546645

CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Browser Side Text Input State Tracking for OOPIF.

Currently, the TextInputStateChanged method is not implemented in
RenderWidgetHostViewChildFrame. Conequently, the IME
will not work until this feature is fixed.

This CL implements the text input state tracking through refactoring most of the
TextInputStateChanged code to the top-level RenderWidgetHostViewBase. The
implementation is
based on FrameTree::GetFocusedFrame() and hence does not apply to non-OOPIF
<webview> (RenderWidgetHostViewGuest).

This CL should also fix a previously reported race condition in the palindrome
crbug.com/546645 caused by RenderWidgetHostViewGuest's way of handling
TextInputStateChanged.

BUG=578168, 546645

CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/2bd4a2f0c0f114979d47f4498ea1bce9b091591e
Cr-Commit-Position: refs/heads/master@{#385537}
==========

[commit-bot: I haz the power, 2016-04-06 20:14:13]

Patchset 28 (id:??) landed as
https://crrev.com/2bd4a2f0c0f114979d47f4498ea1bce9b091591e
Cr-Commit-Position: refs/heads/master@{#385537}

[EhsanK, 2016-04-07 01:34:28]

Description was changed from

==========
Browser Side Text Input State Tracking for OOPIF.

Currently, the TextInputStateChanged method is not implemented in
RenderWidgetHostViewChildFrame. Conequently, the IME
will not work until this feature is fixed.

This CL implements the text input state tracking through refactoring most of the
TextInputStateChanged code to the top-level RenderWidgetHostViewBase. The
implementation is
based on FrameTree::GetFocusedFrame() and hence does not apply to non-OOPIF
<webview> (RenderWidgetHostViewGuest).

This CL should also fix a previously reported race condition in the palindrome
crbug.com/546645 caused by RenderWidgetHostViewGuest's way of handling
TextInputStateChanged.

BUG=578168, 546645

CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/2bd4a2f0c0f114979d47f4498ea1bce9b091591e
Cr-Commit-Position: refs/heads/master@{#385537}
==========

to

==========
Browser Side Text Input State Tracking for OOPIF.

Currently, the TextInputStateChanged method is not implemented in
RenderWidgetHostViewChildFrame. Conequently, the IME
will not work until this feature is fixed.

This CL implements the text input state tracking through refactoring most of the
TextInputStateChanged code to the top-level RenderWidgetHostViewBase.

This CL should also fix a previously reported race condition in the palindrome
crbug.com/546645 caused by RenderWidgetHostViewGuest's way of handling
TextInputStateChanged.

BUG=578168, 546645

CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/2bd4a2f0c0f114979d47f4498ea1bce9b091591e
Cr-Commit-Position: refs/heads/master@{#385537}
==========

[EhsanK, 2016-04-15 19:22:12]

A revert of this CL (patchset #28 id:720001) has been created in
https://codereview.chromium.org/1895523002/ by ekaramad@chromium.org.

The reason for reverting is: This patch caused many regressions..