Issue 6675005: Integrate the new input method API for Views into Chromium.

James Su

Hi all, This is the sixth CL that does the final integration. It depends on: ...

9 years, 9 months ago (2011-03-21 19:13:50 UTC) #1

sadrul

http://codereview.chromium.org/6675005/diff/1/views/widget/widget_gtk.cc File views/widget/widget_gtk.cc (right): http://codereview.chromium.org/6675005/diff/1/views/widget/widget_gtk.cc#newcode1202 views/widget/widget_gtk.cc:1202: DispatchKeyEventPostIME(key); return DispatchKeyEventPostIME(key); http://codereview.chromium.org/6675005/diff/1/views/widget/widget_gtk.cc#newcode1322 views/widget/widget_gtk.cc:1322: } return GTK_IS_WINDOW(widget) ? ...

9 years, 9 months ago (2011-03-21 19:57:45 UTC) #2

James Su

http://codereview.chromium.org/6675005/diff/1/views/widget/widget_gtk.cc File views/widget/widget_gtk.cc (right): http://codereview.chromium.org/6675005/diff/1/views/widget/widget_gtk.cc#newcode1202 views/widget/widget_gtk.cc:1202: DispatchKeyEventPostIME(key); On 2011/03/21 19:57:45, sadrul wrote: > return DispatchKeyEventPostIME(key); ...

9 years, 9 months ago (2011-03-21 21:34:07 UTC) #3

http://codereview.chromium.org/6675005/diff/1/views/widget/widget_gtk.cc
File views/widget/widget_gtk.cc (right):

http://codereview.chromium.org/6675005/diff/1/views/widget/widget_gtk.cc#newc...
views/widget/widget_gtk.cc:1202: DispatchKeyEventPostIME(key);
On 2011/03/21 19:57:45, sadrul wrote:
> return DispatchKeyEventPostIME(key);

It's intentionally to always return true to prevent GtkWindow's default key
event handler.

And child Widget will never get keyboard focus, thus this method will never be
called for child Widget.

It's probably necessary to add a DCHECK() here to enforce it.

http://codereview.chromium.org/6675005/diff/1/views/widget/widget_gtk.cc#newc...
views/widget/widget_gtk.cc:1322: }
On 2011/03/21 19:57:45, sadrul wrote:
> return GTK_IS_WINDOW(widget) ? true : handled;
See my comment in OnKeyEvent().

http://codereview.chromium.org/6675005/diff/1/views/widget/widget_gtk.h
File views/widget/widget_gtk.h (right):

http://codereview.chromium.org/6675005/diff/1/views/widget/widget_gtk.h#newco...
views/widget/widget_gtk.h:436: scoped_ptr<InputMethod> input_method_;
On 2011/03/21 19:57:45, sadrul wrote:
> Would it be possible to move input_method_ to Widget (instead of in WidgetGtk
> and WidgetWin)? (You could look at Widget::compositor_ for an example)

IMHO, it's not possible, different input method implementations may communicate
with native widgets in different ways. Moving input_method_ into Widget would
require lots of down casts in native widget code.

http://codereview.chromium.org/6675005/diff/1/views/widget/widget_win.cc
File views/widget/widget_win.cc (right):

http://codereview.chromium.org/6675005/diff/1/views/widget/widget_win.cc#newc...
views/widget/widget_win.cc:744: DispatchKeyEventPostIME(key);
On 2011/03/21 19:57:45, sadrul wrote:
> Instead of DispatchKeyEvent'ing here (and OnKeyUp, and in widget_gtk.cc),
would
> it be possible to do this from inside RootView::ProcessKeyEvent? That way, a
> Widget can just always send a KeyEvent to the RootView, and the RootView takes
> care of dispatching the event to the IME (and there won't be a need for
> DispatchKeyEventPostIME).
> 
> The first few lines of RootView then would be:
> 
> bool RootView::ProcessKeyEvent(const KeyEvent& event) {
>   if (GetWidget()->input_method()->DispatchKeyEvent(event))
>     return true;
> 
>   bool consumed = false;
>   ...
> }

It's not possible. The native widget may still need to do extra work in its
DispatchKeyEventPostIME() method. Eg. WidgetGtk dispatches key events further
down to native gtk widgets and FocusManager. And WidgetWin needs call
SetMsgHandled().

oshima

1st round. Could you please add more description about the change? It'd be much easier ...

9 years, 9 months ago (2011-03-22 01:50:38 UTC) #4

James Su

http://codereview.chromium.org/6675005/diff/1/chrome/browser/ui/views/find_bar_view.cc File chrome/browser/ui/views/find_bar_view.cc (left): http://codereview.chromium.org/6675005/diff/1/chrome/browser/ui/views/find_bar_view.cc#oldcode93 chrome/browser/ui/views/find_bar_view.cc:93: #endif On 2011/03/22 01:50:39, oshima wrote: > Is it ...

9 years, 9 months ago (2011-03-22 08:41:14 UTC) #5

Ben Goodger (Google)

I made a couple of minor comments on the dependent changelists but am generally happy ...

9 years, 9 months ago (2011-03-22 20:49:30 UTC) #6

James Su

Hi all, I updated the CL to address review feedbacks. Please kindly review it again. ...

9 years, 9 months ago (2011-03-22 22:35:38 UTC) #7

Peng

http://codereview.chromium.org/6675005/diff/17001/views/focus/accelerator_handler_touch.cc File views/focus/accelerator_handler_touch.cc (right): http://codereview.chromium.org/6675005/diff/17001/views/focus/accelerator_handler_touch.cc#newcode180 views/focus/accelerator_handler_touch.cc:180: if (ime && focused_view && focused_view->GetTextInputClient()) { If the ...

9 years, 9 months ago (2011-03-24 03:58:43 UTC) #8

oshima

http://codereview.chromium.org/6675005/diff/17001/chrome/browser/chromeos/login/registration_screen.cc File chrome/browser/chromeos/login/registration_screen.cc (right): http://codereview.chromium.org/6675005/diff/17001/chrome/browser/chromeos/login/registration_screen.cc#newcode127 chrome/browser/chromeos/login/registration_screen.cc:127: views::KeyEvent views_event(reinterpret_cast<GdkEvent*>(event.os_event)); does this have to be reinterpret_cast? http://codereview.chromium.org/6675005/diff/17001/chrome/browser/chromeos/login/screen_locker.cc ...

9 years, 9 months ago (2011-03-24 22:58:47 UTC) #9

James Su

