Add MockInputMethodManager under ui/base/ime/chromeos/

Add MockInputMethodManager under ui/base/ime/chromeos/.
Thus we could use MockInputMethodManager from ash/common/system/chromeos/. 
See https://codereview.chromium.org/2599743003/.

Rename MockInputMethodManager (under chrome/browser/chromeos/input_method/) as MockInputMethodManagerImpl
and make it extends MockInputMethodManager.


BUG=676568
TEST=None

Review-Url: https://codereview.chromium.org/2605843002
Cr-Commit-Position: refs/heads/master@{#441819}
Committed: https://chromium.googlesource.com/chromium/src/+/045de38a389fa2f65d4e703c910d25438a8bde1f

[Azure Wei, 2016-12-28 07:58:40]

Description was changed from

==========
Mock inputmethodmanager.

BUG=676568
TEST=None.
==========

to

==========
Add MockInputMethodManager under ui/base/ime/chromeos/.

Rename MockInputMethodManager (under chrome/browser/chromeos/input_method/) as
MockInputMethodManagerImpl and
extends MockInputMethodManager.

BUG=676568
TEST=None.
==========

[Azure Wei, 2016-12-28 07:58:45]

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

[commit-bot: I haz the power, 2016-12-28 07:58:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Azure Wei, 2016-12-28 08:55:50]

Description was changed from

==========
Add MockInputMethodManager under ui/base/ime/chromeos/.

Rename MockInputMethodManager (under chrome/browser/chromeos/input_method/) as
MockInputMethodManagerImpl and
extends MockInputMethodManager.

BUG=676568
TEST=None.
==========

to

==========
Add MockInputMethodManager under ui/base/ime/chromeos/.
Thus we could use MockInputMethodManager from ash/common/system/chromeos/. 
See https://codereview.chromium.org/2599743003/.

Rename MockInputMethodManager (under chrome/browser/chromeos/input_method/) as
MockInputMethodManagerImpl
and make it extends MockInputMethodManager.


BUG=676568
TEST=None
==========

[Azure Wei, 2016-12-28 08:55:51]

azurewei@chromium.org changed reviewers:
+ shuchen@chromium.org, sky@chromium.org, stevenjb@chromium.org

[Azure Wei, 2016-12-28 08:56:07]

Please review this CL. Thanks!

[commit-bot: I haz the power, 2016-12-28 08:59:29]

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

[commit-bot: I haz the power, 2016-12-28 08:59:30]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2016-12-28 18:28:13]

Thanks for doing this. I will let shuchen@ review this as the owner of
input_method/, just a couple of suggestions from me.

https://codereview.chromium.org/2605843002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/mock_input_method_manager_impl.cc
(right):

https://codereview.chromium.org/2605843002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/mock_input_method_manager_impl.cc:83: }
Any trivial methods like this should just be part of MockInputMethodManager.

https://codereview.chromium.org/2605843002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/mock_input_method_manager_impl.h
(right):

https://codereview.chromium.org/2605843002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/mock_input_method_manager_impl.h:11:
#include "chrome/browser/chromeos/input_method/input_method_util.h"
FWIW, this appears to be the only Chrome dependency, and it only has two chrome
dependencies which could pretty easily be removed. I hate to pile on
dependencies, but I suspect we are going to want to move that at some point
anyway.

If we don't want to do that just now, this could be forward declared if we use a
scoped_ptr for InputMethodUtil.

[Azure Wei, 2017-01-03 06:05:52]

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

[commit-bot: I haz the power, 2017-01-03 06:05:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-03 07:06:58]

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

[commit-bot: I haz the power, 2017-01-03 07:06:59]

Dry run: This issue passed the CQ dry run.

[Azure Wei, 2017-01-03 09:56:22]

https://codereview.chromium.org/2605843002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/mock_input_method_manager_impl.cc
(right):

https://codereview.chromium.org/2605843002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/mock_input_method_manager_impl.cc:83: }
On 2016/12/28 18:28:13, stevenjb wrote:
> Any trivial methods like this should just be part of MockInputMethodManager.

