Bug 103407 – Speech Recognition API: Update SpeechRecognitionEvent to match the specification

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Tommy Widenflycht]

This will be the first of a few patches that adds the new results attribute, and then later removes the deprecated result attribute.

[Tommy Widenflycht]

Created attachment 176250 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Tommy Widenflycht]

Hans & Adam: What would you like me to do regarding tests? Should I convert them (and extend them) directly or is it OK to do in an follow-up patch?
Is it important to keep the tests for the old API until the new API is completely hooked up in Chromium?

[Hans Wennborg]

Comment on attachment 176250 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176250&action=review

> Source/WebCore/Modules/speech/SpeechRecognitionEvent.h:46
> +    // DEPRECATED

nit: I'm not sure "// DEPRECATED" is common in WebKit. That also suggests to me that the code might be around a while, which is not the case here :) I think a "// FIXME: Remove these after embedder has been updated." is more clear. This applies below as well.

> Source/WebCore/Modules/speech/SpeechRecognitionEvent.idl:34
> +    [InitializedByEventConstructor] readonly attribute short resultIndex;

s/short/long/ here?

> Source/WebKit/chromium/public/WebSpeechRecognizerClient.h:76
> +    // the last time the functions was called.

nit: s/the functions/the function/

[Hans Wennborg]

Thanks Tommy! This looks great to me.

(In reply to comment #3)
> Hans & Adam: What would you like me to do regarding tests? Should I convert them (and extend them) directly or is it OK to do in an follow-up patch?

I don't have a strong opinion here, though I suppose sooner is better than later in most cases.

> Is it important to keep the tests for the old API until the new API is completely hooked up in Chromium?

I don't think that is important.

[Adam Barth]

> > Hans & Adam: What would you like me to do regarding tests? Should I convert them (and extend them) directly or is it OK to do in an follow-up patch?
> 
> I don't have a strong opinion here, though I suppose sooner is better than later in most cases.

I'd write the tests in the same patch as the code change.

> > Is it important to keep the tests for the old API until the new API is completely hooked up in Chromium?
> 
> I don't think that is important.

Yeah, we don't need to test the old API during the transition.

[Adam Barth]

Comment on attachment 176250 [details]
Patch

Looks great, but please update the tests.  :)

[Tommy Widenflycht]

Created attachment 176717 [details]
Patch

[Tommy Widenflycht]

The size increased due to me having to move the emma attribute in this patch as well.
I also took the opportunity to rename final to isFinal since final is a reserved keyword in JS, the specification will be updated asap.

[Adam Barth]

Comment on attachment 176717 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176717&action=review

> Source/WebCore/Modules/speech/SpeechRecognitionEvent.cpp:118
> +

you've got an extra line here.

[Tommy Widenflycht]

Created attachment 176918 [details]
Patch for landing

[Tommy Widenflycht]

Comment on attachment 176717 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176717&action=review

>> Source/WebCore/Modules/speech/SpeechRecognitionEvent.cpp:118
>> +
> 
> you've got an extra line here.

fixed.

[WebKit Review Bot]

Comment on attachment 176918 [details]
Patch for landing

Rejecting attachment 176918 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

/mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/15065117

[Tommy Widenflycht]

Created attachment 176920 [details]
Patch for landing

[Tommy Widenflycht]

Committed r136236: <http://trac.webkit.org/changeset/136236>

[WebKit Review Bot]

Re-opened since this is blocked by bug 103833

[Tommy Widenflycht]

Created attachment 177206 [details]
Patch

[Tommy Widenflycht]

I have removed all emma related functionality from the latest patch since the magic v8 code was triggered for more than just the correct class. All other code is identical.

[Kentaro Hara]

(In reply to comment #18)
> I have removed all emma related functionality from the latest patch since the magic v8 code was triggered for more than just the correct class.

Would you elaborate on this?

[Tommy Widenflycht]

(In reply to comment #19)
> (In reply to comment #18)
> > I have removed all emma related functionality from the latest patch since the magic v8 code was triggered for more than just the correct class.
> 
> Would you elaborate on this?

See the rollout bug: https://bugs.webkit.org/show_bug.cgi?id=103833

Since the emma attribute is much less important that the switch to a list of results I am removing it for now while investigating.

[Kentaro Hara]

Comment on attachment 177206 [details]
Patch

Looks reasonable.

Please add the rationale for removing emma to ChangeLog. Also the spec link could be added to ChangeLog.

[Tommy Widenflycht]

Created attachment 177213 [details]
Patch

[Tommy Widenflycht]

Comment on attachment 176250 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176250&action=review

>> Source/WebCore/Modules/speech/SpeechRecognitionEvent.idl:34
>> +    [InitializedByEventConstructor] readonly attribute short resultIndex;
> 
> s/short/long/ here?

fixed.

>> Source/WebKit/chromium/public/WebSpeechRecognizerClient.h:76
>> +    // the last time the functions was called.
> 
> nit: s/the functions/the function/

fixed.

[Tommy Widenflycht]

(In reply to comment #21)
> (From update of attachment 177206 [details])
> Looks reasonable.
> 
> Please add the rationale for removing emma to ChangeLog. Also the spec link could be added to ChangeLog.

Fixed, please take another look since I corrected a type and had to change the Dictionary classes.

[WebKit Review Bot]

Comment on attachment 177213 [details]
Patch

Clearing flags on attachment: 177213

Committed r136392: <http://trac.webkit.org/changeset/136392>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.