http://codereview.chromium.org/6675005/diff/17001/chrome/browser/chromeos/login/registration_screen.cc File chrome/browser/chromeos/login/registration_screen.cc (right): http://codereview.chromium.org/6675005/diff/17001/chrome/browser/chromeos/login/registration_screen.cc#newcode127 chrome/browser/chromeos/login/registration_screen.cc:127: views::KeyEvent views_event(reinterpret_cast<GdkEvent*>(event.os_event)); On 2011/03/24 22:58:47, oshima wrote: > does ...

9 years, 9 months ago (2011-03-25 05:57:20 UTC) #10

http://codereview.chromium.org/6675005/diff/17001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/registration_screen.cc (right):

http://codereview.chromium.org/6675005/diff/17001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/registration_screen.cc:127: views::KeyEvent
views_event(reinterpret_cast<GdkEvent*>(event.os_event));
On 2011/03/24 22:58:47, oshima wrote:
> does this have to be reinterpret_cast?

I think it has to, though I never tried. GdkEvent is a union of all event
structs.

http://codereview.chromium.org/6675005/diff/17001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/screen_locker.cc (right):

http://codereview.chromium.org/6675005/diff/17001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screen_locker.cc:699:
lock_window_->native_widget()->ReplaceInputMethod(NULL);
On 2011/03/24 22:58:47, oshima wrote:
> Two q:
> 1) I assume we'll not use new API for existing gtk implementation, correct?
Yes, I guess so. As long as we are not going to use TextfieldViews and
RenderWidgetHostViewViews. So actually it's pretty safe to disable the
corresponding code in WidgetGtk for ChromeOS. I'll change it asap.

> 
> 2) User can use different keyboard layout, and can use some control key to
> switch keyboard layout. I believe it's using some ibus stuff to control this
> behavior. I assume your change won't break this behavior on chromeos, correct?

Current chromeos uses a very hacky approach (gtk key snooper) to intercept key
events and perform such actions before dispatching the events to chrome.

We need to get rid of this ugly thing in TouchUI and do it in InputMethodIBus
object.

You know, the whole input method/keyboard layout solution in current chromeos is
actually very hacky, caused by limited design and feature of ibus. Sooner or
later we need to rework all of these parts.

TouchUI is a good point for us to rethink everything. But unfortunately, both
ChromeOS Japan team and Peng insist to believe that ibus is good enough and can
be improved to support everything we need. I tried to persuade them but failed.
That's why this new abstract input method API is important for chrome, so that
the chrome code won't be affected when we need to switch underlying input method
framework.

http://codereview.chromium.org/6675005/diff/17001/views/controls/textfield/na...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/6675005/diff/17001/views/controls/textfield/na...
views/controls/textfield/native_textfield_views.cc:562: #if 0
On 2011/03/24 22:58:47, oshima wrote:
> Has this been fixed?

Yes it's been fixed. I need to try it though.

http://codereview.chromium.org/6675005/diff/17001/views/controls/textfield/te...
File views/controls/textfield/textfield_controller.h (right):

http://codereview.chromium.org/6675005/diff/17001/views/controls/textfield/te...
views/controls/textfield/textfield_controller.h:22: // Textfield::SetText() or
Textfield::AppendText().
On 2011/03/24 22:58:47, oshima wrote:
> Okay, so since this CL is changing the behavior on win as well, please double
> check the all usage of Textfield and make sure it doesn't break the behavior,
> which I believe you did. I'm just saying again :)

I'll test all features carefully before landing this CL. It'll be great if you
guys can help test it as well :)

http://codereview.chromium.org/6675005/diff/17001/views/controls/textfield/te...
File views/controls/textfield/textfield_views_model.cc (right):

http://codereview.chromium.org/6675005/diff/17001/views/controls/textfield/te...
views/controls/textfield/textfield_views_model.cc:496: ClearSelection();
On 2011/03/24 22:58:47, oshima wrote:
> I believe there should be no selection at this point?

changing the value of cursor_pos_ will create a selection range automatically :)
Though it's same as selection_start_ = cursor_pos_;, original code always calls
ClearSelection() after changing cursor_pos_.

http://codereview.chromium.org/6675005/diff/17001/views/controls/textfield/te...
File views/controls/textfield/textfield_views_model.h (right):

http://codereview.chromium.org/6675005/diff/17001/views/controls/textfield/te...
views/controls/textfield/textfield_views_model.h:241: Delegate* delegate_;
On 2011/03/24 22:58:47, oshima wrote:
> comment

Done.

http://codereview.chromium.org/6675005/diff/17001/views/widget/widget_gtk.cc
File views/widget/widget_gtk.cc (right):

http://codereview.chromium.org/6675005/diff/17001/views/widget/widget_gtk.cc#...
views/widget/widget_gtk.cc:516: }
On 2011/03/24 22:58:47, oshima wrote:
> Looks like we create InputMethodGtk for regular chromeos.
> I believe we don't need this for current chromeos, which doesn't use
> TextfieldViews, and RenderWidgetHostViewViews)?

Yes. I'll disable this piece of code for ChromeOS.

oshima

Jay, can you check focus_manager.cc change? I can't tell if this is safe or not. ...

9 years, 9 months ago (2011-03-25 17:31:25 UTC) #11

Jay, can you check focus_manager.cc change? I can't tell if this is safe or 
not.

http://codereview.chromium.org/6675005/diff/5046/views/controls/textfield/nat...
File views/controls/textfield/native_textfield_views_unittest.cc (left):

http://codereview.chromium.org/6675005/diff/5046/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views_unittest.cc:309:
EXPECT_STR_EQ("my password", last_contents_);
On 2011/03/22 08:41:14, James Su wrote:
> On 2011/03/22 01:50:39, oshima wrote:
> > is this no longer valid?
> 
> With this CL, TextfieldController::ContentsChanged() will never be called when
> the text is changed programmatically.
> Because in such case, the controller should already know the change, so it
makes
> no sense to inform the controller again.

Above argument is not necessarily true because the code who sets the text and
the code that is interested in the change may be different (in general). I
understand that this is good enough for our use case, but in that case,
can you test if ContentChanged is NOT called?

http://codereview.chromium.org/6675005/diff/5046/views/focus/focus_manager.cc
File views/focus/focus_manager.cc (left):

http://codereview.chromium.org/6675005/diff/5046/views/focus/focus_manager.cc...
views/focus/focus_manager.cc:374: ClearFocus();
On 2011/03/22 08:41:14, James Su wrote:
> On 2011/03/22 01:50:39, oshima wrote:
> > For my education, can you explain why we're removing this?
> 
> To fix NativeTextfieldViewsTest.FocusTraversalTest and input method related
> tests.

Changing the code to fix test doesn't sounds good explanation :)