Done.

https://codereview.chromium.org/2605843002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/mock_input_method_manager_impl.h
(right):

https://codereview.chromium.org/2605843002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/mock_input_method_manager_impl.h:11:
#include "chrome/browser/chromeos/input_method/input_method_util.h"
On 2016/12/28 18:28:13, stevenjb wrote:
> FWIW, this appears to be the only Chrome dependency, and it only has two
chrome
> dependencies which could pretty easily be removed. I hate to pile on
> dependencies, but I suspect we are going to want to move that at some point
> anyway.
> 
> If we don't want to do that just now, this could be forward declared if we use
a
> scoped_ptr for InputMethodUtil.

Done. Used unique_ptr and forward declared for InputMethodUtil.

[sky, 2017-01-03 16:29:47]

https://codereview.chromium.org/2605843002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/events/event_rewriter_unittest.cc (right):

https://codereview.chromium.org/2605843002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/events/event_rewriter_unittest.cc:132:
input_method_manager_mock_);  // pass ownership
Would be nice if this took a unique_ptr so ownership was clear.

https://codereview.chromium.org/2605843002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/mock_input_method_manager_impl.cc
(right):

https://codereview.chromium.org/2605843002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/mock_input_method_manager_impl.cc:41:
std::unique_ptr<InputMethodDescriptors> result(new InputMethodDescriptors);
Use make_unique if possible (see threads on chromium-dev for why).

https://codereview.chromium.org/2605843002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/mock_input_method_manager_impl.h
(right):

https://codereview.chromium.org/2605843002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/mock_input_method_manager_impl.h:1: //
Copyright (c) 2016 The Chromium Authors. All rights reserved.
no (c)

https://codereview.chromium.org/2605843002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/mock_input_method_manager_impl.h:28:
scoped_refptr<InputMethodManager::State> Clone() const override;
Prefix overrides with where they are overriden from (as you do on line 50
below).

https://codereview.chromium.org/2605843002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/mock_input_method_manager_impl.h:44:
MockInputMethodManager* const manager_;
Style guide says no protected members.

https://codereview.chromium.org/2605843002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/mock_input_method_manager_impl.h:77: int
add_observer_count_;
No public/protected members.

https://codereview.chromium.org/2605843002/diff/60001/ui/base/ime/BUILD.gn
File ui/base/ime/BUILD.gn (right):

https://codereview.chromium.org/2605843002/diff/60001/ui/base/ime/BUILD.gn#ne...
ui/base/ime/BUILD.gn:51: "chromeos/mock_input_method_manager.cc",
Is it expected the that mock code is used in production? I would expect it to be
test only.

https://codereview.chromium.org/2605843002/diff/60001/ui/base/ime/chromeos/mo...
File ui/base/ime/chromeos/mock_input_method_manager.cc (right):

https://codereview.chromium.org/2605843002/diff/60001/ui/base/ime/chromeos/mo...
ui/base/ime/chromeos/mock_input_method_manager.cc:13:
MockInputMethodManager::State::~State() {}
Declaration/definition order should match.

https://codereview.chromium.org/2605843002/diff/60001/ui/base/ime/chromeos/mo...
ui/base/ime/chromeos/mock_input_method_manager.cc:30: return 0;
How come you're making this return 0 vs the old of returning an
activeinputmethod?

https://codereview.chromium.org/2605843002/diff/60001/ui/base/ime/chromeos/mo...
File ui/base/ime/chromeos/mock_input_method_manager.h (right):

https://codereview.chromium.org/2605843002/diff/60001/ui/base/ime/chromeos/mo...
ui/base/ime/chromeos/mock_input_method_manager.h:1: // Copyright (c) 2016 The
Chromium Authors. All rights reserved.
optional: I wouldn't update the date as this is a move, but if you do make it
2017.

