Revert 19009 because this prevents inserting return characters....

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

[Hironori Bono, 2009-06-23 10:50:56]

I'm sorry, I noticed my r19009 causes a serious Issue 15024. (I forgot handling
a case that gtk_im_context_filter_keypress() returns false.)
When I fix this issue, I'm going to re-send this change.

[Matt Tolton, 2009-08-24 17:11:14]

> 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.  I 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.  This feels off to me.

Again, my apologies if the tone came across as disrespectful.

[brettw, 2009-08-24 17:15:18]

On Mon, Aug 24, 2009 at 10: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.

Thanks. We should make sure all these areas are well tested. If you
notice any reviews going out that need tests, please speak up. If
we're missing tests for checked-in code, probably the best thing to do
is file a bug.

Brett

[Hironori Bono, 2009-08-25 02:34:22]

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.
>