> 
> The problem is: in StoreFocusedView() we don't store NULL view, thus it makes
no
> sense to restore a NULL view in RestoreFocusedView().

Hmm, i'm not convinced (yet) if this is okay. May be fine, but it's subtle. What
happen to the existing focused view? Won't this change the behavior?
I'm adding jay who may know why we have this now.

http://codereview.chromium.org/6675005/diff/17001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/registration_screen.cc (right):

http://codereview.chromium.org/6675005/diff/17001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/registration_screen.cc:127: views::KeyEvent
views_event(reinterpret_cast<GdkEvent*>(event.os_event));
On 2011/03/25 05:57:21, James Su wrote:
> On 2011/03/24 22:58:47, oshima wrote:
> > does this have to be reinterpret_cast?
> 
> I think it has to, though I never tried. GdkEvent is a union of all event
> structs.

I see, os_event is key event. It needs to be reinterpret_cast then.

http://codereview.chromium.org/6675005/diff/17001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/screen_locker.cc (right):

http://codereview.chromium.org/6675005/diff/17001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screen_locker.cc:699:
lock_window_->native_widget()->ReplaceInputMethod(NULL);
On 2011/03/25 05:57:21, James Su wrote:
> On 2011/03/24 22:58:47, oshima wrote:
> > Two q:
> > 1) I assume we'll not use new API for existing gtk implementation, correct?
> Yes, I guess so. As long as we are not going to use TextfieldViews and
> RenderWidgetHostViewViews. So actually it's pretty safe to disable the
> corresponding code in WidgetGtk for ChromeOS. I'll change it asap.
> 
> > 
> > 2) User can use different keyboard layout, and can use some control key to
> > switch keyboard layout. I believe it's using some ibus stuff to control this
> > behavior. I assume your change won't break this behavior on chromeos,
correct?
> 
> Current chromeos uses a very hacky approach (gtk key snooper) to intercept key
> events and perform such actions before dispatching the events to chrome.
> 
> We need to get rid of this ugly thing in TouchUI and do it in InputMethodIBus
> object.
> 
> You know, the whole input method/keyboard layout solution in current chromeos
is
> actually very hacky, caused by limited design and feature of ibus. Sooner or
> later we need to rework all of these parts.
> 
> TouchUI is a good point for us to rethink everything. But unfortunately, both
> ChromeOS Japan team and Peng insist to believe that ibus is good enough and
can
> be improved to support everything we need. I tried to persuade them but
failed.
> That's why this new abstract input method API is important for chrome, so that
> the chrome code won't be affected when we need to switch underlying input
method
> framework.

I think new API is needed regardless. Improvement is always good, but you need
to prove it, especially when there is working solution. And when you do, it's
important to provide smooth transition path. It should be able to switch between
two implementations easily without hassle.

http://codereview.chromium.org/6675005/diff/17001/views/controls/textfield/na...
File views/controls/textfield/native_textfield_views_unittest.cc (right):

http://codereview.chromium.org/6675005/diff/17001/views/controls/textfield/na...
views/controls/textfield/native_textfield_views_unittest.cc:142:
++on_before_user_action_;
FYI: it's okay to use postfix notation these days. (no need to change, just FYI)

James Su

http://codereview.chromium.org/6675005/diff/5046/views/controls/textfield/native_textfield_views_unittest.cc File views/controls/textfield/native_textfield_views_unittest.cc (left): http://codereview.chromium.org/6675005/diff/5046/views/controls/textfield/native_textfield_views_unittest.cc#oldcode309 views/controls/textfield/native_textfield_views_unittest.cc:309: EXPECT_STR_EQ("my password", last_contents_); On 2011/03/25 17:31:25, oshima wrote: > ...

9 years, 9 months ago (2011-03-25 20:53:59 UTC) #12

http://codereview.chromium.org/6675005/diff/5046/views/controls/textfield/nat...
File views/controls/textfield/native_textfield_views_unittest.cc (left):

http://codereview.chromium.org/6675005/diff/5046/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views_unittest.cc:309:
EXPECT_STR_EQ("my password", last_contents_);
On 2011/03/25 17:31:25, oshima wrote:
> On 2011/03/22 08:41:14, James Su wrote:
> > On 2011/03/22 01:50:39, oshima wrote:
> > > is this no longer valid?
> > 
> > With this CL, TextfieldController::ContentsChanged() will never be called
when
> > the text is changed programmatically.
> > Because in such case, the controller should already know the change, so it
> makes
> > no sense to inform the controller again.
> 
> Above argument is not necessarily true because the code who sets the text and
> the code that is interested in the change may be different (in general). I
> understand that this is good enough for our use case, but in that case,
> can you test if ContentChanged is NOT called?
If I understand correctly, according to the old code and comment, this should be
the intended behavior when this API was designed, but somehow we had broken
implementation. That's why the |ignore_contents_changed_| hack was introduced in
chrome/browser/ui/views/find_bar_view.cc. But please correct me if I'm wrong.

This behavior is tested in ModelChangesTest.

http://codereview.chromium.org/6675005/diff/5046/views/focus/focus_manager.cc
File views/focus/focus_manager.cc (left):

http://codereview.chromium.org/6675005/diff/5046/views/focus/focus_manager.cc...
views/focus/focus_manager.cc:374: ClearFocus();
On 2011/03/25 17:31:25, oshima wrote:
> On 2011/03/22 08:41:14, James Su wrote:
> > On 2011/03/22 01:50:39, oshima wrote:
> > > For my education, can you explain why we're removing this?
> > 
> > To fix NativeTextfieldViewsTest.FocusTraversalTest and input method related
> > tests.
> 
> Changing the code to fix test doesn't sounds good explanation :)
> 
> > 
> > The problem is: in StoreFocusedView() we don't store NULL view, thus it
makes
> no
> > sense to restore a NULL view in RestoreFocusedView().
> 
> Hmm, i'm not convinced (yet) if this is okay. May be fine, but it's subtle.
What
> happen to the existing focused view? Won't this change the behavior?
> I'm adding jay who may know why we have this now.

I'm actually not fully confident of this change. Let's wait for Jay's feedback.

http://codereview.chromium.org/6675005/diff/17001/views/controls/textfield/na...
File views/controls/textfield/native_textfield_views_unittest.cc (right):

http://codereview.chromium.org/6675005/diff/17001/views/controls/textfield/na...
views/controls/textfield/native_textfield_views_unittest.cc:142:
++on_before_user_action_;
On 2011/03/25 17:31:25, oshima wrote:
> FYI: it's okay to use postfix notation these days. (no need to change, just
FYI)

I'm just used to it :)

oshima

