The current speech input plumbing in webkit and webcore does not work when there are more than 1 speech enabled input elements. The main issue is that webcore presents a single listener to webkit/embedder and this listener gets invoked without any knowledge of the target element for the events. This had forced the webcore speech code to allow only one speech recognition session at a time, which isn't useful because even though only one element can record audio at a time, multiple such recorded streams can be in recognition phase in parallel (since recognition takes time and may even involve a server roundtrip). This patch has the following changes to support the above (very useful) case: - WebCore::SpeechInputClient now requires a one time set for the WebCore::SpeechInputListener and takes in a 'requestId' for all calls. This requestId is returned back in all the listener callbacks for identifying which input element the event goes to. - WebCore::SpeechInput class generates the above mentioned request ids as necessary when each speech enabled input element gets created/destroyed and multiplexes the listener callbacks to the appropriate input element based on the request id. - All methods of WebKit::WebSpeechInputController (implemented by embedder) and WebKit::WebSpeechInputListener now require the requestId parameter as well. - The relevant mocks updated.
Created attachment 64231 [details] Patch
Comment on attachment 64231 [details] Patch WebCore/page/SpeechInput.h:74 + HashMap<int, SpeechInputListener*> m_listeners; Why do you need ids? Why not just keep track of listeners via their pointers? Will you only be able to cancel recording from WebKit, and not cancel processing? If so, I think this might work since only one input box should ever be recording at once. In fact, if you get a message from one while another is recording, I think you can take that as a signal to end recording in the other. And that means only having one listener pointer at a time works fine. Why does everything ened IDs though? The multiple speech input boxes can all share the same backend since only one item will ever be listening at once...right? If you can cancel processing from the controls, I guess it's more complicated because you could have multiple doing processing while another is recording and so on. In that case, I feel like you could have a pointer to the recording object and a set of processing objects. In that case, using IDs probably makes sense. But can't the IDs just be memory addresses? It seems a bit simpler than trying to dole out unique IDs. Btw, can you try making a diagram (or even hand-drawing and scanning it in) of all the classes and how they interact and attach it to the top level bug?
> I guess it's more complicated because you could have multiple > doing processing while another is recording and so on That is correct, and this patch gets these cases working. Jeremy and I discussed this more, and the map of IDs<->listeners seems like the right way to go so that if an input element gets destroyed and that memory was reallocated for another of the same type, we don't accidentally forward invalid callbacks to the new one.
Comment on attachment 64231 [details] Patch actually, leave it as ints r=meWebCore/page/SpeechInput.cpp:57 + for (HashMap<int, SpeechInputListener*>::iterator it = m_listeners.begin(); Maybe only do this in debug mode? Or at least assert not reached that you hit it?
Created attachment 64339 [details] Patch Added ASSERT_NOT_REACHED() per Jeremy.
Comment on attachment 64339 [details] Patch well...assert it's not reached and return...or put the whole thing in #if defined(DEBUG) and do an ASSERT(it->second == listener) instead.
Created attachment 64344 [details] Patch Placed the check under #ifdef debug and assert that the given listener is already present.
Comment on attachment 64344 [details] Patch WebCore/page/SpeechInput.cpp:59 + it != m_listeners.end(); ++it) sorry...one more nit....either put the whole for on one line or put {}'s around it...otherwise it's hard to follow.
Created attachment 64347 [details] Patch Move to a single line for loop (sorry, my bad..)
Comment on attachment 64347 [details] Patch r=me
Comment on attachment 64347 [details] Patch Clearing flags on attachment: 64347 Committed r65327: <http://trac.webkit.org/changeset/65327>
All reviewed patches have been landed. Closing bug.