Refactors ui::internal::InputMethodDelegate.

Refactors ui::internal::InputMethodDelegate.

Simplify and combine Dispatch{,Fabricated}KeyEventPostIME methods into one method.

TEST=Run unit_tests, ui_unittests, views_unittests.
TBR=pfeldman@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247947

[Yuki, 2014-01-27 13:02:47]

Could you review this CL?

[Hiro Komatsu, 2014-01-28 06:26:36]

lgtm

https://codereview.chromium.org/142043006/diff/100001/ui/views/corewm/input_m...
File ui/views/corewm/input_method_event_filter.cc (left):

https://codereview.chromium.org/142043006/diff/100001/ui/views/corewm/input_m...
ui/views/corewm/input_method_event_filter.cc:63: DCHECK(event.message !=
WM_CHAR);
I'm not sure it is OK to delete this line.

[Yuki, 2014-01-28 07:18:01]

Hi Sadrul, could you review this CL as an owner of ui/views/corewm?

https://codereview.chromium.org/142043006/diff/100001/ui/views/corewm/input_m...
File ui/views/corewm/input_method_event_filter.cc (left):

https://codereview.chromium.org/142043006/diff/100001/ui/views/corewm/input_m...
ui/views/corewm/input_method_event_filter.cc:63: DCHECK(event.message !=
WM_CHAR);
On 2014/01/28 06:26:37, Hiro Komatsu wrote:
> I'm not sure it is OK to delete this line.

Good catch.  Added it back.

[sadrul, 2014-01-28 07:29:08]

ui/views/ lgtm with the nit:

https://codereview.chromium.org/142043006/diff/130001/ui/views/corewm/input_m...
File ui/views/corewm/input_method_event_filter.cc (right):

https://codereview.chromium.org/142043006/diff/130001/ui/views/corewm/input_m...
ui/views/corewm/input_method_event_filter.cc:69: } else {
Don't need the else. Generally, if you have:

  if (...) {
    <abc>
    return ...;
  } else {
    <xyz>
    return ...;
  }

Then you should just do:

  if (...) {
    <abc>
    return ...;
  }
  <xyz>
  return ...;

instead.

[Yuki, 2014-01-28 07:39:13]

Thanks for the review.

https://codereview.chromium.org/142043006/diff/130001/ui/views/corewm/input_m...
File ui/views/corewm/input_method_event_filter.cc (right):

https://codereview.chromium.org/142043006/diff/130001/ui/views/corewm/input_m...
ui/views/corewm/input_method_event_filter.cc:69: } else {
On 2014/01/28 07:29:08, sadrul wrote:
> Don't need the else. Generally, if you have:
> 
>   if (...) {
>     <abc>
>     return ...;
>   } else {
>     <xyz>
>     return ...;
>   }
> 
> Then you should just do:
> 
>   if (...) {
>     <abc>
>     return ...;
>   }
>   <xyz>
>   return ...;
> 
> instead.

Done.

[commit-bot: I haz the power, 2014-01-28 18:22:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yukishiino@chromium.org/142043006/150001

[commit-bot: I haz the power, 2014-01-28 19:43:49]

Retried try job too often on linux_rel for step(s) cacheinvalidation_unittests,
cc_unittests, check_deps, check_deps2git, chromedriver_unittests,
components_unittests, content_browsertests, content_unittests, crypto_unittests,
google_apis_unittests, gpu_unittests, ipc_tests, jingle_unittests,
media_unittests, nacl_integration, ppapi_unittests, printing_unittests,
remoting_unittests, sandbox_linux_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, telemetry_perf_unittests,
telemetry_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-29 02:53:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yukishiino@chromium.org/142043006/150001

[commit-bot: I haz the power, 2014-01-29 05:00:09]

Retried try job too often on win_x64_rel for step(s) base_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...

[Yuki, 2014-01-29 07:30:19]

It turned out that there are more clients of ui::internal::InputMethodDelegate. 
Let me add reviewers.

ben@
  as an owner of mojo/examples/launcher/launcher.cc

pfeldman@
  as an owner of content/shell/browser/shell_platform_data_aura.cc

sadrul@
  as an owner of ui/events/
  As it turned out there are more two clients, I'd like to add a new ctor in
TranslatedKeyEvent.  This change makes the code simpler and more efficient.

[Ben Goodger (Google), 2014-01-29 16:03:37]

mojo launcher lgtm

[sadrul, 2014-01-30 06:49:08]

This CL LGTM

I have filed a bug for looking into merging TranslatedKeyEvent into KeyEvent
(http://crbug.com/339355), since it doesn't feel like we really need both of
them.

[Yuki, 2014-01-30 06:56:33]

On 2014/01/30 06:49:08, sadrul wrote:
> This CL LGTM
> 
> I have filed a bug for looking into merging TranslatedKeyEvent into KeyEvent
> (http://crbug.com/339355), since it doesn't feel like we really need both of
> them.

Thanks!

Since the change in content/shell/browser/shell_platform_data_aura.cc is the
same as mojo/examples/launcher/launcher.cc , and I believe there shouldn't be a
critical issue.  Let me commit this CL with a TBR.

[commit-bot: I haz the power, 2014-01-30 07:00:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yukishiino@chromium.org/142043006/220001

[commit-bot: I haz the power, 2014-01-30 08:41:17]

Retried try job too often on android_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2014-01-30 09:47:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yukishiino@chromium.org/142043006/220001

[commit-bot: I haz the power, 2014-01-30 13:38:57]

Change committed as 247947