http://codereview.chromium.org/6675005/diff/5046/views/controls/textfield/native_textfield_views_unittest.cc File views/controls/textfield/native_textfield_views_unittest.cc (left): http://codereview.chromium.org/6675005/diff/5046/views/controls/textfield/native_textfield_views_unittest.cc#oldcode309 views/controls/textfield/native_textfield_views_unittest.cc:309: EXPECT_STR_EQ("my password", last_contents_); On 2011/03/25 20:53:59, James Su wrote: ...

9 years, 9 months ago (2011-03-25 21:41:58 UTC) #13

James Su

http://codereview.chromium.org/6675005/diff/5046/views/controls/textfield/native_textfield_views_unittest.cc File views/controls/textfield/native_textfield_views_unittest.cc (left): http://codereview.chromium.org/6675005/diff/5046/views/controls/textfield/native_textfield_views_unittest.cc#oldcode309 views/controls/textfield/native_textfield_views_unittest.cc:309: EXPECT_STR_EQ("my password", last_contents_); On 2011/03/25 21:41:58, oshima wrote: > ...

9 years, 9 months ago (2011-03-25 21:48:20 UTC) #14

http://codereview.chromium.org/6675005/diff/5046/views/controls/textfield/nat...
File views/controls/textfield/native_textfield_views_unittest.cc (left):

http://codereview.chromium.org/6675005/diff/5046/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views_unittest.cc:309:
EXPECT_STR_EQ("my password", last_contents_);
On 2011/03/25 21:41:58, oshima wrote:
> On 2011/03/25 20:53:59, James Su wrote:
> > On 2011/03/25 17:31:25, oshima wrote:
> > > On 2011/03/22 08:41:14, James Su wrote:
> > > > On 2011/03/22 01:50:39, oshima wrote:
> > > > > is this no longer valid?
> > > > 
> > > > With this CL, TextfieldController::ContentsChanged() will never be
called
> > when
> > > > the text is changed programmatically.
> > > > Because in such case, the controller should already know the change, so
it
> > > makes
> > > > no sense to inform the controller again.
> > > 
> > > Above argument is not necessarily true because the code who sets the text
> and
> > > the code that is interested in the change may be different (in general). I
> > > understand that this is good enough for our use case, but in that case,
> > > can you test if ContentChanged is NOT called?
> > If I understand correctly, according to the old code and comment, this
should
> be
> > the intended behavior when this API was designed, but somehow we had broken
> > implementation. That's why the |ignore_contents_changed_| hack was
introduced
> in
> > chrome/browser/ui/views/find_bar_view.cc. But please correct me if I'm
wrong.
> > 
> > This behavior is tested in ModelChangesTest.
> 
> ContentsChanges predates my chrome era and I was trying to replicate existing
> gtk behavior. I agree that your change simplifies findbar code, and I'm all
for
> it unless it breaks other stuff.
> 
> Since ChangeContented has been moved to Textfield class, testing model isn't
> enough and we need to test it here.
Yes. The ModelChangesTest is in this file, see above around line 236.

Jay Civelli

http://codereview.chromium.org/6675005/diff/9010/views/focus/focus_manager.cc File views/focus/focus_manager.cc (left): http://codereview.chromium.org/6675005/diff/9010/views/focus/focus_manager.cc#oldcode374 views/focus/focus_manager.cc:374: ClearFocus(); Why don't we need this anymore?

9 years, 9 months ago (2011-03-28 16:04:36 UTC) #15

James Su

http://codereview.chromium.org/6675005/diff/9010/views/focus/focus_manager.cc File views/focus/focus_manager.cc (left): http://codereview.chromium.org/6675005/diff/9010/views/focus/focus_manager.cc#oldcode374 views/focus/focus_manager.cc:374: ClearFocus(); On 2011/03/28 16:04:36, Jay Civelli wrote: > Why ...

9 years, 9 months ago (2011-03-28 16:54:12 UTC) #16

oshima

LGTM on my bits once you address below. Please make sure you address comments from ...

9 years, 9 months ago (2011-03-28 19:56:48 UTC) #17

LGTM on my bits once you address below. Please make sure you address comments
from IME folks.

http://codereview.chromium.org/6675005/diff/5046/views/controls/textfield/nat...
File views/controls/textfield/native_textfield_views_unittest.cc (left):

http://codereview.chromium.org/6675005/diff/5046/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views_unittest.cc:309:
EXPECT_STR_EQ("my password", last_contents_);
On 2011/03/25 21:48:20, James Su wrote:
> On 2011/03/25 21:41:58, oshima wrote:
> > On 2011/03/25 20:53:59, James Su wrote:
> > > On 2011/03/25 17:31:25, oshima wrote:
> > > > On 2011/03/22 08:41:14, James Su wrote:
> > > > > On 2011/03/22 01:50:39, oshima wrote:
> > > > > > is this no longer valid?
> > > > > 
> > > > > With this CL, TextfieldController::ContentsChanged() will never be
> called
> > > when
> > > > > the text is changed programmatically.
> > > > > Because in such case, the controller should already know the change,
so
> it
> > > > makes
> > > > > no sense to inform the controller again.
> > > > 
> > > > Above argument is not necessarily true because the code who sets the
text
> > and
> > > > the code that is interested in the change may be different (in general).
I
> > > > understand that this is good enough for our use case, but in that case,
> > > > can you test if ContentChanged is NOT called?
> > > If I understand correctly, according to the old code and comment, this
> should
> > be
> > > the intended behavior when this API was designed, but somehow we had
broken
> > > implementation. That's why the |ignore_contents_changed_| hack was
> introduced
> > in
> > > chrome/browser/ui/views/find_bar_view.cc. But please correct me if I'm
> wrong.
> > > 
> > > This behavior is tested in ModelChangesTest.
> > 
> > ContentsChanges predates my chrome era and I was trying to replicate
existing
> > gtk behavior. I agree that your change simplifies findbar code, and I'm all
> for
> > it unless it breaks other stuff.
> > 
> > Since ChangeContented has been moved to Textfield class, testing model isn't
> > enough and we need to test it here.
> Yes. The ModelChangesTest is in this file, see above around line 236.

Please add the same test, as this is for password.

http://codereview.chromium.org/6675005/diff/5046/views/controls/textfield/nat...
File views/controls/textfield/native_textfield_views_unittest.cc (right):

http://codereview.chromium.org/6675005/diff/5046/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views_unittest.cc:246:
EXPECT_EQ(string16(), last_contents_);
On 2011/03/22 08:41:14, James Su wrote:
> On 2011/03/22 01:50:39, oshima wrote:
> > Why this is empty string now?
> 
> Because TextfieldController::ContentsChanged() won't be called when the textt
is
> changed by calling Textfield::SetText().

