Merge ViewHostMsg_TextInputTypeChanged and ViewHostMsg_TextInputStateChanged into one.

Merge ViewHostMsg_TextInputTypeChanged and ViewHostMsg_TextInputStateChanged into one.

This is reland of cl https://codereview.chromium.org/326403002, which
used to cause regresssion crbug.com/417152.
As mentioned #13 of crbug.com/417152, the regression root cause was due
to the |input_method_active_| flag in RenderWidget which caused on
windows, UpdateTextInputType() always sends IPC while
UpdateTextInputState() doesn't.

After |input_method_active_| is removed by cl
https://codereview.chromium.org/1234193002, we can reland it safely.

BUG=377169, 474828
TEST=crbug.com/417152 not repro with this cl and trybots green.

Committed: https://crrev.com/913f8b627c442721376c7ae6c5ceaf33f684a434
Cr-Commit-Position: refs/heads/master@{#341324}

[Shu Chen, 2015-07-23 02:10:29]

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

[Shu Chen, 2015-07-23 02:10:30]

nasko@, can you please help to review this cl? Thanks

[Shu Chen, 2015-07-23 02:10:46]

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

[commit-bot: I haz the power, 2015-07-23 02:10:53]

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

[commit-bot: I haz the power, 2015-07-23 02:12:10]

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

[commit-bot: I haz the power, 2015-07-23 02:12:10]

Dry run: Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Shu Chen, 2015-07-23 02:38:00]

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

[commit-bot: I haz the power, 2015-07-23 02:38:06]

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

[commit-bot: I haz the power, 2015-07-23 02:47:23]

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

[commit-bot: I haz the power, 2015-07-23 02:47:23]

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

[Shu Chen, 2015-07-23 04:33:33]

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

[commit-bot: I haz the power, 2015-07-23 04:33:50]

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

[commit-bot: I haz the power, 2015-07-23 06:13:13]

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

[commit-bot: I haz the power, 2015-07-23 06:13:14]

Dry run: This issue passed the CQ dry run.

[jdduke (slow), 2015-07-23 15:12:49]

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

[jdduke (slow), 2015-07-23 15:12:50]

Thank you! As a non-owner and person unfamiliar with many of these details, the
Android changes lgtm.

https://codereview.chromium.org/1249013005/diff/40001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1249013005/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:3024: RenderWidget::NO_SHOW_IME,
RenderWidget::FROM_NON_IME);
I assume the order relative to SyncSelectionIfRequired doesn't matter?

[Shu Chen, 2015-07-24 08:59:16]

shuchen@chromium.org changed reviewers:
+ sievers@chromium.org, wfh@chromium.org, yukawa@chromium.org

[Shu Chen, 2015-07-24 08:59:17]

Since nasko@ is out, 

+wfh@ for approval of content/common/view_messages.h.

+sievers@@ for approval of other changes under content/....

+yukawa@ for IME related stuff.

Thanks!

[Shu Chen, 2015-07-24 09:14:05]

https://codereview.chromium.org/1249013005/diff/40001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1249013005/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:3024: RenderWidget::NO_SHOW_IME,
RenderWidget::FROM_NON_IME);
On 2015/07/23 15:12:50, jdduke wrote:
> I assume the order relative to SyncSelectionIfRequired doesn't matter?

SyncSelectionIfRequired() sends IPCs to browser for selection info, and
UpdateTextInputState() sends IPCs for input field info. I'm not familiar with
Clank. Is Clank sensitive for the sequence of below IPCs?
 - ViewHostMsg_SelectionChanged
 - ViewHostMsg_SelectionBoundsChanged
 - ViewHostMsg_TextInputStateChanged

[jdduke (slow), 2015-07-24 15:10:49]

On 2015/07/24 09:14:05, Shu Chen wrote:
>
https://codereview.chromium.org/1249013005/diff/40001/content/renderer/render...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/1249013005/diff/40001/content/renderer/render...
> content/renderer/render_frame_impl.cc:3024: RenderWidget::NO_SHOW_IME,
> RenderWidget::FROM_NON_IME);
> On 2015/07/23 15:12:50, jdduke wrote:
> > I assume the order relative to SyncSelectionIfRequired doesn't matter?
> 
> SyncSelectionIfRequired() sends IPCs to browser for selection info, and
> UpdateTextInputState() sends IPCs for input field info. I'm not familiar with
> Clank. Is Clank sensitive for the sequence of below IPCs?
>  - ViewHostMsg_SelectionChanged
>  - ViewHostMsg_SelectionBoundsChanged
>  - ViewHostMsg_TextInputStateChanged

We listen to ViewHostMsg_SelectionChanged, and we don't send
ViewHostMsg_SelectionBoundsChanged. It's possible something has an incorrect
assumption on the browser-side about the ordering, but if the tests pass then I
don't think we should go out of our way to preserve the ordering (unless the
ordering doesn't matter for other platforms, in which case it might be nice to
preserve the old order :)

