Issue 139803010: Support comma separated hardware keyboard layout.

Issue 139803010: Support comma separated hardware keyboard layout. (Closed)

Created:
6 years, 10 months ago by Seigo Nonaka

Modified:
6 years, 10 months ago

Reviewers:
Alexander Alekseev, Nikita (slow), Hiro Komatsu

CC:
chromium-reviews, extensions-reviews_chromium.org, yusukes+watch_chromium.org, yukishiino+watch_chromium.org, nona+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, nkostylev+watch_chromium.org, Yuki

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Support comma separated hardware keyboard layout value. To be able to use comma-separated keyboard_layout, extends GetHardwareKeyboardLayout and EnableLoginLayouts. This change does not break backward compatibility. BUG=334576 TEST=manually done. Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251653

Patch Set 1 : #

Patch Set 2 : Fix compile failure #

Total comments: 19

Patch Set 3 : Addressing comments #

Total comments: 18

Patch Set 4 : Addressing comments #

Total comments: 2

Patch Set 5 : Addressing comments #

Total comments: 8

Patch Set 6 : Addressing comments #

Total comments: 16

Patch Set 7 : Addressing comments #

Patch Set 8 : Fix wrong condition. #

Total comments: 2

Patch Set 9 : Addressing comments #

Patch Set 10 : /EnableInputMethods/ReplaceEnabeldInputMethods/ #

Created: 6 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+314 lines, -155 lines)			Patch
M	chrome/browser/chromeos/base/locale_util.cc	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/chromeos/chrome_browser_main_chromeos.cc	View	1 2 3 4 5 6 7 8 9	2 chunks	+12 lines, -3 lines	0 comments	Download
M	chrome/browser/chromeos/extensions/input_method_apitest_chromeos.cc	View	1 2	2 chunks	+5 lines, -1 line	0 comments	Download
M	chrome/browser/chromeos/input_method/input_method_delegate_impl.h	View	1 2 3 4 5	1 chunk	+4 lines, -1 line	0 comments	Download
M	chrome/browser/chromeos/input_method/input_method_delegate_impl.cc	View	1 2 3 4 5 6	3 chunks	+22 lines, -11 lines	0 comments	Download
M	chrome/browser/chromeos/input_method/input_method_manager_impl.h	View	1 2 3 4 5 6 7 8 9	2 chunks	+5 lines, -4 lines	0 comments	Download
M	chrome/browser/chromeos/input_method/input_method_manager_impl.cc	View	1 2 3 4 5 6 7 8 9	7 chunks	+49 lines, -33 lines	0 comments	Download
M	chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc	View	1 2 3 4 5 6 7 8 9	33 chunks	+92 lines, -42 lines	0 comments	Download
M	chrome/browser/chromeos/input_method/input_method_util.h	View	1 2 3 4 5 6	3 chunks	+22 lines, -5 lines	0 comments	Download
M	chrome/browser/chromeos/input_method/input_method_util.cc	View	1 2 3 4 5 6 7 8 9	2 chunks	+36 lines, -10 lines	0 comments	Download
M	chrome/browser/chromeos/input_method/mock_input_method_manager.h	View	1 2 3 4 5 6 7 8 9	3 chunks	+5 lines, -5 lines	0 comments	Download
M	chrome/browser/chromeos/input_method/mock_input_method_manager.cc	View	1 2 3 4 5 6 7 8 9	3 chunks	+3 lines, -8 lines	0 comments	Download
M	chrome/browser/chromeos/input_method/mode_indicator_browsertest.cc	View		2 chunks	+8 lines, -2 lines	0 comments	Download
M	chrome/browser/chromeos/login/login_display_host_impl.cc	View	1 2 3 4 5 6 7 8 9	5 chunks	+16 lines, -9 lines	0 comments	Download
M	chrome/browser/chromeos/preferences.cc	View	1 2 3 4 5 6 7 8 9	3 chunks	+8 lines, -7 lines	0 comments	Download
M	chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc	View	1 2 3 4 5 6 7 8 9	1 chunk	+1 line, -1 line	0 comments	Download
M	chromeos/ime/fake_input_method_delegate.h	View	1 2 3 4 5	2 chunks	+5 lines, -1 line	0 comments	Download
M	chromeos/ime/fake_input_method_delegate.cc	View	1 2 3 4 5	2 chunks	+6 lines, -1 line	0 comments	Download
M	chromeos/ime/input_method_delegate.h	View	1 2 3 4 5	1 chunk	+6 lines, -3 lines	0 comments	Download
M	chromeos/ime/input_method_manager.h	View	1 2 3 4 5 6 7 8 9	2 chunks	+8 lines, -7 lines	0 comments	Download

