Make InputMethodEngine to inherit from new added class InputMethodEngineBase.

Make InputMethodEngine to inherit from new added class InputMethodEngineBase.

This cl try to add an base class input_method::InputMethodEngineBase. And then the current class chromeos::InputMethodEngine (for Chrome OS) and future added new class input_method::InputMethodEgnine (for Linux and Windows) can share the common codes.

BUG=517773
TEST=No compiling errors.

Committed: https://crrev.com/ecf6567e6518e6e9944993e62ed767dc02d56274
Cr-Commit-Position: refs/heads/master@{#369293}

[Azure Wei, 2016-01-06 09:10:39]

Description was changed from

==========
Make InputMethodEngine to inherit from new added class
InputMethodEngineInterface.

BUG=517773
TEST=No compiling errors.
==========

to

==========
Make InputMethodEngine to inherit from new added class
InputMethodEngineInterface.

This cl trys to add an base class input_method::InputMethodEngineInterface. And
then the current class chromeos::InputMethodEngine (for Chrome OS) and future
added new class input_method::InputMethodEgnine (for Linux and Windows) can
share the common codes. 

BUG=517773
TEST=No compiling errors.
==========

[Azure Wei, 2016-01-06 09:10:39]

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

[Azure Wei, 2016-01-06 09:13:37]

Hi Shu Chen and sky@, can you please help review this cl? Thank you!

This cl is part of : https://codereview.chromium.org/1528483002/, which will
adds the input_method::InputMethodEngine class.

[Shu Chen, 2016-01-06 20:36:10]

https://codereview.chromium.org/1562733002/diff/40001/chrome/browser/input_me...
File chrome/browser/input_method/input_method_engine_interface.cc (right):

https://codereview.chromium.org/1562733002/diff/40001/chrome/browser/input_me...
chrome/browser/input_method/input_method_engine_interface.cc:107: // ui::VKEY_F6
and ui::VKEY_F7 are unassigned on Windows.
I don't understand this, can you please elaborate more?

https://codereview.chromium.org/1562733002/diff/40001/chrome/browser/input_me...
chrome/browser/input_method/input_method_engine_interface.cc:114: #endif  //!
defined(OS_WIN)
// !defined(OS_WIN)

[Azure Wei, 2016-01-07 03:03:13]

https://codereview.chromium.org/1562733002/diff/40001/chrome/browser/input_me...
File chrome/browser/input_method/input_method_engine_interface.cc (right):

https://codereview.chromium.org/1562733002/diff/40001/chrome/browser/input_me...
chrome/browser/input_method/input_method_engine_interface.cc:107: // ui::VKEY_F6
and ui::VKEY_F7 are unassigned on Windows.
On 2016/01/06 20:36:10, Shu Chen wrote:
> I don't understand this, can you please elaborate more?

Sorry, just VKEY_BRIGHTNESS_DOWN/VKEY_BRIGHTNESS_UP are not defined on Windows.
I've changed this function with ui::KeycodeConverter::DomKeyToKeyString() on
Linux and Windows.

https://codereview.chromium.org/1562733002/diff/40001/chrome/browser/input_me...
chrome/browser/input_method/input_method_engine_interface.cc:114: #endif  //!
defined(OS_WIN)
On 2016/01/06 20:36:10, Shu Chen wrote:
> // !defined(OS_WIN)

Done.

[Shu Chen, 2016-01-07 03:06:13]

lgtm.

Just a minor comment, InputMethodEngineInterface is not just an interface, it
should be better to rename it as InputMethodEngineBase.

[Azure Wei, 2016-01-07 05:47:14]

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

[Azure Wei, 2016-01-07 05:47:15]

The CQ bit was unchecked by azurewei@chromium.org

[Azure Wei, 2016-01-07 09:14:16]

On 2016/01/07 03:06:13, Shu Chen wrote:
> lgtm.
> 
> Just a minor comment, InputMethodEngineInterface is not just an interface, it
> should be better to rename it as InputMethodEngineBase.

Done.

[sky, 2016-01-07 16:22:22]

What files do you need me to review?

[Azure Wei, 2016-01-08 01:52:13]

On 2016/01/07 16:22:22, sky wrote:
> What files do you need me to review?

Please help review the two new added files:
chrome/browser/input_method/input_method_engine_base.h
chrome/browser/input_method/input_method_engine_base.cc
Actually, most of the codes are just copy-paste from the original
chrome/browser/chromeos/input_method/input_method_engine.h/.cc
Thank you!

[Azure Wei, 2016-01-08 02:30:11]

On 2016/01/08 01:52:13, Azure Wei wrote:
> On 2016/01/07 16:22:22, sky wrote:
> > What files do you need me to review?
> 
> Please help review the two new added files:
> chrome/browser/input_method/input_method_engine_base.h
> chrome/browser/input_method/input_method_engine_base.cc
> Actually, most of the codes are just copy-paste from the original
> chrome/browser/chromeos/input_method/input_method_engine.h/.cc
> Thank you!

I just added a new OWNERS file under chrome/browser/input_method/ in Patch Set
6, please help to look at this file too. Thank you!

[sky, 2016-01-08 16:35:28]

https://codereview.chromium.org/1562733002/diff/100001/chrome/browser/input_m...
File chrome/browser/input_method/input_method_engine_base.cc (right):

https://codereview.chromium.org/1562733002/diff/100001/chrome/browser/input_m...
chrome/browser/input_method/input_method_engine_base.cc:318: if
(!CheckProfile())
Why do we need all these CheckProfile functions? If InputMethodEngine is profile
specific, shouldn't the appropriate InputMethodEngine be enabled as appropriate?

https://codereview.chromium.org/1562733002/diff/100001/chrome/browser/input_m...
File chrome/browser/input_method/input_method_engine_base.h (right):

https://codereview.chromium.org/1562733002/diff/100001/chrome/browser/input_m...
chrome/browser/input_method/input_method_engine_base.h:28: class
InputMethodEngineBase : virtual public ui::IMEEngineHandlerInterface {
Why are you using virtual?

https://codereview.chromium.org/1562733002/diff/100001/chrome/browser/input_m...
chrome/browser/input_method/input_method_engine_base.h:72: virtual bool
CheckProfile() const = 0;
Functions (just like members) should have descriptions.

[Azure Wei, 2016-01-10 08:19:30]

https://codereview.chromium.org/1562733002/diff/100001/chrome/browser/input_m...
File chrome/browser/input_method/input_method_engine_base.cc (right):

https://codereview.chromium.org/1562733002/diff/100001/chrome/browser/input_m...
chrome/browser/input_method/input_method_engine_base.cc:318: if
(!CheckProfile())
On 2016/01/08 16:35:27, sky wrote:
> Why do we need all these CheckProfile functions? If InputMethodEngine is
profile
> specific, shouldn't the appropriate InputMethodEngine be enabled as
appropriate?

I think CheckProfile() was mainly used for Disable() function. When switching
the log-in account in ChromeOS, a different InputMethodEngine could be enabled
if the two accounts selected two different input methods. The previous
InputMethodEngine::Disable() will be called, but we don't want to clear the
active component id for that profile. The CheckProfile() will return false to
not run the following codes.

I am not quite sure whether other functions need to call CheckProfile(), as I'm
just trying to keep the ChromeOS-specific codes the same as before. And I think
shuchen@ should know better about these functions.

This logic will be different on Linux and Windows desktop, as there will be only
one active input extension all the time. So I declared CheckProfile() as a
virtual function in InputMethodEngineBase.

https://codereview.chromium.org/1562733002/diff/100001/chrome/browser/input_m...
File chrome/browser/input_method/input_method_engine_base.h (right):

https://codereview.chromium.org/1562733002/diff/100001/chrome/browser/input_m...
chrome/browser/input_method/input_method_engine_base.h:28: class
InputMethodEngineBase : virtual public ui::IMEEngineHandlerInterface {
On 2016/01/08 16:35:27, sky wrote:
> Why are you using virtual?

This is because InputMethodEngineBase didn't override all the virtual functions
in IMEEngineHandlerInterface, like SendKeyEvents(), which will be differently
implemented on ChromeOS/Linux/Windows.

https://codereview.chromium.org/1562733002/diff/100001/chrome/browser/input_m...
chrome/browser/input_method/input_method_engine_base.h:72: virtual bool
CheckProfile() const = 0;
On 2016/01/08 16:35:27, sky wrote:
> Functions (just like members) should have descriptions.

Done.

[Azure Wei, 2016-01-11 05:28:11]

Description was changed from

==========
Make InputMethodEngine to inherit from new added class
InputMethodEngineInterface.

This cl trys to add an base class input_method::InputMethodEngineInterface. And
then the current class chromeos::InputMethodEngine (for Chrome OS) and future
added new class input_method::InputMethodEgnine (for Linux and Windows) can
share the common codes. 

BUG=517773
TEST=No compiling errors.
==========

to

==========
Make InputMethodEngine to inherit from new added class InputMethodEngineBase.

This cl try to add an base class input_method::InputMethodEngineBase. And then
the current class chromeos::InputMethodEngine (for Chrome OS) and future added
new class input_method::InputMethodEgnine (for Linux and Windows) can share the
common codes. 

BUG=517773
TEST=No compiling errors.
==========

[sky, 2016-01-11 16:30:14]

https://codereview.chromium.org/1562733002/diff/100001/chrome/browser/input_m...
File chrome/browser/input_method/input_method_engine_base.cc (right):

https://codereview.chromium.org/1562733002/diff/100001/chrome/browser/input_m...
chrome/browser/input_method/input_method_engine_base.cc:318: if
(!CheckProfile())
On 2016/01/10 08:19:30, Azure Wei wrote:
> On 2016/01/08 16:35:27, sky wrote:
> > Why do we need all these CheckProfile functions? If InputMethodEngine is
> profile
> > specific, shouldn't the appropriate InputMethodEngine be enabled as
> appropriate?
> 
> I think CheckProfile() was mainly used for Disable() function. When switching
> the log-in account in ChromeOS, a different InputMethodEngine could be enabled
> if the two accounts selected two different input methods. The previous
> InputMethodEngine::Disable() will be called, but we don't want to clear the
> active component id for that profile. The CheckProfile() will return false to
> not run the following codes.

Not sure I follow. Shouldn't there only be one inputmethod enabled at a time?

> 
> I am not quite sure whether other functions need to call CheckProfile(), as
I'm
> just trying to keep the ChromeOS-specific codes the same as before. And I
think
> shuchen@ should know better about these functions.
> 
> This logic will be different on Linux and Windows desktop, as there will be
only
> one active input extension all the time. So I declared CheckProfile() as a
> virtual function in InputMethodEngineBase.

[Azure Wei, 2016-01-13 12:35:25]

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

[commit-bot: I haz the power, 2016-01-13 12:35:38]

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

[Azure Wei, 2016-01-13 13:07:34]

The CQ bit was unchecked by azurewei@chromium.org

[Azure Wei, 2016-01-13 13:52:39]

On 2016/01/11 16:30:14, sky wrote:
>
https://codereview.chromium.org/1562733002/diff/100001/chrome/browser/input_m...
> File chrome/browser/input_method/input_method_engine_base.cc (right):
> 
>
https://codereview.chromium.org/1562733002/diff/100001/chrome/browser/input_m...
> chrome/browser/input_method/input_method_engine_base.cc:318: if
> (!CheckProfile())
> On 2016/01/10 08:19:30, Azure Wei wrote:
> > On 2016/01/08 16:35:27, sky wrote:
> > > Why do we need all these CheckProfile functions? If InputMethodEngine is
> > profile
> > > specific, shouldn't the appropriate InputMethodEngine be enabled as
> > appropriate?
> > 
> > I think CheckProfile() was mainly used for Disable() function. When
switching
> > the log-in account in ChromeOS, a different InputMethodEngine could be
enabled
> > if the two accounts selected two different input methods. The previous
> > InputMethodEngine::Disable() will be called, but we don't want to clear the
> > active component id for that profile. The CheckProfile() will return false
to
> > not run the following codes.
> 
> Not sure I follow. Shouldn't there only be one inputmethod enabled at a time?

Yes, there should be only one input method enabled at a time, and turns out it
doesn't matter the previous input method could get the event when switching
accounts (event with shortcut like Ctrl+Alt+, ). Sorry that I was wrongly
understanding the codes. I removed the CheckProfile() function and verified both
on locally build and real device. Nothing went wrong so far. So I think it's
safe to remove this function.
Please take another look at this CL. Thank you !

[sky, 2016-01-13 17:08:54]

LGTM

[Azure Wei, 2016-01-14 00:51:43]

The CQ bit was checked by azurewei@chromium.org

[Azure Wei, 2016-01-14 00:51:43]

The patchset sent to the CQ was uploaded after l-g-t-m from shuchen@chromium.org
Link to the patchset: https://codereview.chromium.org/1562733002/#ps180001
(title: "Remove function CheckProfile().")

[commit-bot: I haz the power, 2016-01-14 00:55:58]

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

[Azure Wei, 2016-01-14 02:37:27]

The CQ bit was checked by azurewei@chromium.org

[Azure Wei, 2016-01-14 02:37:29]

The CQ bit was unchecked by azurewei@chromium.org

[Azure Wei, 2016-01-14 02:37:46]

The CQ bit was checked by azurewei@chromium.org

[commit-bot: I haz the power, 2016-01-14 02:40:06]

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

[commit-bot: I haz the power, 2016-01-14 02:54:11]

Description was changed from

==========
Make InputMethodEngine to inherit from new added class InputMethodEngineBase.

This cl try to add an base class input_method::InputMethodEngineBase. And then
the current class chromeos::InputMethodEngine (for Chrome OS) and future added
new class input_method::InputMethodEgnine (for Linux and Windows) can share the
common codes. 

BUG=517773
TEST=No compiling errors.
==========

to

==========
Make InputMethodEngine to inherit from new added class InputMethodEngineBase.

This cl try to add an base class input_method::InputMethodEngineBase. And then
the current class chromeos::InputMethodEngine (for Chrome OS) and future added
new class input_method::InputMethodEgnine (for Linux and Windows) can share the
common codes. 

BUG=517773
TEST=No compiling errors.
==========

[commit-bot: I haz the power, 2016-01-14 02:54:12]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-01-14 02:56:24]

Description was changed from

==========
Make InputMethodEngine to inherit from new added class InputMethodEngineBase.

This cl try to add an base class input_method::InputMethodEngineBase. And then
the current class chromeos::InputMethodEngine (for Chrome OS) and future added
new class input_method::InputMethodEgnine (for Linux and Windows) can share the
common codes. 

BUG=517773
TEST=No compiling errors.
==========

to

==========
Make InputMethodEngine to inherit from new added class InputMethodEngineBase.

This cl try to add an base class input_method::InputMethodEngineBase. And then
the current class chromeos::InputMethodEngine (for Chrome OS) and future added
new class input_method::InputMethodEgnine (for Linux and Windows) can share the
common codes.

BUG=517773
TEST=No compiling errors.

Committed: https://crrev.com/ecf6567e6518e6e9944993e62ed767dc02d56274
Cr-Commit-Position: refs/heads/master@{#369293}
==========

[commit-bot: I haz the power, 2016-01-14 02:56:25]

Patchset 10 (id:??) landed as
https://crrev.com/ecf6567e6518e6e9944993e62ed767dc02d56274
Cr-Commit-Position: refs/heads/master@{#369293}