[webkit-reviews] review denied: [Bug 60451] speech input doesn't fire textInput events : [Attachment 106319] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Leandro Graciá Gil
<leandrogracia at chromium.org>'s request for review:
Bug 60451: speech input doesn't fire textInput events
https://bugs.webkit.org/show_bug.cgi?id=60451

Attachment 106319: Patch
https://bugs.webkit.org/attachment.cgi?id=106319&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=106319&action=review


The concept here is right - textInput events should definitely be fired for any
input. r- for failing test, and for my comments that likely require code
changes.

> Source/WebCore/dom/TextEventInputType.h:38
> +    TextEventInputVoice,

Why add a new unused enum for voice? It's not exposed to clients, and all it
can do is confuse developers (as it's not set for regular voice input, but only
for Chromium's custom speech input feature).

I suggest adding an "other" value. Or perhaps you should use one of the
existing values, like TextEventInputKeyboard, so that callers that check for
that work correctly?

TextEventInputType.h code smells very wrong, so you should be extra careful
with it. Look at TextEventInputLineBreak - its comment says "any tab characters
in the text are backtabs". Other options may have no relation to their names,
too.

> Source/WebCore/html/shadow/TextControlInnerElements.cpp:495
> +    // Sending the text event first to avoid incoherent situations where the
speech change event is used to select an alternative result.
> +    ASSERT(document()->domWindow());
> +    input->dispatchEvent(TextEvent::create(document()->domWindow(),
results.isEmpty() ? "" : results[0]->utterance(), TextEventInputVoice));
>     
input->dispatchEvent(SpeechInputEvent::create(eventNames().webkitspeechchangeEv
ent, results));

These events look quite irregular. Why is there a webkitspeechchange event, in
the first place?

What happens if preventDefault() is called by textInput event handler? You just
go ahead and dispatch webkitspeechchange, which seems wrong.