Bug 43922

Summary: Support for multiple speech enabled elements in same page.
Product: WebKit Reporter: Satish Sampath <satish>
Component: FormsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, jorlow, steveblock
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 39485    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Satish Sampath
Reported 2010-08-12 09:00:49 PDT
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.
Attachments
Patch (30.42 KB, patch)
2010-08-12 09:59 PDT, Satish Sampath 		no flags
DetailsFormatted DiffDiff
Patch (30.43 KB, patch)
2010-08-13 08:06 PDT, Satish Sampath 		no flags
DetailsFormatted DiffDiff
Patch (30.39 KB, patch)
2010-08-13 08:30 PDT, Satish Sampath 		no flags
DetailsFormatted DiffDiff
Patch (30.39 KB, patch)
2010-08-13 08:35 PDT, Satish Sampath 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Satish Sampath
Comment 1 2010-08-12 09:59:19 PDT
Created attachment 64231 [details] Patch
Jeremy Orlow
Comment 2 2010-08-13 04:54:06 PDT
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?
Satish Sampath
Comment 3 2010-08-13 07:51:06 PDT
> 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.
Jeremy Orlow
Comment 4 2010-08-13 07:56:40 PDT
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?
Satish Sampath
Comment 5 2010-08-13 08:06:43 PDT
Created attachment 64339 [details] Patch Added ASSERT_NOT_REACHED() per Jeremy.
Jeremy Orlow
Comment 6 2010-08-13 08:09:50 PDT
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.
Satish Sampath
Comment 7 2010-08-13 08:30:23 PDT
Created attachment 64344 [details] Patch Placed the check under #ifdef debug and assert that the given listener is already present.
Jeremy Orlow
Comment 8 2010-08-13 08:32:39 PDT
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.
Satish Sampath
Comment 9 2010-08-13 08:35:20 PDT
Created attachment 64347 [details] Patch Move to a single line for loop (sorry, my bad..)
Jeremy Orlow
Comment 10 2010-08-13 08:47:04 PDT
Comment on attachment 64347 [details] Patch r=me
WebKit Commit Bot
Comment 11 2010-08-13 09:49:55 PDT
Comment on attachment 64347 [details] Patch Clearing flags on attachment: 64347 Committed r65327: <http://trac.webkit.org/changeset/65327>
WebKit Commit Bot
Comment 12 2010-08-13 09:50:00 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.