Bug 103407

Summary: Speech Recognition API: Update SpeechRecognitionEvent to match the specification
Product: WebKit Reporter: Tommy Widenflycht <tommyw>
Component: WebCore Misc.Assignee: Tommy Widenflycht <tommyw>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, fishd, hans, haraken, jamesr, japhet, ojan, tkent+wkapi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 103833    
Bug Blocks: 80261    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing
webkit.review.bot: commit-queue-
Patch for landing
none
Patch
none
Patch none

Description Tommy Widenflycht 2012-11-27 06:00:56 PST
This will be the first of a few patches that adds the new results attribute, and then later removes the deprecated result attribute.
Comment 1 Tommy Widenflycht 2012-11-27 06:07:13 PST
Created attachment 176250 [details]
Patch
Comment 2 WebKit Review Bot 2012-11-27 06:09:07 PST
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Tommy Widenflycht 2012-11-27 06:12:54 PST
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?
Comment 4 Hans Wennborg 2012-11-27 06:17:20 PST
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/
Comment 5 Hans Wennborg 2012-11-27 06:18:37 PST
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.
Comment 6 Adam Barth 2012-11-27 09:45:38 PST
> > 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.
Comment 7 Adam Barth 2012-11-27 09:47:30 PST
Comment on attachment 176250 [details]
Patch

Looks great, but please update the tests.  :)
Comment 8 Tommy Widenflycht 2012-11-29 06:46:24 PST
Created attachment 176717 [details]
Patch
Comment 9 Tommy Widenflycht 2012-11-29 06:51:02 PST
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.
Comment 10 Adam Barth 2012-11-29 08:55:22 PST
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.
Comment 11 Tommy Widenflycht 2012-11-30 01:17:04 PST
Created attachment 176918 [details]
Patch for landing
Comment 12 Tommy Widenflycht 2012-11-30 01:18:32 PST
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.
Comment 13 WebKit Review Bot 2012-11-30 01:23:36 PST
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
Comment 14 Tommy Widenflycht 2012-11-30 01:28:28 PST
Created attachment 176920 [details]
Patch for landing
Comment 15 Tommy Widenflycht 2012-11-30 07:10:31 PST
Committed r136236: <http://trac.webkit.org/changeset/136236>
Comment 16 WebKit Review Bot 2012-12-02 00:48:21 PST
Re-opened since this is blocked by bug 103833
Comment 17 Tommy Widenflycht 2012-12-03 01:17:39 PST
Created attachment 177206 [details]
Patch
Comment 18 Tommy Widenflycht 2012-12-03 01:19:40 PST
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.
Comment 19 Kentaro Hara 2012-12-03 01:23:41 PST
(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?
Comment 20 Tommy Widenflycht 2012-12-03 01:27:15 PST
(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.
Comment 21 Kentaro Hara 2012-12-03 01:42:24 PST
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.
Comment 22 Tommy Widenflycht 2012-12-03 02:15:07 PST
Created attachment 177213 [details]
Patch
Comment 23 Tommy Widenflycht 2012-12-03 02:15:49 PST
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.
Comment 24 Tommy Widenflycht 2012-12-03 02:16:50 PST
(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.
Comment 25 WebKit Review Bot 2012-12-03 06:28:21 PST
Comment on attachment 177213 [details]
Patch

Clearing flags on attachment: 177213

Committed r136392: <http://trac.webkit.org/changeset/136392>
Comment 26 WebKit Review Bot 2012-12-03 06:28:26 PST
All reviewed patches have been landed.  Closing bug.