https://codereview.chromium.org/2605843002/diff/60001/ui/base/ime/chromeos/mo...
ui/base/ime/chromeos/mock_input_method_manager.h:62: };
private: DISALLOW?

[stevenjb, 2017-01-03 18:27:16]

Thanks! lgtm (w/ cleanup feedback from sky@)

[Shu Chen, 2017-01-04 00:48:47]

lgtm & pls address sky@'s comments.

[Azure Wei, 2017-01-04 05:42:07]

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

[commit-bot: I haz the power, 2017-01-04 05:42:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-04 06:34:59]

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

[commit-bot: I haz the power, 2017-01-04 06:35:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Azure Wei, 2017-01-04 07:17:44]

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

[commit-bot: I haz the power, 2017-01-04 07:17:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-04 08:23:20]

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

[commit-bot: I haz the power, 2017-01-04 08:23:21]

Dry run: This issue passed the CQ dry run.

[Azure Wei, 2017-01-04 09:13:27]

https://codereview.chromium.org/2605843002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/events/event_rewriter_unittest.cc (right):

https://codereview.chromium.org/2605843002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/events/event_rewriter_unittest.cc:132:
input_method_manager_mock_);  // pass ownership
On 2017/01/03 16:29:46, sky wrote:
> Would be nice if this took a unique_ptr so ownership was clear.

Do you mean make input_method_manager_mock_ as
std::unique_ptr<MockInputMethodManagerImpl> and pass
input_method_manager_mock_.release() here?
input_method_manager_mock_ is not a real unique_ptr since it will be changed in
TestRewriteCapsLockMod3InUse.

https://codereview.chromium.org/2605843002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/mock_input_method_manager_impl.cc
(right):

https://codereview.chromium.org/2605843002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/mock_input_method_manager_impl.cc:41:
std::unique_ptr<InputMethodDescriptors> result(new InputMethodDescriptors);
On 2017/01/03 16:29:46, sky wrote:
> Use make_unique if possible (see threads on chromium-dev for why).

Done.

https://codereview.chromium.org/2605843002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/mock_input_method_manager_impl.h
(right):

https://codereview.chromium.org/2605843002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/mock_input_method_manager_impl.h:1: //
Copyright (c) 2016 The Chromium Authors. All rights reserved.
On 2017/01/03 16:29:47, sky wrote:
> no (c)

Done.

https://codereview.chromium.org/2605843002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/mock_input_method_manager_impl.h:28:
scoped_refptr<InputMethodManager::State> Clone() const override;
On 2017/01/03 16:29:46, sky wrote:
> Prefix overrides with where they are overriden from (as you do on line 50
> below).

Done.

https://codereview.chromium.org/2605843002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/mock_input_method_manager_impl.h:44:
MockInputMethodManager* const manager_;
On 2017/01/03 16:29:47, sky wrote:
> Style guide says no protected members.

Made it private.

https://codereview.chromium.org/2605843002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/mock_input_method_manager_impl.h:77: int
add_observer_count_;
On 2017/01/03 16:29:47, sky wrote:
> No public/protected members.

Made them private.

https://codereview.chromium.org/2605843002/diff/60001/ui/base/ime/BUILD.gn
File ui/base/ime/BUILD.gn (right):

https://codereview.chromium.org/2605843002/diff/60001/ui/base/ime/BUILD.gn#ne...
ui/base/ime/BUILD.gn:51: "chromeos/mock_input_method_manager.cc",
On 2017/01/03 16:29:47, sky wrote:
> Is it expected the that mock code is used in production? I would expect it to
be
> test only.

Moved them to ui/base:test_support.

https://codereview.chromium.org/2605843002/diff/60001/ui/base/ime/chromeos/mo...
File ui/base/ime/chromeos/mock_input_method_manager.cc (right):

https://codereview.chromium.org/2605843002/diff/60001/ui/base/ime/chromeos/mo...
ui/base/ime/chromeos/mock_input_method_manager.cc:13:
MockInputMethodManager::State::~State() {}
On 2017/01/03 16:29:47, sky wrote:
> Declaration/definition order should match.