EXPECT_TRUE(last_contents_.empty())

Jay Civelli

LGTM On Mon, Mar 28, 2011 at 12:56 PM, <oshima@chromium.org> wrote: > LGTM on my ...

9 years, 9 months ago (2011-03-28 20:31:49 UTC) #18

LGTM

On Mon, Mar 28, 2011 at 12:56 PM, <oshima@chromium.org> wrote:

> LGTM on my bits once you address below. Please make sure you address
> comments
> from IME folks.
>
>
>
>
>
http://codereview.chromium.org/6675005/diff/5046/views/controls/textfield/nat...
> File views/controls/textfield/native_textfield_views_unittest.cc (left):
>
>
>
http://codereview.chromium.org/6675005/diff/5046/views/controls/textfield/nat...
> views/controls/textfield/native_textfield_views_unittest.cc:309:
> EXPECT_STR_EQ("my password", last_contents_);
> On 2011/03/25 21:48:20, James Su wrote:
>
>> On 2011/03/25 21:41:58, oshima wrote:
>> > On 2011/03/25 20:53:59, James Su wrote:
>> > > On 2011/03/25 17:31:25, oshima wrote:
>> > > > On 2011/03/22 08:41:14, James Su wrote:
>> > > > > On 2011/03/22 01:50:39, oshima wrote:
>> > > > > > is this no longer valid?
>> > > > >
>> > > > > With this CL, TextfieldController::ContentsChanged() will
>>
> never be
>
>> called
>> > > when
>> > > > > the text is changed programmatically.
>> > > > > Because in such case, the controller should already know the
>>
> change, so
>
>> it
>> > > > makes
>> > > > > no sense to inform the controller again.
>> > > >
>> > > > Above argument is not necessarily true because the code who sets
>>
> the text
>
>> > and
>> > > > the code that is interested in the change may be different (in
>>
> general). I
>
>> > > > understand that this is good enough for our use case, but in
>>
> that case,
>
>> > > > can you test if ContentChanged is NOT called?
>> > > If I understand correctly, according to the old code and comment,
>>
> this
>
>> should
>> > be
>> > > the intended behavior when this API was designed, but somehow we
>>
> had broken
>
>> > > implementation. That's why the |ignore_contents_changed_| hack was
>> introduced
>> > in
>> > > chrome/browser/ui/views/find_bar_view.cc. But please correct me if
>>
> I'm
>
>> wrong.
>> > >
>> > > This behavior is tested in ModelChangesTest.
>> >
>> > ContentsChanges predates my chrome era and I was trying to replicate
>>
> existing
>
>> > gtk behavior. I agree that your change simplifies findbar code, and
>>
> I'm all
>
>> for
>> > it unless it breaks other stuff.
>> >
>> > Since ChangeContented has been moved to Textfield class, testing
>>
> model isn't
>
>> > enough and we need to test it here.
>> Yes. The ModelChangesTest is in this file, see above around line 236.
>>
>
> Please add the same test, as this is for password.
>
>
>
>
http://codereview.chromium.org/6675005/diff/5046/views/controls/textfield/nat...
> File views/controls/textfield/native_textfield_views_unittest.cc
> (right):
>
>
>
http://codereview.chromium.org/6675005/diff/5046/views/controls/textfield/nat...
> views/controls/textfield/native_textfield_views_unittest.cc:246:
> EXPECT_EQ(string16(), last_contents_);
>
> On 2011/03/22 08:41:14, James Su wrote:
>
>> On 2011/03/22 01:50:39, oshima wrote:
>> > Why this is empty string now?
>>
>
>  Because TextfieldController::ContentsChanged() won't be called when
>>
> the textt is
>
>> changed by calling Textfield::SetText().
>>
>
> EXPECT_TRUE(last_contents_.empty())
>
>
> http://codereview.chromium.org/6675005/
>

James Su

http://codereview.chromium.org/6675005/diff/5046/views/controls/textfield/native_textfield_views_unittest.cc File views/controls/textfield/native_textfield_views_unittest.cc (left): http://codereview.chromium.org/6675005/diff/5046/views/controls/textfield/native_textfield_views_unittest.cc#oldcode309 views/controls/textfield/native_textfield_views_unittest.cc:309: EXPECT_STR_EQ("my password", last_contents_); On 2011/03/28 19:56:49, oshima wrote: > ...

9 years, 9 months ago (2011-03-28 21:25:33 UTC) #19

http://codereview.chromium.org/6675005/diff/5046/views/controls/textfield/nat...
File views/controls/textfield/native_textfield_views_unittest.cc (left):

http://codereview.chromium.org/6675005/diff/5046/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views_unittest.cc:309:
EXPECT_STR_EQ("my password", last_contents_);
On 2011/03/28 19:56:49, oshima wrote:
> On 2011/03/25 21:48:20, James Su wrote:
> > On 2011/03/25 21:41:58, oshima wrote:
> > > On 2011/03/25 20:53:59, James Su wrote:
> > > > On 2011/03/25 17:31:25, oshima wrote:
> > > > > On 2011/03/22 08:41:14, James Su wrote:
> > > > > > On 2011/03/22 01:50:39, oshima wrote:
> > > > > > > is this no longer valid?
> > > > > > 
> > > > > > With this CL, TextfieldController::ContentsChanged() will never be
> > called
> > > > when
> > > > > > the text is changed programmatically.
> > > > > > Because in such case, the controller should already know the change,
> so
> > it
> > > > > makes
> > > > > > no sense to inform the controller again.
> > > > > 
> > > > > Above argument is not necessarily true because the code who sets the
> text
> > > and
> > > > > the code that is interested in the change may be different (in
general).
> I
> > > > > understand that this is good enough for our use case, but in that
case,
> > > > > can you test if ContentChanged is NOT called?
> > > > If I understand correctly, according to the old code and comment, this
> > should
> > > be
> > > > the intended behavior when this API was designed, but somehow we had
> broken
> > > > implementation. That's why the |ignore_contents_changed_| hack was
> > introduced
> > > in
> > > > chrome/browser/ui/views/find_bar_view.cc. But please correct me if I'm
> > wrong.
> > > > 
> > > > This behavior is tested in ModelChangesTest.
> > > 
> > > ContentsChanges predates my chrome era and I was trying to replicate
> existing
> > > gtk behavior. I agree that your change simplifies findbar code, and I'm
all
> > for
> > > it unless it breaks other stuff.
> > > 
> > > Since ChangeContented has been moved to Textfield class, testing model
isn't
> > > enough and we need to test it here.
> > Yes. The ModelChangesTest is in this file, see above around line 236.
> 
> Please add the same test, as this is for password.

