Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10933018:
Speech JavaScript API: Use the MediaStreamManager to ask for user permission. (Closed)
|
Created:
8 years, 3 months ago by hans Modified:
8 years, 3 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSpeech JavaScript API: Use the MediaStreamManager to ask for user permission.
BUG=132965
TEST=manual
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156615
Patch Set 1 #
Total comments: 23
Patch Set 2 : Addressing Satish's comments #
Total comments: 10
Patch Set 3 : Address xians's comments #
Total comments: 1
Patch Set 4 : Rebase #Patch Set 5 : Remove media_stream_manager_ #
Total comments: 8
Patch Set 6 : Address nits from xians #Messages
Total messages: 22 (0 generated)
Here's a new patch for asking for user's permission for the JS speech API. The tricky bit is that we need to do checks in two places: We need to be in chrome/ and on the UI thread to check the RenderViewHost and which type of view it is, but then we need to be in content/ to use the MediaStreamManager. One issue is that the MediaStreamManager does not take down the infobar when a request is aborted. This is a known TODO that they're working on, though.
tommyw: Perhaps you can take a look to see if this makes sense from a media stream perspective?
Good start! https://chromiumcodereview.appspot.com/10933018/diff/1/chrome/browser/speech/... File chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc (right): https://chromiumcodereview.appspot.com/10933018/diff/1/chrome/browser/speech/... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:570: void ChromeSpeechRecognitionManagerDelegate::CheckRenderViewTypeForTrayIcon( there is a lot of shared code between this and the previous method. How about passing in an additional bool to indicate if it is for the JS API or x-webkit-speech API and merge the two methods? https://chromiumcodereview.appspot.com/10933018/diff/1/content/browser/speech... File content/browser/speech/speech_recognition_manager_impl.cc (left): https://chromiumcodereview.appspot.com/10933018/diff/1/content/browser/speech... content/browser/speech/speech_recognition_manager_impl.cc:464: DCHECK(session.recognizer.get() && session.recognizer->IsCapturingAudio()); why was the second condition removed? https://chromiumcodereview.appspot.com/10933018/diff/1/content/browser/speech... File content/browser/speech/speech_recognition_manager_impl.cc (right): https://chromiumcodereview.appspot.com/10933018/diff/1/content/browser/speech... content/browser/speech/speech_recognition_manager_impl.cc:149: DCHECK(SessionExists(session_id)); this is not new code, but is it possible that in the time control goes to the UI thread and comes back the session had got deleted, either due to JS API aborting the start call or something else? If it is a valid scenario lets check and do an early return instead of a dcheck https://chromiumcodereview.appspot.com/10933018/diff/1/content/browser/speech... content/browser/speech/speech_recognition_manager_impl.cc:602: : media_stream_manager_(media_stream_manager), indent to right by 2 spaces https://chromiumcodereview.appspot.com/10933018/diff/1/content/browser/speech... content/browser/speech/speech_recognition_manager_impl.cc:611: media_stream_manager_->GenerateStream( these method names don't seem to make sense when called for bringing up an infobar for speech reco permission. Perhaps they need to be renamed to be more generic? Hmm this actually feels like we are using a class in MediaStream that is a bit higher than what we actually need.. i.e. a manager class that shows infobars at the right time than the infobar class itself. Looking at the class declaration that seems to be the case. Can just the infobar and permissions management code be extracted out to a smaller class and that can be reused by speech recognition? https://chromiumcodereview.appspot.com/10933018/diff/1/content/browser/speech... content/browser/speech/speech_recognition_manager_impl.cc:621: media_stream_manager_->CancelGenerateStream(label_); ditto https://chromiumcodereview.appspot.com/10933018/diff/1/content/browser/speech... content/browser/speech/speech_recognition_manager_impl.cc:628: remove empty line https://chromiumcodereview.appspot.com/10933018/diff/1/content/browser/speech... File content/browser/speech/speech_recognition_manager_impl.h (right): https://chromiumcodereview.appspot.com/10933018/diff/1/content/browser/speech... content/browser/speech/speech_recognition_manager_impl.h:130: bool is_waiting_for_permission; instead of this flag in each Session object, how about storing the session_id in the PermissionRequest class and check with the other session_id value when necessary? That would reduce a few lines of code https://chromiumcodereview.appspot.com/10933018/diff/1/content/browser/speech... content/browser/speech/speech_recognition_manager_impl.h:179: class PermissionRequest : public media_stream::MediaStreamRequester { can this be forward declared in this header and the actual definition be moved to the .cc file? https://chromiumcodereview.appspot.com/10933018/diff/1/content/public/common/... File content/public/common/speech_recognition_error.h (right): https://chromiumcodereview.appspot.com/10933018/diff/1/content/public/common/... content/public/common/speech_recognition_error.h:20: SPEECH_RECOGNITION_ERROR_NOT_ALLOWED, I don't have a strong preference here, but this could also be named as SPEECH_RECOGNITION_ERROR_DENIED.. up to you to decide which is appropriate https://chromiumcodereview.appspot.com/10933018/diff/1/content/renderer/speec... File content/renderer/speech_recognition_dispatcher.cc (left): https://chromiumcodereview.appspot.com/10933018/diff/1/content/renderer/speec... content/renderer/speech_recognition_dispatcher.cc:62: // TODO(primiano): Should return false in order to communicate the failure Is this being removed because a different change already handles it?
My chromium-fu is low but I added some relevant people as reviewers.
Thanks for the review! New patch uploaded. https://chromiumcodereview.appspot.com/10933018/diff/1/chrome/browser/speech/... File chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc (right): https://chromiumcodereview.appspot.com/10933018/diff/1/chrome/browser/speech/... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:570: void ChromeSpeechRecognitionManagerDelegate::CheckRenderViewTypeForTrayIcon( On 2012/09/12 06:15:09, Satish wrote: > there is a lot of shared code between this and the previous method. How about > passing in an additional bool to indicate if it is for the JS API or > x-webkit-speech API and merge the two methods? Done. https://chromiumcodereview.appspot.com/10933018/diff/1/content/browser/speech... File content/browser/speech/speech_recognition_manager_impl.cc (left): https://chromiumcodereview.appspot.com/10933018/diff/1/content/browser/speech... content/browser/speech/speech_recognition_manager_impl.cc:464: DCHECK(session.recognizer.get() && session.recognizer->IsCapturingAudio()); On 2012/09/12 06:15:09, Satish wrote: > why was the second condition removed? Because it's possible to stop or abort a session while it's waiting for permission, in which case it hasn't started capturing audio yet. https://chromiumcodereview.appspot.com/10933018/diff/1/content/browser/speech... File content/browser/speech/speech_recognition_manager_impl.cc (right): https://chromiumcodereview.appspot.com/10933018/diff/1/content/browser/speech... content/browser/speech/speech_recognition_manager_impl.cc:149: DCHECK(SessionExists(session_id)); On 2012/09/12 06:15:09, Satish wrote: > this is not new code, but is it possible that in the time control goes to the UI > thread and comes back the session had got deleted, either due to JS API aborting > the start call or something else? If it is a valid scenario lets check and do an > early return instead of a dcheck Done. https://chromiumcodereview.appspot.com/10933018/diff/1/content/browser/speech... content/browser/speech/speech_recognition_manager_impl.cc:602: : media_stream_manager_(media_stream_manager), On 2012/09/12 06:15:09, Satish wrote: > indent to right by 2 spaces This line got moved, and I think I got it right in the new place. https://chromiumcodereview.appspot.com/10933018/diff/1/content/browser/speech... content/browser/speech/speech_recognition_manager_impl.cc:611: media_stream_manager_->GenerateStream( On 2012/09/12 06:15:09, Satish wrote: > these method names don't seem to make sense when called for bringing up an > infobar for speech reco permission. Perhaps they need to be renamed to be more > generic? > > Hmm this actually feels like we are using a class in MediaStream that is a bit > higher than what we actually need.. i.e. a manager class that shows infobars at > the right time than the infobar class itself. Looking at the class declaration > that seems to be the case. Can just the infobar and permissions management code > be extracted out to a smaller class and that can be reused by speech > recognition? MediaStream does not have a convenient way of just accessing the infobar and permission stuff. It's tightly integrated with the process of opening a new stream. I think it would be hard to extract it and make it part of two different flows. Maybe the MediaStream people CC'd can comment on this. Though I admit that opening a stream just to check if we have permission is slightly overkill, I do think that it solves our problem quite nicely for now. https://chromiumcodereview.appspot.com/10933018/diff/1/content/browser/speech... content/browser/speech/speech_recognition_manager_impl.cc:628: On 2012/09/12 06:15:09, Satish wrote: > remove empty line Done. https://chromiumcodereview.appspot.com/10933018/diff/1/content/browser/speech... File content/browser/speech/speech_recognition_manager_impl.h (right): https://chromiumcodereview.appspot.com/10933018/diff/1/content/browser/speech... content/browser/speech/speech_recognition_manager_impl.h:130: bool is_waiting_for_permission; On 2012/09/12 06:15:09, Satish wrote: > instead of this flag in each Session object, how about storing the session_id in > the PermissionRequest class and check with the other session_id value when > necessary? That would reduce a few lines of code Done. https://chromiumcodereview.appspot.com/10933018/diff/1/content/browser/speech... content/browser/speech/speech_recognition_manager_impl.h:179: class PermissionRequest : public media_stream::MediaStreamRequester { On 2012/09/12 06:15:09, Satish wrote: > can this be forward declared in this header and the actual definition be moved > to the .cc file? No, because it inherits from another class it can't be forward declared. I did try, though :) https://chromiumcodereview.appspot.com/10933018/diff/1/content/browser/speech... content/browser/speech/speech_recognition_manager_impl.h:179: class PermissionRequest : public media_stream::MediaStreamRequester { On 2012/09/12 06:15:09, Satish wrote: > can this be forward declared in this header and the actual definition be moved > to the .cc file? Done. https://chromiumcodereview.appspot.com/10933018/diff/1/content/public/common/... File content/public/common/speech_recognition_error.h (right): https://chromiumcodereview.appspot.com/10933018/diff/1/content/public/common/... content/public/common/speech_recognition_error.h:20: SPEECH_RECOGNITION_ERROR_NOT_ALLOWED, On 2012/09/12 06:15:09, Satish wrote: > I don't have a strong preference here, but this could also be named as > SPEECH_RECOGNITION_ERROR_DENIED.. up to you to decide which is appropriate I'd like to go with NOT_ALLOWED for now, but I don't feel strongly about it. https://chromiumcodereview.appspot.com/10933018/diff/1/content/renderer/speec... File content/renderer/speech_recognition_dispatcher.cc (left): https://chromiumcodereview.appspot.com/10933018/diff/1/content/renderer/speec... content/renderer/speech_recognition_dispatcher.cc:62: // TODO(primiano): Should return false in order to communicate the failure On 2012/09/12 06:15:09, Satish wrote: > Is this being removed because a different change already handles it? Yes, it's handled in WebKit.
Nice work, it looks really good. https://chromiumcodereview.appspot.com/10933018/diff/10002/chrome/browser/spe... File chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc (right): https://chromiumcodereview.appspot.com/10933018/diff/10002/chrome/browser/spe... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:486: session_id, It looks to me that we don't use the session_id in CheckRenderViewType any more. If RequiresTrayIcon(session_id) needs to be called on IO thread, remove the session_id, otherwise we call RequiresTrayIcon in CheckRenderViewType() https://chromiumcodereview.appspot.com/10933018/diff/10002/content/browser/sp... File content/browser/speech/speech_recognition_manager_impl.cc (right): https://chromiumcodereview.appspot.com/10933018/diff/10002/content/browser/sp... content/browser/speech/speech_recognition_manager_impl.cc:54: class SpeechRecognitionManagerImpl::PermissionRequest : mvoe this ":" one line below with 4 space as indentation. https://chromiumcodereview.appspot.com/10933018/diff/10002/content/browser/sp... content/browser/speech/speech_recognition_manager_impl.cc:65: virtual ~PermissionRequest() {} When a request is being handled by the media_stream_manager (no StreamGenerated() or StreamGenerationFailed() is received yet), deleting this PermissionRequest can crash. We should protect this case by calling Abort() is it has not got callback from GenerateStream() https://chromiumcodereview.appspot.com/10933018/diff/10002/content/browser/sp... content/browser/speech/speech_recognition_manager_impl.cc:81: int Session() { return session_id_; } nit, int Session() const { ... } https://chromiumcodereview.appspot.com/10933018/diff/10002/content/browser/sp... content/browser/speech/speech_recognition_manager_impl.cc:137: SpeechRecognitionManagerImpl::~SpeechRecognitionManagerImpl() { who owns the SpeechRecognitionManagerImpl? please ensure that it should not outlive media_stream_manager_.
Thank you very much for the review! New patch uploaded. https://chromiumcodereview.appspot.com/10933018/diff/10002/chrome/browser/spe... File chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc (right): https://chromiumcodereview.appspot.com/10933018/diff/10002/chrome/browser/spe... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:486: session_id, On 2012/09/12 11:29:49, xians1 wrote: > It looks to me that we don't use the session_id in CheckRenderViewType any more. > If RequiresTrayIcon(session_id) needs to be called on IO thread, remove the > session_id, otherwise we call RequiresTrayIcon in CheckRenderViewType() Done. https://chromiumcodereview.appspot.com/10933018/diff/10002/content/browser/sp... File content/browser/speech/speech_recognition_manager_impl.cc (right): https://chromiumcodereview.appspot.com/10933018/diff/10002/content/browser/sp... content/browser/speech/speech_recognition_manager_impl.cc:54: class SpeechRecognitionManagerImpl::PermissionRequest : On 2012/09/12 11:29:49, xians1 wrote: > mvoe this ":" one line below with 4 space as indentation. Done. https://chromiumcodereview.appspot.com/10933018/diff/10002/content/browser/sp... content/browser/speech/speech_recognition_manager_impl.cc:65: virtual ~PermissionRequest() {} On 2012/09/12 11:29:49, xians1 wrote: > When a request is being handled by the media_stream_manager (no > StreamGenerated() or StreamGenerationFailed() is received yet), deleting this > PermissionRequest can crash. > > We should protect this case by calling Abort() is it has not got callback from > GenerateStream() Done. https://chromiumcodereview.appspot.com/10933018/diff/10002/content/browser/sp... content/browser/speech/speech_recognition_manager_impl.cc:81: int Session() { return session_id_; } On 2012/09/12 11:29:49, xians1 wrote: > nit, int Session() const { ... } Done. https://chromiumcodereview.appspot.com/10933018/diff/10002/content/browser/sp... content/browser/speech/speech_recognition_manager_impl.cc:137: SpeechRecognitionManagerImpl::~SpeechRecognitionManagerImpl() { On 2012/09/12 11:29:49, xians1 wrote: > who owns the SpeechRecognitionManagerImpl? please ensure that it should not > outlive media_stream_manager_. SpeechRecognitionManagerImpl is a singleton, and it gets leaked, I believe. When media_stream_manager_ dies, I believe the IO thread has already been shut down, so I'm not worried about any more requests using it.
https://chromiumcodereview.appspot.com/10933018/diff/1/content/browser/speech... File content/browser/speech/speech_recognition_manager_impl.cc (right): https://chromiumcodereview.appspot.com/10933018/diff/1/content/browser/speech... content/browser/speech/speech_recognition_manager_impl.cc:611: media_stream_manager_->GenerateStream( On 2012/09/12 09:46:58, hans wrote: > On 2012/09/12 06:15:09, Satish wrote: > > these method names don't seem to make sense when called for bringing up an > > infobar for speech reco permission. Perhaps they need to be renamed to be more > > generic? > > > > Hmm this actually feels like we are using a class in MediaStream that is a bit > > higher than what we actually need.. i.e. a manager class that shows infobars > at > > the right time than the infobar class itself. Looking at the class declaration > > that seems to be the case. Can just the infobar and permissions management > code > > be extracted out to a smaller class and that can be reused by speech > > recognition? > > MediaStream does not have a convenient way of just accessing the infobar and > permission stuff. It's tightly integrated with the process of opening a new > stream. I think it would be hard to extract it and make it part of two different > flows. Maybe the MediaStream people CC'd can comment on this. > > Though I admit that opening a stream just to check if we have permission is > slightly overkill, I do think that it solves our problem quite nicely for now. xians/tommyw, any thoughts on this? Can the infobar/permissions code in MediaStreamManager be factored out into a separate class that we can use?
On Wed, Sep 12, 2012 at 3:51 PM, <satish@chromium.org> wrote: > > https://chromiumcodereview.**appspot.com/10933018/diff/1/** > content/browser/speech/speech_**recognition_manager_impl.cc<https://chromiumcodereview.appspot.com/10933018/diff/1/content/browser/speech/speech_recognition_manager_impl.cc> > File content/browser/speech/speech_**recognition_manager_impl.cc (right): > > https://chromiumcodereview.**appspot.com/10933018/diff/1/** > content/browser/speech/speech_**recognition_manager_impl.cc#**newcode611<https://chromiumcodereview.appspot.com/10933018/diff/1/content/browser/speech/speech_recognition_manager_impl.cc#newcode611> > content/browser/speech/speech_**recognition_manager_impl.cc:**611: > media_stream_manager_->**GenerateStream( > On 2012/09/12 09:46:58, hans wrote: > >> On 2012/09/12 06:15:09, Satish wrote: >> > these method names don't seem to make sense when called for bringing >> > up an > >> > infobar for speech reco permission. Perhaps they need to be renamed >> > to be more > >> > generic? >> > >> > Hmm this actually feels like we are using a class in MediaStream >> > that is a bit > >> > higher than what we actually need.. i.e. a manager class that shows >> > infobars > >> at >> > the right time than the infobar class itself. Looking at the class >> > declaration > >> > that seems to be the case. Can just the infobar and permissions >> > management > >> code >> > be extracted out to a smaller class and that can be reused by speech >> > recognition? >> > > MediaStream does not have a convenient way of just accessing the >> > infobar and > >> permission stuff. It's tightly integrated with the process of opening >> > a new > >> stream. I think it would be hard to extract it and make it part of two >> > different > >> flows. Maybe the MediaStream people CC'd can comment on this. >> > > Though I admit that opening a stream just to check if we have >> > permission is > >> slightly overkill, I do think that it solves our problem quite nicely >> > for now. > > xians/tommyw, any thoughts on this? Can the infobar/permissions code in > MediaStreamManager be factored out into a separate class that we can > use? > > It is definitely a good idea to separate the infobar-permission code from media_stream_manager.cc. Basically, this new class is be a listener to device monitors to get the up-to-date device lists, when getting a new request, it triggers the infobar, and uses a callback to return what the users select. And we have two alternatives to use this new device_access class: #1, media_stream_manager.cc owns this new object. And we need to add one RequestDevicesAccess() API to media_stream_manager.cc to use this class. The benefits of doing this include: we can control only one infobar will be showed up in a tab when multiple apps are requesting the device accesses. a code base which can be extended to other UIs like system tray, omnibox. #2, different apps can create their own device_access objects, and these objects won't interference with each other. media_stream_manager.cc is one user case. This device_access class is easier to be used. Satish, which one do you think fits your need more? But anyhow, doing such a refactoring might take a while, I am estimating it to be at least 1 week's work (half week for coding, another half week for review). BR, -SX > https://chromiumcodereview.**appspot.com/10933018/<https://chromiumcodereview... >
Hans can probably tell more authoritatively, but I think #1 is more useful for us too.. if the page is calling getUserMedia() and while waiting for permission it also calls speech recognition start, it would be best to show just the first infobar (assuming both are for audio/mic access..) and once user grants permission both api calls return success.
https://chromiumcodereview.appspot.com/10933018/diff/4002/content/browser/spe... File content/browser/speech/speech_recognition_manager_impl.cc (right): https://chromiumcodereview.appspot.com/10933018/diff/4002/content/browser/spe... content/browser/speech/speech_recognition_manager_impl.cc:97: BrowserThread::PostTask( StreamGenerated is called on IO thread. Do you need this PostTask? Do you have plan to use other MediaStreamManager besides BrowserMainLoop::GetMediaStreamManager()?The |media_stream_manager_| has to be valid till this task is executed.
On 2012/09/12 22:58:25, wjia wrote: > content/browser/speech/speech_recognition_manager_impl.cc:97: > BrowserThread::PostTask( > StreamGenerated is called on IO thread. Do you need this PostTask? Yes, IIRC the MediaStreamManager crashed if I called back synchronously. > Do you have plan to use other MediaStreamManager besides > BrowserMainLoop::GetMediaStreamManager()?The |media_stream_manager_| has to be > valid till this task is executed. No, I'll just change it to use BrowserMainLoop::GetMediaStreamManager() > But anyhow, doing such a refactoring might take a while, I am estimating it to be at least 1 week's work (half week for coding, another half week for review). I agree that this refactoring would take a while. Given that we're hoping to enable this soon, I'd prefer to land this now. Satish?
On 2012/09/13 12:53:48, hans wrote: > On 2012/09/12 22:58:25, wjia wrote: > > content/browser/speech/speech_recognition_manager_impl.cc:97: > > BrowserThread::PostTask( > > StreamGenerated is called on IO thread. Do you need this PostTask? > > Yes, IIRC the MediaStreamManager crashed if I called back synchronously. > > > Do you have plan to use other MediaStreamManager besides > > BrowserMainLoop::GetMediaStreamManager()?The |media_stream_manager_| has to be > > valid till this task is executed. > > No, I'll just change it to use BrowserMainLoop::GetMediaStreamManager() > > > > > But anyhow, doing such a refactoring might take a while, I am estimating it > to be at least 1 week's work (half week for coding, another half week for > review). > > I agree that this refactoring would take a while. Given that we're hoping to > enable this soon, I'd prefer to land this now. Satish? Seems ok to do the refactor in a separate CL after this one.
On 2012/09/13 14:28:19, Satish wrote: > On 2012/09/13 12:53:48, hans wrote: > > On 2012/09/12 22:58:25, wjia wrote: > > > content/browser/speech/speech_recognition_manager_impl.cc:97: > > > BrowserThread::PostTask( > > > StreamGenerated is called on IO thread. Do you need this PostTask? > > > > Yes, IIRC the MediaStreamManager crashed if I called back synchronously. > > > > > Do you have plan to use other MediaStreamManager besides > > > BrowserMainLoop::GetMediaStreamManager()?The |media_stream_manager_| has to > be > > > valid till this task is executed. > > > > No, I'll just change it to use BrowserMainLoop::GetMediaStreamManager() > > > > > > > > > But anyhow, doing such a refactoring might take a while, I am estimating it > > to be at least 1 week's work (half week for coding, another half week for > > review). > > > > I agree that this refactoring would take a while. Given that we're hoping to > > enable this soon, I'd prefer to land this now. Satish? > > Seems ok to do the refactor in a separate CL after this one. Cool. xians, wjia: does the patch look good to you?
On 2012/09/13 14:33:36, hans wrote: > On 2012/09/13 14:28:19, Satish wrote: > > On 2012/09/13 12:53:48, hans wrote: > > > On 2012/09/12 22:58:25, wjia wrote: > > > > content/browser/speech/speech_recognition_manager_impl.cc:97: > > > > BrowserThread::PostTask( > > > > StreamGenerated is called on IO thread. Do you need this PostTask? > > > > > > Yes, IIRC the MediaStreamManager crashed if I called back synchronously. > > > > > > > Do you have plan to use other MediaStreamManager besides > > > > BrowserMainLoop::GetMediaStreamManager()?The |media_stream_manager_| has > to > > be > > > > valid till this task is executed. > > > > > > No, I'll just change it to use BrowserMainLoop::GetMediaStreamManager() > > > > > > > > > > > > > But anyhow, doing such a refactoring might take a while, I am estimating > it > > > to be at least 1 week's work (half week for coding, another half week for > > > review). > > > > > > I agree that this refactoring would take a while. Given that we're hoping to > > > enable this soon, I'd prefer to land this now. Satish? > > > > Seems ok to do the refactor in a separate CL after this one. > sgtm > Cool. > > xians, wjia: does the patch look good to you? lgtm with some nits, please address them.
https://chromiumcodereview.appspot.com/10933018/diff/1010/content/browser/spe... File content/browser/speech/speech_recognition_manager_impl.cc (right): https://chromiumcodereview.appspot.com/10933018/diff/1010/content/browser/spe... content/browser/speech/speech_recognition_manager_impl.cc:69: void Start(int render_process_id, int render_view_id, const GURL& origin) { add thread DCheck here. https://chromiumcodereview.appspot.com/10933018/diff/1010/content/browser/spe... content/browser/speech/speech_recognition_manager_impl.cc:80: void Abort() { ditto https://chromiumcodereview.appspot.com/10933018/diff/1010/content/browser/spe... content/browser/speech/speech_recognition_manager_impl.cc:94: started_ = false; ditto https://chromiumcodereview.appspot.com/10933018/diff/1010/content/browser/spe... content/browser/speech/speech_recognition_manager_impl.cc:105: started_ = false; ditto
Thank you very much! Nits fixed and new patch uploaded. https://chromiumcodereview.appspot.com/10933018/diff/1010/content/browser/spe... File content/browser/speech/speech_recognition_manager_impl.cc (right): https://chromiumcodereview.appspot.com/10933018/diff/1010/content/browser/spe... content/browser/speech/speech_recognition_manager_impl.cc:69: void Start(int render_process_id, int render_view_id, const GURL& origin) { On 2012/09/13 15:11:02, xians1 wrote: > add thread DCheck here. Done. https://chromiumcodereview.appspot.com/10933018/diff/1010/content/browser/spe... content/browser/speech/speech_recognition_manager_impl.cc:80: void Abort() { On 2012/09/13 15:11:02, xians1 wrote: > ditto Done. https://chromiumcodereview.appspot.com/10933018/diff/1010/content/browser/spe... content/browser/speech/speech_recognition_manager_impl.cc:94: started_ = false; On 2012/09/13 15:11:02, xians1 wrote: > ditto Done. https://chromiumcodereview.appspot.com/10933018/diff/1010/content/browser/spe... content/browser/speech/speech_recognition_manager_impl.cc:105: started_ = false; On 2012/09/13 15:11:02, xians1 wrote: > ditto Done.
piman: can you do OWNER review for content/public, content/public/browser, and content/renderer
lgtm
Thanks! LGTM.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hans@chromium.org/10933018/1011
Change committed as 156615 |