Done.

https://codereview.chromium.org/2605843002/diff/60001/ui/base/ime/chromeos/mo...
ui/base/ime/chromeos/mock_input_method_manager.cc:30: return 0;
On 2017/01/03 16:29:47, sky wrote:
> How come you're making this return 0 vs the old of returning an
> activeinputmethod?

Oh, this could just return active_input_method_ids.size().

https://codereview.chromium.org/2605843002/diff/60001/ui/base/ime/chromeos/mo...
File ui/base/ime/chromeos/mock_input_method_manager.h (right):

https://codereview.chromium.org/2605843002/diff/60001/ui/base/ime/chromeos/mo...
ui/base/ime/chromeos/mock_input_method_manager.h:1: // Copyright (c) 2016 The
Chromium Authors. All rights reserved.
On 2017/01/03 16:29:47, sky wrote:
> optional: I wouldn't update the date as this is a move, but if you do make it
> 2017.

Reverted this change.

https://codereview.chromium.org/2605843002/diff/60001/ui/base/ime/chromeos/mo...
ui/base/ime/chromeos/mock_input_method_manager.h:62: };
On 2017/01/03 16:29:47, sky wrote:
> private: DISALLOW?

Done.

[sky, 2017-01-04 16:35:52]

https://codereview.chromium.org/2605843002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/events/event_rewriter_unittest.cc (right):

https://codereview.chromium.org/2605843002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/events/event_rewriter_unittest.cc:132:
input_method_manager_mock_);  // pass ownership
On 2017/01/04 09:13:27, Azure Wei wrote:
> On 2017/01/03 16:29:46, sky wrote:
> > Would be nice if this took a unique_ptr so ownership was clear.
> 
> Do you mean make input_method_manager_mock_ as
> std::unique_ptr<MockInputMethodManagerImpl> and pass
> input_method_manager_mock_.release() here?
> input_method_manager_mock_ is not a real unique_ptr since it will be changed
in
> TestRewriteCapsLockMod3InUse.

This is what you have:

chromeos::input_method::InitializeForTesting(
    input_method_manager_mock_);  // pass ownership

You need the comment because it isn't obvious from the code that
InitializeForTesting takes ownership. If InitializeForTesting took a
std::unique_ptr, ownership would be obvious.

I'm not sure what you mean by changing to TestRewriteCapsLockMod3InUse.

https://codereview.chromium.org/2605843002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/input_method/mock_input_method_manager_impl.cc
(right):

https://codereview.chromium.org/2605843002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/input_method/mock_input_method_manager_impl.cc:11: #if
_LIBCPP_STD_VER > 11
Why do you need this?

https://codereview.chromium.org/2605843002/diff/120001/ui/base/BUILD.gn
File ui/base/BUILD.gn (right):

https://codereview.chromium.org/2605843002/diff/120001/ui/base/BUILD.gn#newco...
ui/base/BUILD.gn:633: "ime/chromeos/mock_input_method_manager.cc",
Shouldn't you only compile these files on chromeos?

https://codereview.chromium.org/2605843002/diff/120001/ui/base/ime/chromeos/m...
File ui/base/ime/chromeos/mock_input_method_manager.cc (right):

https://codereview.chromium.org/2605843002/diff/120001/ui/base/ime/chromeos/m...
ui/base/ime/chromeos/mock_input_method_manager.cc:1: // Copyright (c) 2016 The
Chromium Authors. All rights reserved.
2017, or don't change. If you do change, remove the (c).

https://codereview.chromium.org/2605843002/diff/120001/ui/base/ime/chromeos/m...
ui/base/ime/chromeos/mock_input_method_manager.cc:11:
MockInputMethodManager::State::State() {}
I would expect all the State functions to be grouped together, probably before
MockInputMethodManager. (again this comes from style guide saying
declaration/definition order should match).

[Azure Wei, 2017-01-05 15:22:06]

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