[yukawa, 2015-07-24 20:05:54]

lgtm from the viewpoint of IME support.

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

https://codereview.chromium.org/1249013005/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:211: // Updates the
type of the input method attached to the view.
nit: can we update the comment too?

https://codereview.chromium.org/1249013005/diff/40001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/1249013005/diff/40001/content/renderer/render...
content/renderer/render_view_browsertest.cc:979:
ViewHostMsg_TextInputState_Params p =  base::get<0>(params);
nit: an unnecessary whitespace.

https://codereview.chromium.org/1249013005/diff/40001/content/renderer/render...
content/renderer/render_view_browsertest.cc:1000: p =  base::get<0>(params);
nit: an unnecessary whitespace.

https://codereview.chromium.org/1249013005/diff/40001/content/renderer/render...
content/renderer/render_view_browsertest.cc:1024: p =  base::get<0>(params);
nit: an unnecessary whitespace.

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

https://codereview.chromium.org/1249013005/diff/40001/content/renderer/render...
content/renderer/render_widget.cc:1383: // The following two can result in
further layout and possibly
Can someone make sure that "following two" is still valid?

[Will Harris, 2015-07-24 21:41:06]

view_messages.h IPC looks okay from security. lgtm.

https://codereview.chromium.org/1249013005/diff/40001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/1249013005/diff/40001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.h:51: struct
ViewHostMsg_TextInputState_Params;
no need to fwd declare again here

[Shu Chen, 2015-07-27 01:59:18]

On 2015/07/24 15:10:49, jdduke wrote:
> On 2015/07/24 09:14:05, Shu Chen wrote:
> >
>
https://codereview.chromium.org/1249013005/diff/40001/content/renderer/render...
> > File content/renderer/render_frame_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1249013005/diff/40001/content/renderer/render...
> > content/renderer/render_frame_impl.cc:3024: RenderWidget::NO_SHOW_IME,
> > RenderWidget::FROM_NON_IME);
> > On 2015/07/23 15:12:50, jdduke wrote:
> > > I assume the order relative to SyncSelectionIfRequired doesn't matter?
> > 
> > SyncSelectionIfRequired() sends IPCs to browser for selection info, and
> > UpdateTextInputState() sends IPCs for input field info. I'm not familiar
with
> > Clank. Is Clank sensitive for the sequence of below IPCs?
> >  - ViewHostMsg_SelectionChanged
> >  - ViewHostMsg_SelectionBoundsChanged
> >  - ViewHostMsg_TextInputStateChanged
> 
> We listen to ViewHostMsg_SelectionChanged, and we don't send
> ViewHostMsg_SelectionBoundsChanged. It's possible something has an incorrect
> assumption on the browser-side about the ordering, but if the tests pass then
I
> don't think we should go out of our way to preserve the ordering (unless the
> ordering doesn't matter for other platforms, in which case it might be nice to
> preserve the old order :)

This only changes the order for Android platform.
I think the code comment makes sense and state-changed IPC should be sent before
selection-changed IPC.
Let's change the order for Android and see whether there will be regressions.

[Shu Chen, 2015-07-27 02:01:17]

https://codereview.chromium.org/1249013005/diff/40001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/1249013005/diff/40001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.h:51: struct
ViewHostMsg_TextInputState_Params;
On 2015/07/24 21:41:06, Will Harris wrote:
> no need to fwd declare again here

Done.

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

https://codereview.chromium.org/1249013005/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:211: // Updates the
type of the input method attached to the view.
On 2015/07/24 20:05:53, yukawa wrote:
> nit: can we update the comment too?

Done.

https://codereview.chromium.org/1249013005/diff/40001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/1249013005/diff/40001/content/renderer/render...
content/renderer/render_view_browsertest.cc:979:
ViewHostMsg_TextInputState_Params p =  base::get<0>(params);
On 2015/07/24 20:05:54, yukawa wrote:
> nit: an unnecessary whitespace.

Done.

https://codereview.chromium.org/1249013005/diff/40001/content/renderer/render...
content/renderer/render_view_browsertest.cc:1000: p =  base::get<0>(params);
On 2015/07/24 20:05:53, yukawa wrote:
> nit: an unnecessary whitespace.

Done.

https://codereview.chromium.org/1249013005/diff/40001/content/renderer/render...
content/renderer/render_view_browsertest.cc:1024: p =  base::get<0>(params);
On 2015/07/24 20:05:53, yukawa wrote:
> nit: an unnecessary whitespace.

Done.

[Shu Chen, 2015-07-29 01:46:46]

Pinging...

sievers@/nasko, can you please take a look? Thanks

[nasko, 2015-07-29 17:41:08]

wfh@ already reviewed what you included me for, but I did a quick pass anyway.
I'd defer to someone else to ensure the input events changes make sense.

https://codereview.chromium.org/1249013005/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (left):

