Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(66)

Issue 5372001: [cros] Fix two issues related to gtk im context support. (Closed)

Created:
10 years ago by James Su
Modified:
9 years, 7 months ago
Reviewers:
satorux1
CC:
chromium-reviews, ben+cc_chromium.org, darin-cc_chromium.org, brettw-cc_chromium.org
Visibility:
Public.

Description

[cros] Fix two issues related to gtk im context support. BUG=chromium-os:9208 BUG=chromium-os:9575 TEST=See bug reports. Issue 9208 is due to ibus's async nature: some ibus engines may update preedit string when getting reset. It may happen after calling GtkIMContextWrapper::CancelComposition() method, which then cause this method being called recursively. So we need to suppress any preedit string signals triggered by GtkIMContextWrapper::CancelComposition() method, just like what we have done for commit signal (http://crbug.com/50485 and issue http://crosbug.com/4792). Issue 9575 is caused by improper handling of "grab-notify" signal in RWHVGtk, which should not focus in the input context again when the window has been focused out. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=67207

Patch Set 1 #

Total comments: 1

Patch Set 2 : Update according to review feedback. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+10 lines, -1 line) Patch
M chrome/browser/renderer_host/gtk_im_context_wrapper.cc View 1 1 chunk +7 lines, -1 line 0 comments Download
M chrome/browser/renderer_host/render_widget_host_view_gtk.cc View 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 2 (0 generated)
James Su
10 years ago (2010-11-24 01:47:39 UTC) #1
satorux1
10 years ago (2010-11-24 01:56:02 UTC) #2
LGTM. Thank you for fixing the important bugs. Can you merge this to 552d?

http://codereview.chromium.org/5372001/diff/1/chrome/browser/renderer_host/gt...
File chrome/browser/renderer_host/gtk_im_context_wrapper.cc (right):

http://codereview.chromium.org/5372001/diff/1/chrome/browser/renderer_host/gt...
chrome/browser/renderer_host/gtk_im_context_wrapper.cc:459: if
(!suppress_next_commit_)
You might want to make the code look like line 466-468, for consistency:

if (suppress_next_commit_)
  return;

Powered by Google App Engine
This is Rietveld 408576698