Messages

Total messages: 22 (0 generated)

Expand Messages | Collapse Messages

Seigo Nonaka

Hi Komatsu, Nikita, Could you kindly take a look? I know current InputMethodManager and InputMethodUtil ...

6 years, 10 months ago (2014-02-10 19:05:19 UTC) #1

Alexander Alekseev

Could you please cache values of: vector<string> hardware_input_method_ids; vector<string> hardware_login_input_method_ids; ? So that all GetHardware(Login)InputMethodIds() ...

6 years, 10 months ago (2014-02-11 13:36:01 UTC) #3

Seigo Nonaka

https://codereview.chromium.org/139803010/diff/110001/chrome/browser/chromeos/base/locale_util.cc File chrome/browser/chromeos/base/locale_util.cc (right): https://codereview.chromium.org/139803010/diff/110001/chrome/browser/chromeos/base/locale_util.cc#newcode67 chrome/browser/chromeos/base/locale_util.cc:67: manager->GetInputMethodUtil()->GetHardwareLoginInputMethodId( On 2014/02/11 13:36:02, alemate wrote: > nit: GetHardwareLoginInputMethodId ...

6 years, 10 months ago (2014-02-12 13:21:01 UTC) #4

Alexander Alekseev

I think you can avoid checks for "if(cache_valid)" on every access to the lists of ...

6 years, 10 months ago (2014-02-12 14:45:34 UTC) #5

I think you can avoid checks for "if(cache_valid)" on every access to the lists
of hardware layouts. (And also drop manual call to "invalidate cache"). Consider
this:

// Returns original VPD value.
const string& InputMethodDelegateImpl::GetHardwareKeyboardLayout() const;

// private method 
void InputMethodUtil::UpdateHardwareLayouts() {
    hardware_layouts_.resize(0);
    hardware_login_layouts_.resize(0);
    Tokenize(delegate->GetHardwareKeyboardLayout(), ",", &hardware_layouts_);
    // Filter hardware_layouts_ to hardware_login_layouts_ here.
    ...
    // Ensure both lists are non-empty.
    // (Probably add empty string to them, it they are empty.)
    ...
}

// Call UpdateHardwareLayouts() in InputMethodUtil constructor.

// Create new:
void InputMethodUtil::SetHardwareKeyboardLayoutForTesting(const
string& value) {
  delegate->set_hardware_keyboard_layout(value);
  UpdateHardwareLayouts();
}

// Simple getter
const vector<string>& InputMethodUtil::GetHardwareKeyboardLayout() const {
  DCHECK(!hardware_layouts_.empty());
  return hardware_layouts_;
}

const vector<string>& InputMethodUtil::GetHardwareLoginKeyboardLayout() const {
  DCHECK(!hardware_login_layouts_.empty());
  return hardware_login_layouts_;
}

This way you will have only one copy of hardware layouts, and only update it on
underlying VPD change. 

What do you think?

https://codereview.chromium.org/139803010/diff/110001/chromeos/ime/input_meth...
File chromeos/ime/input_method_delegate.h (right):

https://codereview.chromium.org/139803010/diff/110001/chromeos/ime/input_meth...
chromeos/ime/input_method_delegate.h:24: virtual void GetHardwareKeyboardLayout(
On 2014/02/12 13:21:02, Seigo Nonaka wrote:
> On 2014/02/11 13:36:02, alemate wrote:
> > virtual const std::vector<std::string>& GetHardwareKeyboardLayouts() const =
> 0;
> > 
> > ?
> 
> Done.

Let's return "const std::string&" here, and split in InputMethodUtil.

https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:583:
std::vector<std::string> input_methods =
const vector<string>&

https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:588:
input_methods.empty() ? "" : input_methods[0]);
You select first hardware input method which might be non-login for some reason.
Let's call GetHardwareLoginInputMethodIds() here and take the first one.

Also if GetHardwareLoginInputMethodIds() would return const reference to a
non-empty vector, it would be no-op.

https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc
(right):

https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc:325:
delegate_->set_hardware_keyboard_layout(keyboard_layouts);
Why set_hardware_keyboard_layout() doesn't invalidate cache itself?