[commit-bot: I haz the power, 2017-01-05 15:22:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Azure Wei, 2017-01-05 15:41:29]

https://codereview.chromium.org/2605843002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/input_method/mock_input_method_manager_impl.cc
(right):

https://codereview.chromium.org/2605843002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/input_method/mock_input_method_manager_impl.cc:11: #if
_LIBCPP_STD_VER > 11
On 2017/01/04 16:35:52, sky wrote:
> Why do you need this?

I thought this would would be needed by make_unique. Removed.

https://codereview.chromium.org/2605843002/diff/120001/ui/base/BUILD.gn
File ui/base/BUILD.gn (right):

https://codereview.chromium.org/2605843002/diff/120001/ui/base/BUILD.gn#newco...
ui/base/BUILD.gn:633: "ime/chromeos/mock_input_method_manager.cc",
On 2017/01/04 16:35:52, sky wrote:
> Shouldn't you only compile these files on chromeos?

Added these under condition is_chromeos.

https://codereview.chromium.org/2605843002/diff/120001/ui/base/ime/chromeos/m...
File ui/base/ime/chromeos/mock_input_method_manager.cc (right):

https://codereview.chromium.org/2605843002/diff/120001/ui/base/ime/chromeos/m...
ui/base/ime/chromeos/mock_input_method_manager.cc:1: // Copyright (c) 2016 The
Chromium Authors. All rights reserved.
On 2017/01/04 16:35:52, sky wrote:
> 2017, or don't change. If you do change, remove the (c).

Reverted this change.

https://codereview.chromium.org/2605843002/diff/120001/ui/base/ime/chromeos/m...
ui/base/ime/chromeos/mock_input_method_manager.cc:11:
MockInputMethodManager::State::State() {}
On 2017/01/04 16:35:52, sky wrote:
> I would expect all the State functions to be grouped together, probably before
> MockInputMethodManager. (again this comes from style guide saying
> declaration/definition order should match).

Reordered these method definitions.

[commit-bot: I haz the power, 2017-01-05 16:29:55]

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

[commit-bot: I haz the power, 2017-01-05 16:29:56]

Dry run: This issue passed the CQ dry run.

[sky, 2017-01-05 17:23:16]

LGTM

[Azure Wei, 2017-01-06 00:31:43]

The CQ bit was checked by azurewei@chromium.org

[Azure Wei, 2017-01-06 00:31:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
shuchen@chromium.org, stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2605843002/#ps140001
(title: "Addressed comments.")

[commit-bot: I haz the power, 2017-01-06 00:32:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 02:06:04]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1483662703247680,
"parent_rev": "ae0ddfaace8dd20347b3898655f0209583ad8b96", "commit_rev":
"045de38a389fa2f65d4e703c910d25438a8bde1f"}

[commit-bot: I haz the power, 2017-01-06 02:06:34]

Description was changed from

==========
Add MockInputMethodManager under ui/base/ime/chromeos/.
Thus we could use MockInputMethodManager from ash/common/system/chromeos/. 
See https://codereview.chromium.org/2599743003/.

Rename MockInputMethodManager (under chrome/browser/chromeos/input_method/) as
MockInputMethodManagerImpl
and make it extends MockInputMethodManager.


BUG=676568
TEST=None
==========

to

==========
Add MockInputMethodManager under ui/base/ime/chromeos/.
Thus we could use MockInputMethodManager from ash/common/system/chromeos/. 
See https://codereview.chromium.org/2599743003/.

Rename MockInputMethodManager (under chrome/browser/chromeos/input_method/) as
MockInputMethodManagerImpl
and make it extends MockInputMethodManager.


BUG=676568
TEST=None

Review-Url: https://codereview.chromium.org/2605843002
Cr-Commit-Position: refs/heads/master@{#441819}
Committed:
https://chromium.googlesource.com/chromium/src/+/045de38a389fa2f65d4e703c910d...
==========

[commit-bot: I haz the power, 2017-01-06 02:06:36]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/045de38a389fa2f65d4e703c910d...