This will be the first of a few patches that adds the new results attribute, and then later removes the deprecated result attribute.
Created attachment 176250 [details] Patch
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.
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 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/
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.
> > 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 on attachment 176250 [details] Patch Looks great, but please update the tests. :)
Created attachment 176717 [details] Patch
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 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.
Created attachment 176918 [details] Patch for landing
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 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
Created attachment 176920 [details] Patch for landing
Committed r136236: <http://trac.webkit.org/changeset/136236>
Re-opened since this is blocked by bug 103833
Created attachment 177206 [details] Patch
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.
(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?
(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 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.
Created attachment 177213 [details] Patch
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.
(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 on attachment 177213 [details] Patch Clearing flags on attachment: 177213 Committed r136392: <http://trac.webkit.org/changeset/136392>
All reviewed patches have been landed. Closing bug.