Let's create InputMethodUtil::SetHardwareKeyboardLayoutForTesting(const
std::string&), which would update cache.

https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc:359:
// The physical layouts are French and Hungarian.
English (US) and Russian.

https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_util.cc (right):

https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_util.cc:620: if
(hardware_layouts_.get())
Why are hardware_layouts_ and hardware_login_layouts_ are not automatically
constructed when delegate is created? So you would not need to store them in
scoped_ptr<>.

https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_util.h (right):

https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_util.h:181:
scoped_ptr<std::vector<std::string> > hardware_layouts_;
Do not need scoped_ptr here.

https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_display_host_impl.cc (right):

https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_display_host_impl.cc:159:
manager->GetInputMethodUtil()->InvalidateHardwareInputMethodIdsCache();
Wy do you need to invalidate cache here manually?

Seigo Nonaka

https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos/chrome_browser_main_chromeos.cc File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos/chrome_browser_main_chromeos.cc#newcode583 chrome/browser/chromeos/chrome_browser_main_chromeos.cc:583: std::vector<std::string> input_methods = On 2014/02/12 14:45:35, alemate wrote: > ...

6 years, 10 months ago (2014-02-12 16:49:50 UTC) #6

Seigo Nonaka

On 2014/02/12 14:45:34, alemate wrote: > I think you can avoid checks for "if(cache_valid)" on ...

6 years, 10 months ago (2014-02-12 16:58:20 UTC) #7

On 2014/02/12 14:45:34, alemate wrote:
> I think you can avoid checks for "if(cache_valid)" on every access to the
lists
> of hardware layouts. (And also drop manual call to "invalidate cache").
Consider
> this:
> 
> // Returns original VPD value.
> const string& InputMethodDelegateImpl::GetHardwareKeyboardLayout() const;
> 
> // private method 
> void InputMethodUtil::UpdateHardwareLayouts() {
>     hardware_layouts_.resize(0);
>     hardware_login_layouts_.resize(0);
>     Tokenize(delegate->GetHardwareKeyboardLayout(), ",", &hardware_layouts_);
>     // Filter hardware_layouts_ to hardware_login_layouts_ here.
>     ...
>     // Ensure both lists are non-empty.
>     // (Probably add empty string to them, it they are empty.)
>     ...
> }
> 
> // Call UpdateHardwareLayouts() in InputMethodUtil constructor.
> 
> // Create new:
> void InputMethodUtil::SetHardwareKeyboardLayoutForTesting(const
> string& value) {
>   delegate->set_hardware_keyboard_layout(value);
>   UpdateHardwareLayouts();
> }
> 
> // Simple getter
> const vector<string>& InputMethodUtil::GetHardwareKeyboardLayout() const {
>   DCHECK(!hardware_layouts_.empty());
>   return hardware_layouts_;
> }
> 
> const vector<string>& InputMethodUtil::GetHardwareLoginKeyboardLayout() const
{
>   DCHECK(!hardware_login_layouts_.empty());
>   return hardware_login_layouts_;
> }
> 
> 
> This way you will have only one copy of hardware layouts, and only update it
on
> underlying VPD change. 
> 
> 
> What do you think?
> 
Unfortunately, InputMethodUtil initialization is done too early to obtain
HardwareKeyboardLayout, even VPD value is not initialized.
And also GetHardwareKeyboardLayout may be called before calling
pref->SetString(kHardwareKeyboardLayout).

But as you say, technically updating HardwareKeyboardLayout is not necessary.
I'm now refactoring these logic to be more clear and more reliable, so can I
keep manual invalidation as a temporary fix?

Thank you.

