Replace ViewHostMsg_TextInputState_Params with content::TextInputState.

Replace ViewHostMsg_TextInputState_Params with content::TextInputState.

This refactoring is the first step in relanding
"Browser Side Text Input State Tracking for OOPIF"
(original CL: https://codereview.chromium.org/1652483002).

For context on initialization of all struct variables refer to this CL:
https://codereview.chromium.org/1894473002.

For context on the default copy constructor refer to this CL:
https://codereview.chromium.org/1867153002.

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

Committed: https://crrev.com/9053e57bfd10c2466cbcb0fe50389e473c1682ef
Cr-Commit-Position: refs/heads/master@{#389863}

[EhsanK, 2016-04-22 17:51:45]

Description was changed from

==========
Replace ViewHostMsg_TextInputState_Params with content::TextInputState.

This refactoring is the first step in relanding
"Browser Side Text Input State Tracking for OOPIF"
(original CL: https://codereview.chromium.org/1652483002).

For context on initialization of all struct variables refer to this CL:
https://codereview.chromium.org/1894473002.

For context on the default copy constructor refer to this CL:
https://codereview.chromium.org/1867153002.

BUG=578168
==========

to

==========
Replace ViewHostMsg_TextInputState_Params with content::TextInputState.

This refactoring is the first step in relanding
"Browser Side Text Input State Tracking for OOPIF"
(original CL: https://codereview.chromium.org/1652483002).

For context on initialization of all struct variables refer to this CL:
https://codereview.chromium.org/1894473002.

For context on the default copy constructor refer to this CL:
https://codereview.chromium.org/1867153002.

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

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

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

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

kenrb@chromium.org: Please review changes in 

creis@chromium.org: Please review changes in 

This is the first part of the relanding. This simply replaces the old struct.
Please take a look.

[Charlie Reis, 2016-04-22 23:02:38]

Thanks!  Mostly nits, but one outstanding question from kinaba's CL.

On 2016/04/22 19:12:29, ehsaaan wrote:
> mailto:kenrb@chromium.org: Please review changes in 
> 
> mailto:creis@chromium.org: Please review changes in 

I assume you want us both to look at everything?

> 
> This is the first part of the relanding. This simply replaces the old struct.
> Please take a look.

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

https://codereview.chromium.org/1914603002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_widget_host_view_child_frame.h:36: struct
TextInputState;
nit: Move to declarations below, since this belongs in the content namespace.

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

https://codereview.chromium.org/1914603002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_widget_host_view_guest.h:27: struct
TextInputState;
nit: Move to declarations below, since this belongs in the content namespace.

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

https://codereview.chromium.org/1914603002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_impl.h:51: struct
TextInputState;
nit: Belongs in content namespace.  (Same in remaining files.)

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

https://codereview.chromium.org/1914603002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_mus.h:21: struct
NativeWebKeyboardEvent;
Need to declare the struct.

https://codereview.chromium.org/1914603002/diff/1/content/common/text_input_s...
File content/common/text_input_state.cc (right):

https://codereview.chromium.org/1914603002/diff/1/content/common/text_input_s...
content/common/text_input_state.cc:19: is_non_ime_change(true) {}
Does this require a change to BrowserPluginGuest, as we discussed in
https://codereview.chromium.org/1894473002/diff/1/content/common/text_input_s...

https://codereview.chromium.org/1914603002/diff/1/content/common/text_input_s...
File content/common/text_input_state.h (right):

https://codereview.chromium.org/1914603002/diff/1/content/common/text_input_s...
content/common/text_input_state.h:16: struct CONTENT_EXPORT TextInputState {
Let's put a top-level comment on this class here, saying how this relates to IME
support (and/or anything else it's used for).

https://codereview.chromium.org/1914603002/diff/1/content/common/text_input_s...
content/common/text_input_state.h:18: TextInputState(const TextInputState&
other);
nit: Add blank line after.

https://codereview.chromium.org/1914603002/diff/1/content/test/test_render_vi...
File content/test/test_render_view_host.h (right):

https://codereview.chromium.org/1914603002/diff/1/content/test/test_render_vi...
content/test/test_render_view_host.h:34: struct TextInputState;
nit: Belongs in content namespace.

[Charlie Reis, 2016-04-22 23:03:00]

Description was changed from

==========
Replace ViewHostMsg_TextInputState_Params with content::TextInputState.

This refactoring is the first step in relanding
"Browser Side Text Input State Tracking for OOPIF"
(original CL: https://codereview.chromium.org/1652483002).

For context on initialization of all struct variables refer to this CL:
https://codereview.chromium.org/1894473002.

For context on the default copy constructor refer to this CL:
https://codereview.chromium.org/1867153002.

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

to

==========
Replace ViewHostMsg_TextInputState_Params with content::TextInputState.

This refactoring is the first step in relanding
"Browser Side Text Input State Tracking for OOPIF"
(original CL: https://codereview.chromium.org/1652483002).

For context on initialization of all struct variables refer to this CL:
https://codereview.chromium.org/1894473002.

For context on the default copy constructor refer to this CL:
https://codereview.chromium.org/1867153002.

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

[Charlie Reis, 2016-04-22 23:03:17]

[+site-isolation-reviews]

[EhsanK, 2016-04-25 19:09:00]

Please, take another look.

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

https://codereview.chromium.org/1914603002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_widget_host_view_child_frame.h:36: struct
TextInputState;
On 2016/04/22 23:02:38, Charlie Reis wrote:
> nit: Move to declarations below, since this belongs in the content namespace.

Done.

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

https://codereview.chromium.org/1914603002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_widget_host_view_guest.h:27: struct
TextInputState;
On 2016/04/22 23:02:38, Charlie Reis wrote:
> nit: Move to declarations below, since this belongs in the content namespace.

Done.

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

https://codereview.chromium.org/1914603002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_impl.h:51: struct
TextInputState;
On 2016/04/22 23:02:38, Charlie Reis wrote:
> nit: Belongs in content namespace.  (Same in remaining files.)

Done.

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

https://codereview.chromium.org/1914603002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_mus.h:21: struct
NativeWebKeyboardEvent;
On 2016/04/22 23:02:38, Charlie Reis wrote:
> Need to declare the struct.

Done.

https://codereview.chromium.org/1914603002/diff/1/content/common/text_input_s...
File content/common/text_input_state.cc (right):

https://codereview.chromium.org/1914603002/diff/1/content/common/text_input_s...
content/common/text_input_state.cc:19: is_non_ime_change(true) {}
On 2016/04/22 23:02:38, Charlie Reis wrote:
> Does this require a change to BrowserPluginGuest, as we discussed in
>
https://codereview.chromium.org/1894473002/diff/1/content/common/text_input_s...

This initialization is compatible to what we decided to do last (the least
harmful way), which was to However: 
  - The issue in that CL was that we were creating TextInputState() to reset the
state for RWHVGuest by calling RWHVGuest::TextInputStateChanged. This was
leading to either an uncalled for IPC (which is the case right now), or
potential crashes if |is_non_ime_change| was true.

  - That issue is something to resolve in the upcoming CL to land text input
state tracking for OOPIF. I actually, think I will not modify <webview> text
input tracking at this time until OOPIF lands. BrowserPlugin <webview> might
need further fixes before we can implement a similar strategy
(crbug.com/602640).

https://codereview.chromium.org/1914603002/diff/1/content/common/text_input_s...
File content/common/text_input_state.h (right):

https://codereview.chromium.org/1914603002/diff/1/content/common/text_input_s...
content/common/text_input_state.h:16: struct CONTENT_EXPORT TextInputState {
On 2016/04/22 23:02:38, Charlie Reis wrote:
> Let's put a top-level comment on this class here, saying how this relates to
IME
> support (and/or anything else it's used for).

Done.

https://codereview.chromium.org/1914603002/diff/1/content/common/text_input_s...
content/common/text_input_state.h:18: TextInputState(const TextInputState&
other);
On 2016/04/22 23:02:38, Charlie Reis wrote:
> nit: Add blank line after.

Done.

https://codereview.chromium.org/1914603002/diff/1/content/test/test_render_vi...
File content/test/test_render_view_host.h (right):

https://codereview.chromium.org/1914603002/diff/1/content/test/test_render_vi...
content/test/test_render_view_host.h:34: struct TextInputState;
On 2016/04/22 23:02:38, Charlie Reis wrote:
> nit: Belongs in content namespace.

Done.

[Charlie Reis, 2016-04-26 06:13:23]

Thanks, LGTM if Ken is happy.

Side note: If you need to rebase between patch sets, it helps reviewers if you
upload that as a separate patch set than your actual changes.  (Otherwise it's
hard to review the diffs between patch sets.)

https://codereview.chromium.org/1914603002/diff/1/content/common/text_input_s...
File content/common/text_input_state.cc (right):

https://codereview.chromium.org/1914603002/diff/1/content/common/text_input_s...
content/common/text_input_state.cc:19: is_non_ime_change(true) {}
On 2016/04/25 19:09:00, ehsaaan wrote:
> On 2016/04/22 23:02:38, Charlie Reis wrote:
> > Does this require a change to BrowserPluginGuest, as we discussed in
> >
>
https://codereview.chromium.org/1894473002/diff/1/content/common/text_input_s...
> 
> This initialization is compatible to what we decided to do last (the least
> harmful way), which was to However: 
>   - The issue in that CL was that we were creating TextInputState() to reset
the
> state for RWHVGuest by calling RWHVGuest::TextInputStateChanged. This was
> leading to either an uncalled for IPC (which is the case right now), or
> potential crashes if |is_non_ime_change| was true.
> 
>   - That issue is something to resolve in the upcoming CL to land text input
> state tracking for OOPIF. I actually, think I will not modify <webview> text
> input tracking at this time until OOPIF lands. BrowserPlugin <webview> might
> need further fixes before we can implement a similar strategy
> (crbug.com/602640).

Acknowledged.

https://codereview.chromium.org/1914603002/diff/20001/content/common/text_inp...
File content/common/text_input_state.h (right):

https://codereview.chromium.org/1914603002/diff/20001/content/common/text_inp...
content/common/text_input_state.h:20: // text input state information on the
browser side.
nit: This last sentence seems redundant with the first.  We can probably drop
it.

[EhsanK, 2016-04-26 14:29:05]

On 2016/04/26 06:13:23, Charlie Reis wrote:
> Thanks, LGTM if Ken is happy.
> 
> Side note: If you need to rebase between patch sets, it helps reviewers if you
> upload that as a separate patch set than your actual changes.  (Otherwise it's
> hard to review the diffs between patch sets.)
> 
>
https://codereview.chromium.org/1914603002/diff/1/content/common/text_input_s...
> File content/common/text_input_state.cc (right):
> 
>
https://codereview.chromium.org/1914603002/diff/1/content/common/text_input_s...
> content/common/text_input_state.cc:19: is_non_ime_change(true) {}
> On 2016/04/25 19:09:00, ehsaaan wrote:
> > On 2016/04/22 23:02:38, Charlie Reis wrote:
> > > Does this require a change to BrowserPluginGuest, as we discussed in
> > >
> >
>
https://codereview.chromium.org/1894473002/diff/1/content/common/text_input_s...
> > 
> > This initialization is compatible to what we decided to do last (the least
> > harmful way), which was to However: 
> >   - The issue in that CL was that we were creating TextInputState() to reset
> the
> > state for RWHVGuest by calling RWHVGuest::TextInputStateChanged. This was
> > leading to either an uncalled for IPC (which is the case right now), or
> > potential crashes if |is_non_ime_change| was true.
> > 
> >   - That issue is something to resolve in the upcoming CL to land text input
> > state tracking for OOPIF. I actually, think I will not modify <webview> text
> > input tracking at this time until OOPIF lands. BrowserPlugin <webview> might
> > need further fixes before we can implement a similar strategy
> > (crbug.com/602640).
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/1914603002/diff/20001/content/common/text_inp...
> File content/common/text_input_state.h (right):
> 
>
https://codereview.chromium.org/1914603002/diff/20001/content/common/text_inp...
> content/common/text_input_state.h:20: // text input state information on the
> browser side.
> nit: This last sentence seems redundant with the first.  We can probably drop
> it.

Acknowledged! Thanks for the reviews, Charlie.

[kenrb, 2016-04-26 15:54:19]

lgtm with a couple of nits.

https://codereview.chromium.org/1914603002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/1914603002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_guest.cc:350: const
TextInputState& params) {
Should we also include text_input_state.h in this file, even though we don't
access any of the fields?

https://codereview.chromium.org/1914603002/diff/20001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1914603002/diff/20001/content/renderer/render...
content/renderer/render_widget.cc:1061: TextInputState params;
I think this is pulling the definition through the include of view_messages.h.
You should also #include text_input_state.h here (because IWYU).

[EhsanK, 2016-04-26 18:02:06]

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

[commit-bot: I haz the power, 2016-04-26 18:02:20]

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

[EhsanK, 2016-04-26 18:02:47]

Thank you for the reviews! FYI, I will commit this patch after Dry Run.

https://codereview.chromium.org/1914603002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/1914603002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_guest.cc:350: const
TextInputState& params) {
On 2016/04/26 15:54:19, kenrb wrote:
> Should we also include text_input_state.h in this file, even though we don't
> access any of the fields?

Per our offline discussion, the forward declaration in the header file should
suffice.

https://codereview.chromium.org/1914603002/diff/20001/content/common/text_inp...
File content/common/text_input_state.h (right):

https://codereview.chromium.org/1914603002/diff/20001/content/common/text_inp...
content/common/text_input_state.h:20: // text input state information on the
browser side.
On 2016/04/26 06:13:23, Charlie Reis wrote:
> nit: This last sentence seems redundant with the first.  We can probably drop
> it.

Done.

https://codereview.chromium.org/1914603002/diff/20001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1914603002/diff/20001/content/renderer/render...
content/renderer/render_widget.cc:1061: TextInputState params;
On 2016/04/26 15:54:19, kenrb wrote:
> I think this is pulling the definition through the include of view_messages.h.
> You should also #include text_input_state.h here (because IWYU).

Acknowledged.

[commit-bot: I haz the power, 2016-04-26 19:54:54]

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

[commit-bot: I haz the power, 2016-04-26 19:54:55]

Dry run: This issue passed the CQ dry run.

[EhsanK, 2016-04-26 19:55:26]

The CQ bit was checked by ekaramad@chromium.org

[EhsanK, 2016-04-26 19:55:26]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org,
creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1914603002/#ps40001
(title: "Addressing Comments")

[commit-bot: I haz the power, 2016-04-26 19:55:52]

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

[commit-bot: I haz the power, 2016-04-26 20:00:51]

Description was changed from

==========
Replace ViewHostMsg_TextInputState_Params with content::TextInputState.

This refactoring is the first step in relanding
"Browser Side Text Input State Tracking for OOPIF"
(original CL: https://codereview.chromium.org/1652483002).

For context on initialization of all struct variables refer to this CL:
https://codereview.chromium.org/1894473002.

For context on the default copy constructor refer to this CL:
https://codereview.chromium.org/1867153002.

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

to

==========
Replace ViewHostMsg_TextInputState_Params with content::TextInputState.

This refactoring is the first step in relanding
"Browser Side Text Input State Tracking for OOPIF"
(original CL: https://codereview.chromium.org/1652483002).

For context on initialization of all struct variables refer to this CL:
https://codereview.chromium.org/1894473002.

For context on the default copy constructor refer to this CL:
https://codereview.chromium.org/1867153002.

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

[commit-bot: I haz the power, 2016-04-26 20:00:53]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-26 20:01:53]

Description was changed from

==========
Replace ViewHostMsg_TextInputState_Params with content::TextInputState.

This refactoring is the first step in relanding
"Browser Side Text Input State Tracking for OOPIF"
(original CL: https://codereview.chromium.org/1652483002).

For context on initialization of all struct variables refer to this CL:
https://codereview.chromium.org/1894473002.

For context on the default copy constructor refer to this CL:
https://codereview.chromium.org/1867153002.

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

to

==========
Replace ViewHostMsg_TextInputState_Params with content::TextInputState.

This refactoring is the first step in relanding
"Browser Side Text Input State Tracking for OOPIF"
(original CL: https://codereview.chromium.org/1652483002).

For context on initialization of all struct variables refer to this CL:
https://codereview.chromium.org/1894473002.

For context on the default copy constructor refer to this CL:
https://codereview.chromium.org/1867153002.

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

Committed: https://crrev.com/9053e57bfd10c2466cbcb0fe50389e473c1682ef
Cr-Commit-Position: refs/heads/master@{#389863}
==========

[commit-bot: I haz the power, 2016-04-26 20:01:54]

Patchset 3 (id:??) landed as
https://crrev.com/9053e57bfd10c2466cbcb0fe50389e473c1682ef
Cr-Commit-Position: refs/heads/master@{#389863}