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

Issue 147010: Revert 19009 because this prevents inserting return characters.... (Closed)

Created:
11 years, 6 months ago by Hironori Bono
Modified:
9 years, 7 months ago
Reviewers:
Evan Martin
CC:
chromium-reviews_googlegroups.com, darin (slow to review), Ben Goodger (Google)
Visibility:
Public.

Description

Revert 19009 because this prevents inserting return characters. TBR=evan BUG=10953 "IME support" BUG=11226 "Dead keys and accents input not working" BUG=13604 "Hotkeys not working in non-us keyboard layout" TEST=none Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=19015

Patch Set 1 #

Patch Set 2 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+4 lines, -324 lines) Patch
M chrome/browser/renderer_host/render_widget_host.h View 1 chunk +0 lines, -39 lines 0 comments Download
M chrome/browser/renderer_host/render_widget_host.cc View 1 chunk +0 lines, -22 lines 0 comments Download
M chrome/browser/renderer_host/render_widget_host_view_gtk.h View 2 chunks +0 lines, -23 lines 0 comments Download
M chrome/browser/renderer_host/render_widget_host_view_gtk.cc View 5 chunks +2 lines, -117 lines 0 comments Download
MM chrome/common/native_web_keyboard_event.h View 1 1 chunk +0 lines, -1 line 0 comments Download
MM chrome/common/native_web_keyboard_event_linux.cc View 1 1 chunk +0 lines, -7 lines 0 comments Download
M webkit/api/public/gtk/WebInputEventFactory.h View 1 chunk +0 lines, -1 line 0 comments Download
M webkit/api/src/gtk/WebInputEventFactory.cpp View 4 chunks +2 lines, -114 lines 0 comments Download

Messages

Total messages: 4 (0 generated)
Hironori Bono
I'm sorry, I noticed my r19009 causes a serious Issue 15024. (I forgot handling a ...
11 years, 6 months ago (2009-06-23 10:50:56 UTC) #1
Matt Tolton
> The reviewer for that patch should have requested a unit test. It's > usually ...
11 years, 4 months ago (2009-08-24 17:11:14 UTC) #2
brettw
On Mon, Aug 24, 2009 at 10:11 AM, Matt Tolton<matt@tolton.com> wrote: >> The reviewer for ...
11 years, 4 months ago (2009-08-24 17:15:18 UTC) #3
Hironori Bono
11 years, 4 months ago (2009-08-25 02:34:22 UTC) #4
Hi Matt,

Sorry for making you feel off with my comments and changes that don't
have unit tests.
By the way, I have been adding input-related unit tests that verifies
our input back-end (e.g. "RenderViewTest.ImeComposition",
"RenderViewTest.OnHandleKeyboardEvent",
"RenderViewTest.InsertCharacters", etc.) Before integrating IME
support into Mac and Linux Chrome, I checked if
RenderViewTest.ImeComposition works well with them, and I checked
whether or not I need to add more test cases to the test. My first IME
change for Linux was very buggy because it doesn't implement anything
not covered by the unit tests. :) Unfortunately,
"RenderViewTest.OnHandleKeyboardEvent" and
"RenderViewTest.InsertCharacters" were disabled on Linux and Mac. I
have been working for re-enabling them on Mac and Linux. Even though I
asked James to add unit tests just because these unit tests cannot
test his change, I apologize all if my previous comment made you feel
off.

On the other hand, it is definitely my fault that I wasn't able to add
any automated tests that test our input front-ends. It is just because
we cannot test an IME on a machine that doesn't have the IME
installed. Even though I'm implementing a semi-automatic test that
tests our input front-ends (and IMEs), it is still very flaky.

Regards,

Hironori Bono
E-mail: hbono@chromium.org

On Tue, Aug 25, 2009 at 2:11 AM, Matt Tolton<matt@tolton.com> wrote:
>
>> The reviewer for that patch should have requested a unit test. It's
>> usually easier to see this need in other's patches than your own,
>> which is why all reviewers need to be on the lookout for this.
>>
>> That doesn't affect this patch at all, and I think the tone of the
>> question was very disrespectful.
>
> Hi Brett,
>
> My apologies, I sincerely did not intend disrespect. =A0I also seem to
> have linked to the wrong patch, hbono also added IME support to linux
> (which is the one I meant to link to):
>
> http://codereview.chromium.org/147010
>
> The point I was trying to make is that hbono seems to be concerned
> about regressions in this IME code with James' patch, but he wrote no
> unit test to try to help catch regressions. =A0This feels off to me.
>
> Again, my apologies if the tone came across as disrespectful.
>

Powered by Google App Engine
This is Rietveld 408576698