>
https://codereview.chromium.org/139803010/diff/110001/chromeos/ime/input_meth...
> File chromeos/ime/input_method_delegate.h (right):
> 
>
https://codereview.chromium.org/139803010/diff/110001/chromeos/ime/input_meth...
> chromeos/ime/input_method_delegate.h:24: virtual void
GetHardwareKeyboardLayout(
> On 2014/02/12 13:21:02, Seigo Nonaka wrote:
> > On 2014/02/11 13:36:02, alemate wrote:
> > > virtual const std::vector<std::string>& GetHardwareKeyboardLayouts() const
=
> > 0;
> > > 
> > > ?
> > 
> > Done.
> 
> Let's return "const std::string&" here, and split in InputMethodUtil.
> 
>
https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos...
> File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):
> 
>
https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos...
> chrome/browser/chromeos/chrome_browser_main_chromeos.cc:583:
> std::vector<std::string> input_methods =
> const vector<string>&
> 
>
https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos...
> chrome/browser/chromeos/chrome_browser_main_chromeos.cc:588:
> input_methods.empty() ? "" : input_methods[0]);
> You select first hardware input method which might be non-login for some
reason.
> Let's call GetHardwareLoginInputMethodIds() here and take the first one.
> 
> Also if GetHardwareLoginInputMethodIds() would return const reference to a
> non-empty vector, it would be no-op.
> 
>
https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos...
> File
chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos...
>
chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc:325:
> delegate_->set_hardware_keyboard_layout(keyboard_layouts);
> Why set_hardware_keyboard_layout() doesn't invalidate cache itself?
> 
> Let's create InputMethodUtil::SetHardwareKeyboardLayoutForTesting(const
> std::string&), which would update cache.
> 
>
https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos...
>
chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc:359:
> // The physical layouts are French and Hungarian.
> English (US) and Russian.
> 
>
https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos...
> File chrome/browser/chromeos/input_method/input_method_util.cc (right):
> 
>
https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos...
> chrome/browser/chromeos/input_method/input_method_util.cc:620: if
> (hardware_layouts_.get())
> Why are hardware_layouts_ and hardware_login_layouts_ are not automatically
> constructed when delegate is created? So you would not need to store them in
> scoped_ptr<>.
> 
>
https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos...
> File chrome/browser/chromeos/input_method/input_method_util.h (right):
> 
>
https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos...
> chrome/browser/chromeos/input_method/input_method_util.h:181:
> scoped_ptr<std::vector<std::string> > hardware_layouts_;
> Do not need scoped_ptr here.
> 
>
https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos...
> File chrome/browser/chromeos/login/login_display_host_impl.cc (right):
> 
>
https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos...
> chrome/browser/chromeos/login/login_display_host_impl.cc:159:
> manager->GetInputMethodUtil()->InvalidateHardwareInputMethodIdsCache();
> Wy do you need to invalidate cache here manually?

Alexander Alekseev

> Unfortunately, InputMethodUtil initialization is done too early to obtain > HardwareKeyboardLayout, even VPD value ...

6 years, 10 months ago (2014-02-12 17:55:51 UTC) #8

> Unfortunately, InputMethodUtil initialization is done too early to obtain     

> HardwareKeyboardLayout, even VPD value is not initialized.                    

> And also GetHardwareKeyboardLayout may be called before calling               

> pref->SetString(kHardwareKeyboardLayout).                                     

>                                                                               

> But as you say, technically updating HardwareKeyboardLayout is not necessary. 

> I'm now refactoring these logic to be more clear and more reliable, so can I  

> keep manual invalidation as a temporary fix?                                  

>                                                                               

> Thank you.                                                                    

It would definitely be better to subscribe to kHardwareKeyboardLayout update... 

But OK, let's leave manual cache cleanup for now, but please add TODO there.

https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:588:
input_methods.empty() ? "" : input_methods[0]);
On 2014/02/12 16:49:51, Seigo Nonaka wrote:
> If my understanding is correct, this function is called just after Guest user
> login, right?
> Do we need to restrict the active input method to login keyboard?

Yes, this is called after guest session is started, so this method defines input
methods that would be allowed inside Guest session. That is why we enable all
hardware layouts.

See Kenji's comment #27 in crbug.com/329018.

> 
> On 2014/02/12 14:45:35, alemate wrote:
> > You select first hardware input method which might be non-login for some
> reason.
> > Let's call GetHardwareLoginInputMethodIds() here and take the first one.
> > 
> > Also if GetHardwareLoginInputMethodIds() would return const reference to a
> > non-empty vector, it would be no-op.
>

https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc
(right):

https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc:325:
delegate_->set_hardware_keyboard_layout(keyboard_layouts);
On 2014/02/12 16:49:51, Seigo Nonaka wrote:
> To invalidate internal cache, need to include
> chrome/browser/chromeos/input_method/input_method_util.h but it is not allowed
> in fake_input_method_delegate due to DEPS rule.
> 
> On 2014/02/12 14:45:35, alemate wrote:
> > Why set_hardware_keyboard_layout() doesn't invalidate cache itself?
> > 
> > Let's create InputMethodUtil::SetHardwareKeyboardLayoutForTesting(const
> > std::string&), which would update cache.
> 

