[webkit-reviews] review denied: [Bug 10871] REGRESSION: IME key events different in nightly : [Attachment 13499] patch that doesn't break autorepeat

bugzilla-request-daemon at macosforge.org bugzilla-request-daemon at macosforge.org
Wed Mar 7 06:19:43 PST 2007


Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 10871: REGRESSION: IME key events different in nightly
http://bugs.webkit.org/show_bug.cgi?id=10871

Attachment 13499: patch that doesn't break autorepeat
http://bugs.webkit.org/attachment.cgi?id=13499&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
I would like to see handleKeyPress divided into two separate calls rather than
a single one with a boolean. In my mind, one of the calls is "allow input
methods to handle key"; because of AppKit that one has a side effect of
determining what the action is, but that's not the purpose of the call, rather
an unfortunate side effect. Another call is "do the usual key press handling".
I don't know what names to use but I think it's better to not use one call for
both.

The second call could be the one to use the "saved action".

The name keypressSelectorString is Macintosh-specific. If it's really going to
be Mac-specific, then maybe it should be in #if PLATFORM(MAC) and be a SEL. On
the other hand, if it's going to be platform-independent then it should be
called "command" rather than selector. I don't think the names "saved action"
and "keypressSelectorString" and "keypressText" are named in a way that makes
it clear they're connected; if the keypressSelectorString and keypressText were
instead a struct named SavedAction or something along those lines it would be
clearer. I think terminology could be important here for people understanding
this, especially people on platforms that don't have the strange quirk Mac does
where we have to do both things! We could treat this need to cache the command
on the keyboard event as a Mac-specific thing -- that might make things easier
for people on other platforms who don't need this although it would make
KeyboardEvent.h uglier.

This breaks non-Mac platforms, right, because of the new parameter to the
EditorClient function?

+	 event->setKeypressSelectorString(String((char*)aSelector));

The above code is incorrect. You need to either use NSStringFromSelector or
sel_getName. I think the above code might even fail on Leopard, although it
will work on Tiger.

+	 // The only way we know if its from an input method is to check
hasMarkedText.	There might be a better way to do this.

I think "there might be a better way" is confusing, even though it's true. I
think the comment should say, "We assume it's from the input method if we have
marked text." and an on another line "FIXME: In theory that could be wrong for
some input methods, so we should look for another way to determine if the call
is from the input method."

The substance of the code looks great.

I'd like to see some new layout tests -- is there no way to activate an input
method from a layout test? We should consider how to make this part of
regression tests.

review- specifically because of the "cast selector to char*" issue. If you
resolve that it's OK to land without further review, but please consider my
other comments too.



More information about the webkit-reviews mailing list