https://codereview.chromium.org/1249013005/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:497:
IPC_MESSAGE_HANDLER(ViewHostMsg_SetNeedsBeginFrames,
This IPC seems irrelevant to this CL. Why is it being removed?

https://codereview.chromium.org/1249013005/diff/60001/content/renderer/npapi/...
File content/renderer/npapi/webplugin_delegate_proxy.cc (right):

https://codereview.chromium.org/1249013005/diff/60001/content/renderer/npapi/...
content/renderer/npapi/webplugin_delegate_proxy.cc:914:
ViewHostMsg_TextInputState_Params p;
nit: s/p/params/

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

https://codereview.chromium.org/1249013005/diff/60001/content/renderer/render...
content/renderer/render_widget.cc:1945: ViewHostMsg_TextInputState_Params p;
nit: s/p/params/

[no sievers, 2015-07-30 00:10:58]

https://codereview.chromium.org/1249013005/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (left):

https://codereview.chromium.org/1249013005/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:497:
IPC_MESSAGE_HANDLER(ViewHostMsg_SetNeedsBeginFrames,
On 2015/07/29 17:41:07, nasko wrote:
> This IPC seems irrelevant to this CL. Why is it being removed?

Yea, I think you want to keep OnSetNeedsBeginFrames().

[Shu Chen, 2015-07-30 00:47:22]

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

https://codereview.chromium.org/1249013005/diff/40001/content/renderer/render...
content/renderer/render_widget.cc:1383: // The following two can result in
further layout and possibly
On 2015/07/24 20:05:54, yukawa wrote:
> Can someone make sure that "following two" is still valid?

I've updated the comment: s/following two/UpdateTextInputState.

https://codereview.chromium.org/1249013005/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (left):

https://codereview.chromium.org/1249013005/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:497:
IPC_MESSAGE_HANDLER(ViewHostMsg_SetNeedsBeginFrames,
On 2015/07/30 00:10:58, sievers wrote:
> On 2015/07/29 17:41:07, nasko wrote:
> > This IPC seems irrelevant to this CL. Why is it being removed?
> 
> Yea, I think you want to keep OnSetNeedsBeginFrames().

This is a typo of solving conflict when rebase. I've added it back. Thanks for
catching it.

https://codereview.chromium.org/1249013005/diff/60001/content/renderer/npapi/...
File content/renderer/npapi/webplugin_delegate_proxy.cc (right):

https://codereview.chromium.org/1249013005/diff/60001/content/renderer/npapi/...
content/renderer/npapi/webplugin_delegate_proxy.cc:914:
ViewHostMsg_TextInputState_Params p;
On 2015/07/29 17:41:07, nasko wrote:
> nit: s/p/params/

Done.

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

https://codereview.chromium.org/1249013005/diff/60001/content/renderer/render...
content/renderer/render_widget.cc:1945: ViewHostMsg_TextInputState_Params p;
On 2015/07/29 17:41:08, nasko wrote:
> nit: s/p/params/

Done.

[no sievers, 2015-07-30 17:11:20]

lgtm.

Btw putting the rebase in a separate patch set does make reviewing deltas easier
;)

[Shu Chen, 2015-07-31 03:18:31]

The CQ bit was checked by shuchen@chromium.org

[Shu Chen, 2015-07-31 03:18:31]

The patchset sent to the CQ was uploaded after l-g-t-m from jdduke@chromium.org,
wfh@chromium.org, yukawa@chromium.org
Link to the patchset: https://codereview.chromium.org/1249013005/#ps80001
(title: "addressed comments.")

[commit-bot: I haz the power, 2015-07-31 03:18:52]

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

[commit-bot: I haz the power, 2015-07-31 04:22:23]

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

[commit-bot: I haz the power, 2015-07-31 04:22:25]

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

[Shu Chen, 2015-07-31 04:46:49]

The CQ bit was checked by shuchen@chromium.org

[commit-bot: I haz the power, 2015-07-31 04:47:11]

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

[commit-bot: I haz the power, 2015-07-31 06:17:47]

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

[commit-bot: I haz the power, 2015-07-31 06:17:49]

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

[Shu Chen, 2015-07-31 10:48:53]

The CQ bit was checked by shuchen@chromium.org

[commit-bot: I haz the power, 2015-07-31 10:49:01]

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

[commit-bot: I haz the power, 2015-07-31 11:36:54]

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

[commit-bot: I haz the power, 2015-07-31 11:36:55]

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

[Shu Chen, 2015-07-31 12:23:54]

The CQ bit was checked by shuchen@chromium.org

[commit-bot: I haz the power, 2015-07-31 12:24:22]

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

[commit-bot: I haz the power, 2015-07-31 13:23:04]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-07-31 13:23:40]

Patchset 5 (id:??) landed as
https://crrev.com/913f8b627c442721376c7ae6c5ceaf33f684a434
Cr-Commit-Position: refs/heads/master@{#341324}