I mean you can replace two calls:
  GetInputMethodUtil()->InvalidateHardwareInputMethodIdsCache();
  delegate_->set_hardware_keyboard_layout(keyboard_layouts);

by
  GetInputMethodUtil()->SetHardwareKeyboardLayoutForTesting(keyboard_layouts);

I'm afraid that if cache synchronization relies upon external (against
InputMethodUtil class) actions, it would break once someone would modify this
code.

https://codereview.chromium.org/139803010/diff/860001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_delegate_impl.h (right):

https://codereview.chromium.org/139803010/diff/860001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_delegate_impl.h:32:
std::vector<std::string> hardware_layouts_;
Unused?

Seigo Nonaka

https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos/chrome_browser_main_chromeos.cc File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos/chrome_browser_main_chromeos.cc#newcode588 chrome/browser/chromeos/chrome_browser_main_chromeos.cc:588: input_methods.empty() ? "" : input_methods[0]); Sure, done. On 2014/02/12 ...

6 years, 10 months ago (2014-02-13 04:36:05 UTC) #9

https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:588:
input_methods.empty() ? "" : input_methods[0]);
Sure, done.

On 2014/02/12 17:55:52, alemate wrote:
> On 2014/02/12 16:49:51, Seigo Nonaka wrote:
> > If my understanding is correct, this function is called just after Guest
user
> > login, right?
> > Do we need to restrict the active input method to login keyboard?
> 
> Yes, this is called after guest session is started, so this method defines
input
> methods that would be allowed inside Guest session. That is why we enable all
> hardware layouts.
> 
> See Kenji's comment #27 in crbug.com/329018.
> 
> 
> > 
> > On 2014/02/12 14:45:35, alemate wrote:
> > > You select first hardware input method which might be non-login for some
> > reason.
> > > Let's call GetHardwareLoginInputMethodIds() here and take the first one.
> > > 
> > > Also if GetHardwareLoginInputMethodIds() would return const reference to a
> > > non-empty vector, it would be no-op.
> > 
>

https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc
(right):

https://codereview.chromium.org/139803010/diff/750001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc:325:
delegate_->set_hardware_keyboard_layout(keyboard_layouts);
On 2014/02/12 17:55:52, alemate wrote:
> On 2014/02/12 16:49:51, Seigo Nonaka wrote:
> > To invalidate internal cache, need to include
> > chrome/browser/chromeos/input_method/input_method_util.h but it is not
allowed
> > in fake_input_method_delegate due to DEPS rule.
> > 
> > On 2014/02/12 14:45:35, alemate wrote:
> > > Why set_hardware_keyboard_layout() doesn't invalidate cache itself?
> > > 
> > > Let's create InputMethodUtil::SetHardwareKeyboardLayoutForTesting(const
> > > std::string&), which would update cache.
> > 
> 
> I mean you can replace two calls:
>   GetInputMethodUtil()->InvalidateHardwareInputMethodIdsCache();
>   delegate_->set_hardware_keyboard_layout(keyboard_layouts);
> 
> by
>   GetInputMethodUtil()->SetHardwareKeyboardLayoutForTesting(keyboard_layouts);
> 
> 
> I'm afraid that if cache synchronization relies upon external (against
> InputMethodUtil class) actions, it would break once someone would modify this
> code.

Done.

https://codereview.chromium.org/139803010/diff/860001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_delegate_impl.h (right):

https://codereview.chromium.org/139803010/diff/860001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_delegate_impl.h:32:
std::vector<std::string> hardware_layouts_;
On 2014/02/12 17:55:52, alemate wrote:
> Unused?

Done.

Alexander Alekseev

lgtm with nits. https://codereview.chromium.org/139803010/diff/1060001/chrome/browser/chromeos/chrome_browser_main_chromeos.cc File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/139803010/diff/1060001/chrome/browser/chromeos/chrome_browser_main_chromeos.cc#newcode589 chrome/browser/chromeos/chrome_browser_main_chromeos.cc:589: // Seconds, enable locale based input ...

6 years, 10 months ago (2014-02-13 12:47:04 UTC) #10

Seigo Nonaka