Done.

http://codereview.chromium.org/6675005/diff/5046/views/controls/textfield/nat...
File views/controls/textfield/native_textfield_views_unittest.cc (right):

http://codereview.chromium.org/6675005/diff/5046/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views_unittest.cc:246:
EXPECT_EQ(string16(), last_contents_);
On 2011/03/28 19:56:49, oshima wrote:
> On 2011/03/22 08:41:14, James Su wrote:
> > On 2011/03/22 01:50:39, oshima wrote:
> > > Why this is empty string now?
> > 
> > Because TextfieldController::ContentsChanged() won't be called when the
textt
> is
> > changed by calling Textfield::SetText().
> 
> EXPECT_TRUE(last_contents_.empty())

Done.

Peng

http://codereview.chromium.org/6675005/diff/17001/views/focus/accelerator_handler_touch.cc File views/focus/accelerator_handler_touch.cc (right): http://codereview.chromium.org/6675005/diff/17001/views/focus/accelerator_handler_touch.cc#newcode180 views/focus/accelerator_handler_touch.cc:180: if (ime && focused_view && focused_view->GetTextInputClient()) { Did you ...

9 years, 9 months ago (2011-03-29 03:04:46 UTC) #20

James Su

http://codereview.chromium.org/6675005/diff/17001/views/focus/accelerator_handler_touch.cc File views/focus/accelerator_handler_touch.cc (right): http://codereview.chromium.org/6675005/diff/17001/views/focus/accelerator_handler_touch.cc#newcode180 views/focus/accelerator_handler_touch.cc:180: if (ime && focused_view && focused_view->GetTextInputClient()) { On 2011/03/29 ...

9 years, 9 months ago (2011-03-29 05:00:19 UTC) #21

oshima

a few points that came to my mind. http://codereview.chromium.org/6675005/diff/39001/views/controls/textfield/native_textfield_views.cc File views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/6675005/diff/39001/views/controls/textfield/native_textfield_views.cc#newcode933 views/controls/textfield/native_textfield_views.cc:933: return; ...

9 years, 9 months ago (2011-03-29 19:07:54 UTC) #22

James Su

http://codereview.chromium.org/6675005/diff/39001/views/controls/textfield/native_textfield_views.cc File views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/6675005/diff/39001/views/controls/textfield/native_textfield_views.cc#newcode933 views/controls/textfield/native_textfield_views.cc:933: return; On 2011/03/29 19:07:55, oshima wrote: > can we ...

9 years, 9 months ago (2011-03-29 19:21:51 UTC) #23

oshima

http://codereview.chromium.org/6675005/diff/39001/views/controls/textfield/native_textfield_views.cc File views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/6675005/diff/39001/views/controls/textfield/native_textfield_views.cc#newcode933 views/controls/textfield/native_textfield_views.cc:933: return; On 2011/03/29 19:21:51, James Su wrote: > On ...

9 years, 8 months ago (2011-03-30 21:06:34 UTC) #24

Peng

http://codereview.chromium.org/6675005/diff/39001/views/controls/textfield/native_textfield_views.cc File views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/6675005/diff/39001/views/controls/textfield/native_textfield_views.cc#newcode933 views/controls/textfield/native_textfield_views.cc:933: return; We have several input_method implementations (Ibus, gtk, win, ...

9 years, 8 months ago (2011-03-30 21:28:27 UTC) #25

James Su

http://codereview.chromium.org/6675005/diff/39001/views/controls/textfield/native_textfield_views.cc File views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/6675005/diff/39001/views/controls/textfield/native_textfield_views.cc#newcode933 views/controls/textfield/native_textfield_views.cc:933: return; On 2011/03/30 21:06:34, oshima wrote: > On 2011/03/29 ...

9 years, 8 months ago (2011-03-30 21:30:32 UTC) #26

http://codereview.chromium.org/6675005/diff/39001/views/controls/textfield/na...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/6675005/diff/39001/views/controls/textfield/na...
views/controls/textfield/native_textfield_views.cc:933: return;
On 2011/03/30 21:06:34, oshima wrote:
> On 2011/03/29 19:21:51, James Su wrote:
> > On 2011/03/29 19:07:55, oshima wrote:
> > > can we check this in input_method? is it bad idea?
> > 
> > InputMethod implementations also do this check. This is just a double check.
> > Removing this check won't cause problem, though I'd prefer to keep it, as
the
> > API requires that an un-focused view shouldn't call InputMethod's methods at
> > all.
> > What's your opinion?
> 
> The code that has no effect or speculative code makes it difficult to
understand
> so I'd prefer to avoid it unless there is good reason.
>  Does input_method use CHECK/DCHECK or smiply return?
> 

Existing InputMethod implementations simply return. so it's safe to remove this
check here.

http://codereview.chromium.org/6675005/diff/39001/views/controls/textfield/na...
views/controls/textfield/native_textfield_views.cc:933: return;
On 2011/03/30 21:28:27, Peng wrote:
> We have several input_method implementations (Ibus, gtk, win, and etc). I
think
> all of them have to deal focus issue by themselves.
> 
> 
> On 2011/03/30 21:06:34, oshima wrote:
> > On 2011/03/29 19:21:51, James Su wrote:
> > > On 2011/03/29 19:07:55, oshima wrote:
> > > > can we check this in input_method? is it bad idea?
> > > 
> > > InputMethod implementations also do this check. This is just a double
check.
> > > Removing this check won't cause problem, though I'd prefer to keep it, as
> the
> > > API requires that an un-focused view shouldn't call InputMethod's methods
at
> > > all.
> > > What's your opinion?
> > 
> > The code that has no effect or speculative code makes it difficult to
> understand
> > so I'd prefer to avoid it unless there is good reason.
> >  Does input_method use CHECK/DCHECK or smiply return?
> > 
> 

Yes. I updated comments of these methods to clarify the behavior.

http://codereview.chromium.org/6675005/diff/39001/views/controls/textfield/na...
views/controls/textfield/native_textfield_views.cc:935: InputMethod*
input_method = textfield_->GetInputMethod();
On 2011/03/30 21:06:34, oshima wrote:
> On 2011/03/29 19:21:51, James Su wrote:
> > On 2011/03/29 19:07:55, oshima wrote:
> > > when input_method can be NULL?
> > 
> > Right now, it's only possible in test code, where the input method could be
> set
> > to NULL explicitly to test related corner cases.
> 
> If it shouldn't be null for regular case, I'd prefer CHECK/DCHECK rather than
> passing through.
> It'll make the code simpler too.

I'd prefer to keep this check, otherwise we need special code for dealing with
unittests.

http://codereview.chromium.org/6675005/diff/39001/views/controls/textfield/na...
File views/controls/textfield/native_textfield_views.h (right):

http://codereview.chromium.org/6675005/diff/39001/views/controls/textfield/na...
views/controls/textfield/native_textfield_views.h:251: gfx::Rect cursor_bounds_;
On 2011/03/30 21:06:34, oshima wrote:
> If we can assume input_method is always present, moving this to input method
may
> simplify the code. What do you think?
|cursor_bounds_| is also used in painting code, I doubt if we can remove it
without significant code change.

oshima

http://codereview.chromium.org/6675005/diff/39001/views/controls/textfield/native_textfield_views.cc File views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/6675005/diff/39001/views/controls/textfield/native_textfield_views.cc#newcode933 views/controls/textfield/native_textfield_views.cc:933: return; On 2011/03/30 21:30:32, James Su wrote: > On ...

9 years, 8 months ago (2011-03-30 21:51:25 UTC) #27

http://codereview.chromium.org/6675005/diff/39001/views/controls/textfield/na...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/6675005/diff/39001/views/controls/textfield/na...
views/controls/textfield/native_textfield_views.cc:933: return;
On 2011/03/30 21:30:32, James Su wrote:
> On 2011/03/30 21:28:27, Peng wrote:
> > We have several input_method implementations (Ibus, gtk, win, and etc). I
> think
> > all of them have to deal focus issue by themselves.
> > 
> > 
> > On 2011/03/30 21:06:34, oshima wrote:
> > > On 2011/03/29 19:21:51, James Su wrote:
> > > > On 2011/03/29 19:07:55, oshima wrote:
> > > > > can we check this in input_method? is it bad idea?
> > > > 
> > > > InputMethod implementations also do this check. This is just a double
> check.
> > > > Removing this check won't cause problem, though I'd prefer to keep it,
as
> > the
> > > > API requires that an un-focused view shouldn't call InputMethod's
methods
> at
> > > > all.
> > > > What's your opinion?
> > > 
> > > The code that has no effect or speculative code makes it difficult to
> > understand
> > > so I'd prefer to avoid it unless there is good reason.
> > >  Does input_method use CHECK/DCHECK or smiply return?
> > > 
> > 
> 
> Yes. I updated comments of these methods to clarify the behavior.


Thanks. Just for clarification, I was fine with either

1) "if" outside, and CHECK/DCHECK inside
2) no if outside and "if" inside.

You pick 2) and I'm fine with this too.

http://codereview.chromium.org/6675005/diff/39001/views/controls/textfield/na...
views/controls/textfield/native_textfield_views.cc:935: InputMethod*
input_method = textfield_->GetInputMethod();
On 2011/03/30 21:30:32, James Su wrote:
> On 2011/03/30 21:06:34, oshima wrote:
> > On 2011/03/29 19:21:51, James Su wrote:
> > > On 2011/03/29 19:07:55, oshima wrote:
> > > > when input_method can be NULL?
> > > 
> > > Right now, it's only possible in test code, where the input method could
be
> > set
> > > to NULL explicitly to test related corner cases.
> > 
> > If it shouldn't be null for regular case, I'd prefer CHECK/DCHECK rather
than
> > passing through.
> > It'll make the code simpler too.
> 
> I'd prefer to keep this check, otherwise we need special code for dealing with
> unittests.

Altering the code for unittest to pass sounds wrong direction. Can't we use mock
object for unittest?

http://codereview.chromium.org/6675005/diff/44001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/screen_locker.cc (right):

http://codereview.chromium.org/6675005/diff/44001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screen_locker.cc:699:
lock_window_->native_widget()->ReplaceInputMethod(NULL);
chromeos screen locker doesn't use TextfieldViews. Do we still need this?

James Su

http://codereview.chromium.org/6675005/diff/39001/views/controls/textfield/native_textfield_views.cc File views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/6675005/diff/39001/views/controls/textfield/native_textfield_views.cc#newcode935 views/controls/textfield/native_textfield_views.cc:935: InputMethod* input_method = textfield_->GetInputMethod(); On 2011/03/30 21:51:25, oshima wrote: ...

9 years, 8 months ago (2011-03-30 22:46:38 UTC) #28

oshima

On Wed, Mar 30, 2011 at 2:30 PM, <suzhe@chromium.org> wrote: > > > http://codereview.chromium.org/6675005/diff/39001/views/controls/textfield/native_textfield_views.cc > ...

9 years, 8 months ago (2011-03-31 17:44:17 UTC) #29