https://codereview.chromium.org/139803010/diff/1060001/chrome/browser/chromeos/chrome_browser_main_chromeos.cc File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/139803010/diff/1060001/chrome/browser/chromeos/chrome_browser_main_chromeos.cc#newcode589 chrome/browser/chromeos/chrome_browser_main_chromeos.cc:589: // Seconds, enable locale based input methods. On 2014/02/13 ...

6 years, 10 months ago (2014-02-14 03:51:44 UTC) #11

Seigo Nonaka

Nikita, Komatsu could you take an owner review? Thank you.

6 years, 10 months ago (2014-02-14 03:53:50 UTC) #12

Hiro Komatsu

https://codereview.chromium.org/139803010/diff/1150001/chrome/browser/chromeos/chrome_browser_main_chromeos.cc File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/139803010/diff/1150001/chrome/browser/chromeos/chrome_browser_main_chromeos.cc#newcode587 chrome/browser/chromeos/chrome_browser_main_chromeos.cc:587: ime_manager->EnableInputMethod(input_methods[i]); EnableInputMethods? https://codereview.chromium.org/139803010/diff/1150001/chrome/browser/chromeos/chrome_browser_main_chromeos.cc#newcode600 chrome/browser/chromeos/chrome_browser_main_chromeos.cc:600: // Finally, actrivate the first ...

6 years, 10 months ago (2014-02-14 07:16:51 UTC) #13

Seigo Nonaka

https://codereview.chromium.org/139803010/diff/1150001/chrome/browser/chromeos/chrome_browser_main_chromeos.cc File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/139803010/diff/1150001/chrome/browser/chromeos/chrome_browser_main_chromeos.cc#newcode587 chrome/browser/chromeos/chrome_browser_main_chromeos.cc:587: ime_manager->EnableInputMethod(input_methods[i]); On 2014/02/14 07:16:51, Hiro Komatsu wrote: > EnableInputMethods? ...

6 years, 10 months ago (2014-02-14 08:13:22 UTC) #14

Nikita (slow)

lgtm https://codereview.chromium.org/139803010/diff/1330001/chrome/browser/chromeos/preferences.cc File chrome/browser/chromeos/preferences.cc (right): https://codereview.chromium.org/139803010/diff/1330001/chrome/browser/chromeos/preferences.cc#newcode99 chrome/browser/chromeos/preferences.cc:99: hardware_keyboard_id = JoinString(hardware_layouts, ','); nit: I thought there's ...

6 years, 10 months ago (2014-02-14 08:45:44 UTC) #15

Seigo Nonaka

https://codereview.chromium.org/139803010/diff/1330001/chrome/browser/chromeos/preferences.cc File chrome/browser/chromeos/preferences.cc (right): https://codereview.chromium.org/139803010/diff/1330001/chrome/browser/chromeos/preferences.cc#newcode99 chrome/browser/chromeos/preferences.cc:99: hardware_keyboard_id = JoinString(hardware_layouts, ','); On 2014/02/14 08:45:45, Nikita Kostylev ...

6 years, 10 months ago (2014-02-14 11:18:33 UTC) #16

Alexander Alekseev

https://codereview.chromium.org/139803010/diff/1150001/chrome/browser/chromeos/chrome_browser_main_chromeos.cc File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/139803010/diff/1150001/chrome/browser/chromeos/chrome_browser_main_chromeos.cc#newcode587 chrome/browser/chromeos/chrome_browser_main_chromeos.cc:587: ime_manager->EnableInputMethod(input_methods[i]); On 2014/02/14 08:13:22, Seigo Nonaka wrote: > On ...

6 years, 10 months ago (2014-02-14 13:59:16 UTC) #17

Hiro Komatsu

lgtm lgtm assuming you will address alemate's comment.

6 years, 10 months ago (2014-02-15 23:57:16 UTC) #18

Seigo Nonaka

https://codereview.chromium.org/139803010/diff/1150001/chrome/browser/chromeos/chrome_browser_main_chromeos.cc File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/139803010/diff/1150001/chrome/browser/chromeos/chrome_browser_main_chromeos.cc#newcode587 chrome/browser/chromeos/chrome_browser_main_chromeos.cc:587: ime_manager->EnableInputMethod(input_methods[i]); Nice catch! Reverted to previous impl and renamed ...

6 years, 10 months ago (2014-02-16 15:50:29 UTC) #19

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nona@chromium.org/139803010/1480001

6 years, 10 months ago (2014-02-16 20:43:47 UTC) #21

Message was sent while issue was closed.

Change committed as 251653

Expand Messages | Collapse Messages