On Wed, Mar 30, 2011 at 2:30 PM, <suzhe@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6675005/diff/39001/views/controls/textfield/na...
> File views/controls/textfield/native_textfield_views.cc (right):
>
>
>
http://codereview.chromium.org/6675005/diff/39001/views/controls/textfield/na...
> views/controls/textfield/native_textfield_views.cc:933: return;
> On 2011/03/30 21:06:34, oshima wrote:
>
>> On 2011/03/29 19:21:51, James Su wrote:
>> > On 2011/03/29 19:07:55, oshima wrote:
>> > > can we check this in input_method? is it bad idea?
>> >
>> > InputMethod implementations also do this check. This is just a
>>
> double check.
>
>> > Removing this check won't cause problem, though I'd prefer to keep
>>
> it, as the
>
>> > API requires that an un-focused view shouldn't call InputMethod's
>>
> methods at
>
>> > all.
>> > What's your opinion?
>>
>
>  The code that has no effect or speculative code makes it difficult to
>>
> understand
>
>> so I'd prefer to avoid it unless there is good reason.
>>  Does input_method use CHECK/DCHECK or smiply return?
>>
>
>
> Existing InputMethod implementations simply return. so it's safe to
> remove this check here.
>
>
>
>
http://codereview.chromium.org/6675005/diff/39001/views/controls/textfield/na...
> views/controls/textfield/native_textfield_views.cc:933: return;
> On 2011/03/30 21:28:27, Peng wrote:
>
>> We have several input_method implementations (Ibus, gtk, win, and
>>
> etc). I think
>
>> all of them have to deal focus issue by themselves.
>>
>
>
>  On 2011/03/30 21:06:34, oshima wrote:
>> > On 2011/03/29 19:21:51, James Su wrote:
>> > > On 2011/03/29 19:07:55, oshima wrote:
>> > > > can we check this in input_method? is it bad idea?
>> > >
>> > > InputMethod implementations also do this check. This is just a
>>
> double check.
>
>> > > Removing this check won't cause problem, though I'd prefer to keep
>>
> it, as
>
>> the
>> > > API requires that an un-focused view shouldn't call InputMethod's
>>
> methods at
>
>> > > all.
>> > > What's your opinion?
>> >
>> > The code that has no effect or speculative code makes it difficult
>>
> to
>
>> understand
>> > so I'd prefer to avoid it unless there is good reason.
>> >  Does input_method use CHECK/DCHECK or smiply return?
>> >
>>
>
>
> Yes. I updated comments of these methods to clarify the behavior.
>
>
>
>
http://codereview.chromium.org/6675005/diff/39001/views/controls/textfield/na...
> views/controls/textfield/native_textfield_views.cc:935: InputMethod*
> input_method = textfield_->GetInputMethod();
> On 2011/03/30 21:06:34, oshima wrote:
>
>> On 2011/03/29 19:21:51, James Su wrote:
>> > On 2011/03/29 19:07:55, oshima wrote:
>> > > when input_method can be NULL?
>> >
>> > Right now, it's only possible in test code, where the input method
>>
> could be
>
>> set
>> > to NULL explicitly to test related corner cases.
>>
>
>  If it shouldn't be null for regular case, I'd prefer CHECK/DCHECK
>>
> rather than
>
>> passing through.
>> It'll make the code simpler too.
>>
>
> I'd prefer to keep this check, otherwise we need special code for
> dealing with unittests.
>
>
>
>
http://codereview.chromium.org/6675005/diff/39001/views/controls/textfield/na...
> File views/controls/textfield/native_textfield_views.h (right):
>
>
>
http://codereview.chromium.org/6675005/diff/39001/views/controls/textfield/na...
> views/controls/textfield/native_textfield_views.h:251: gfx::Rect
> cursor_bounds_;
> On 2011/03/30 21:06:34, oshima wrote:
>
>> If we can assume input_method is always present, moving this to input
>>
> method may
>
>> simplify the code. What do you think?
>>
> |cursor_bounds_| is also used in painting code, I doubt if we can remove
> it without significant code change.


Textfield can use cursor bounds from text_input_client (has to be this as
input_method is shared), if it always exists.
I understand that the change won't be small, but I believe it'll be in good
direction, it will reduce the amount of code.
I'm fine if you can look into it later.

- oshima

>
>
> http://codereview.chromium.org/6675005/
>

Peng

http://codereview.chromium.org/6675005/diff/17001/views/focus/accelerator_handler_touch.cc File views/focus/accelerator_handler_touch.cc (right): http://codereview.chromium.org/6675005/diff/17001/views/focus/accelerator_handler_touch.cc#newcode180 views/focus/accelerator_handler_touch.cc:180: if (ime && focused_view && focused_view->GetTextInputClient()) { On 2011/03/29 ...

9 years, 8 months ago (2011-03-31 19:18:58 UTC) #30

James Su

http://codereview.chromium.org/6675005/diff/17001/views/focus/accelerator_handler_touch.cc File views/focus/accelerator_handler_touch.cc (right): http://codereview.chromium.org/6675005/diff/17001/views/focus/accelerator_handler_touch.cc#newcode180 views/focus/accelerator_handler_touch.cc:180: if (ime && focused_view && focused_view->GetTextInputClient()) { On 2011/03/31 ...

9 years, 8 months ago (2011-03-31 19:23:00 UTC) #31

Peng

On 2011/03/31 19:23:00, James Su wrote: > http://codereview.chromium.org/6675005/diff/17001/views/focus/accelerator_handler_touch.cc > File views/focus/accelerator_handler_touch.cc (right): > > http://codereview.chromium.org/6675005/diff/17001/views/focus/accelerator_handler_touch.cc#newcode180 ...

9 years, 8 months ago (2011-03-31 21:19:59 UTC) #32

Peng

9 years, 8 months ago (2011-03-31 21:22:53 UTC) #33

On 2011/03/31 21:19:59, Peng wrote:
> On 2011/03/31 19:23:00, James Su wrote:
> >
>
http://codereview.chromium.org/6675005/diff/17001/views/focus/accelerator_han...
> > File views/focus/accelerator_handler_touch.cc (right):
> > 
> >
>
http://codereview.chromium.org/6675005/diff/17001/views/focus/accelerator_han...
> > views/focus/accelerator_handler_touch.cc:180: if (ime && focused_view &&
> > focused_view->GetTextInputClient()) {
> > On 2011/03/31 19:18:58, Peng wrote:
> > > On 2011/03/29 05:00:20, James Su wrote:
> > > > On 2011/03/29 03:04:46, Peng wrote:
> > > > > Did you fix this?
> > Yes, it's fixed already.
> > 
> > > > > 
> > > > > On 2011/03/24 03:58:43, Peng wrote:
> > > > > > If the focused_view->GetTextInputClient() is NULL, the key event
will
> > not
> > > be
> > > > > > dispatched to input method.
> > > > > > 
> > > > > > But for ChromeOS, IME switch hotkey should work, even if the focused
> > view
> > > > does
> > > > > > not support input method. 
> > > > > 
> > > > 
> > > > Good point. I'll fix it asap.
> > > 
> > > Did you fix this one?
> 
> Thanks. I saw the code in this CL. But it still leak some code in
> InputMethodIBus for this feature.
> 
> LGTM.

Sorry. leak => lack.

Issue 6675005: Integrate the new input method API for Views into Chromium. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Fix compile error. #

Patch Set 3 : Add some DCHECKs. #

Patch Set 4 : Address comment feedbacks. #

Patch Set 5 : Fix trybot failures. #

Patch Set 6 : Adress review feedbacks. #

Patch Set 7 : Rebase. #

Patch Set 8 : Fix bugs related to TouchUI. #

Patch Set 9 : Fix a weird compile error on trybots. #

Patch Set 10 : Rebase. #

Patch Set 11 : Disable input method if TextfieldViews is not enabled. #

Patch Set 12 : Enable GetTextFromRange. #

Patch Set 13 : Rebase #

Patch Set 14 : Update copyright headers. #

Patch Set 15 : Update according to review feedback. #

Patch Set 16 : TouchUI: Always dispatch key events to the input method. #

Patch Set 17 : Update. #

Patch Set 18 : Rebase #

Patch Set 19 : Rename OnFocusIn/OnFocusOut to OnFocus/OnBlur. #

Patch Set 20 : Fix unittest. #

Patch Set 21 : Remove redundant check of focus state. #

Patch Set 22 : Update. #

Patch Set 23 : Rebase. #

Patch Set 24 : Rebase #

Patch Set 25 : Fix Windows build. #

Messages