Make SpeechRecognition per RenderFrame instead of per RenderView.

Make SpeechRecognition per RenderFrame instead of per RenderView.

This is updating the Chromium code. It is part of a multi sided patch list.

Part 1: https://codereview.chromium.org/636863002
Part 2: <this>
Part 3: https://codereview.chromium.org/752303003/

BUG=390749

[mlamouri (slow - plz ping), 2014-11-10 20:09:39]

mlamouri@chromium.org changed reviewers:
+ creis@chromium.org

[mlamouri (slow - plz ping), 2014-11-10 20:09:39]

mlamouri@chromium.org changed required reviewers:
+ creis@chromium.org

[mlamouri (slow - plz ping), 2014-11-10 20:09:40]

I'm not a very big fan of using RenderFrameHost* instead of render_frame_id and
render_process_id in the Context object but most of the speech code runs in the
IO thread and getting the RenderFrameHost from the two ids must run in the UI
thread. Using those would make things terribly more harder to read. Hopefully,
the handling of ::RenderFrameDeleted() solves that issue.

[Charlie Reis, 2014-11-11 04:37:31]

Hmm, I started reviewing this but it's accessing UI thread objects (WebContents,
RFH) on the IO thread.  We'll need to avoid that, perhaps by sticking with IDs. 
More info here:

http://www.chromium.org/developers/design-documents/threading

https://codereview.chromium.org/636863003/diff/60001/chrome/browser/speech/ch...
File chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc
(right):

https://codereview.chromium.org/636863003/diff/60001/chrome/browser/speech/ch...
chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:64:
web_contents->ForEachFrame(
This is illegal.  We can't use a WebContents on the IO thread.

https://codereview.chromium.org/636863003/diff/60001/content/browser/speech/s...
File content/browser/speech/speech_recognition_dispatcher_host.cc (right):

https://codereview.chromium.org/636863003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognition_dispatcher_host.cc:221:
context.render_frame_host->GetRoutingID(), context.request_id));
This appears to be called on the IO thread, which is also illegal.

https://codereview.chromium.org/636863003/diff/60001/content/public/browser/s...
File content/public/browser/speech_recognition_session_context.h (right):

https://codereview.chromium.org/636863003/diff/60001/content/public/browser/s...
content/public/browser/speech_recognition_session_context.h:31: // really
inconvenient. Instead, SpeechRecognitionDispatcherHost informs the
This is unsafe.  RenderFrameHost itself cannot be accessed outside the UI
thread.

[mlamouri (slow - plz ping), 2014-11-28 12:06:00]

mlamouri@chromium.org changed required reviewers:
- creis@chromium.org

[mlamouri (slow - plz ping), 2014-11-28 14:31:49]

Thanks for the review Charlie. I was actually not aware that WebContents and
RenderFrameHost are not meant to be used outside of the UI thread. I guess
that's why ::From* methods must be used from the UI thread but somehow, it's not
really explicit ;)

Anyway, I believe the threading issues should be fixed. PTAL.

[Charlie Reis, 2014-12-01 23:49:43]

Thanks, looking better.  One question about WebContentsObserver vs
BrowserMessageFilter below, plus some nits.

https://codereview.chromium.org/636863003/diff/80001/chrome/browser/speech/ch...
File chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc (left):

https://codereview.chromium.org/636863003/diff/80001/chrome/browser/speech/ch...
chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:362:
DCHECK_NE(context.render_process_id, 0);
I see that the rest of this block has been moved inside CheckRenderViewType. 
Can we keep this DCHECK as well, or is there a reason to remove it?

https://codereview.chromium.org/636863003/diff/80001/chrome/browser/speech/ch...
File chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc
(right):

https://codereview.chromium.org/636863003/diff/80001/chrome/browser/speech/ch...
chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:54: void
AbortSpeechSessionsForFrameIO(int render_process_id, int render_frame_id) {
nit: OnIO

https://codereview.chromium.org/636863003/diff/80001/chrome/browser/speech/ch...
chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:236: //
process id and render view id to |tab_closed_callback_| after the process
This comment looks stale.

https://codereview.chromium.org/636863003/diff/80001/chrome/browser/speech/ch...
chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:240: //
Callback used to notify, on the thread specified by |callback_thread_| the
callback_thread_ looks stale.  This gets called on the UI thread, right?

https://codereview.chromium.org/636863003/diff/80001/chrome/browser/speech/ch...
File chrome/browser/speech/chrome_speech_recognition_manager_delegate.h (right):

https://codereview.chromium.org/636863003/diff/80001/chrome/browser/speech/ch...
chrome/browser/speech/chrome_speech_recognition_manager_delegate.h:57: //
Callback called by |tab_watcher_| on the IO thread to signal tab closure.
This is called on the UI thread and not the IO thread, correct?  (Otherwise we
couldn't pass WebContents to it.)

https://codereview.chromium.org/636863003/diff/80001/content/browser/speech/s...
File content/browser/speech/speech_recognition_dispatcher_host.cc (right):

https://codereview.chromium.org/636863003/diff/80001/content/browser/speech/s...
content/browser/speech/speech_recognition_dispatcher_host.cc:27: void
SendMessageToFrame(int render_process_id,
Is this called from another thread?  All the callers seem pretty uniform, but I
can't tell if they're UI or IO.

https://codereview.chromium.org/636863003/diff/80001/content/browser/speech/s...
content/browser/speech/speech_recognition_dispatcher_host.cc:39:
RenderFrameHost::FromID(render_process_id, render_frame_id)->Send(message);
We should be careful about FromID returning NULL, in case the tab was closed
while the task was being posted from the IO thread.

https://codereview.chromium.org/636863003/diff/80001/content/browser/speech/s...
File content/browser/speech/speech_recognition_dispatcher_host.h (right):

https://codereview.chromium.org/636863003/diff/80001/content/browser/speech/s...
content/browser/speech/speech_recognition_dispatcher_host.h:34: : public
WebContentsObserver,
What benefits are we getting from making this a WebContentsObserver rather than
a BrowserMessageFilter?

I ask because it feels a bit awkward for IPC messages to arrive on the IO
thread, get posted to the UI thread (by ChannelProxy) for routing (via
WebContentsObserver), then get posted back to the IO thread here to do work.

Would it be less complex to leave it as a BrowserMessageFilter and always pass
the RenderFrame's routing ID instead, similar to how it used to work with
RenderViews?  Or am I missing something?

https://codereview.chromium.org/636863003/diff/80001/content/browser/speech/s...
content/browser/speech/speech_recognition_dispatcher_host.h:73: void
OnAbortRequestIO(int render_process_id,
nit: AbortRequestOnIO (similar below)

[mlamouri (slow - plz ping), 2014-12-08 17:41:04]

PTAL

https://codereview.chromium.org/636863003/diff/80001/chrome/browser/speech/ch...
File chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc (left):

https://codereview.chromium.org/636863003/diff/80001/chrome/browser/speech/ch...
chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:362:
DCHECK_NE(context.render_process_id, 0);
On 2014/12/01 23:49:42, Charlie Reis wrote:
> I see that the rest of this block has been moved inside CheckRenderViewType. 
> Can we keep this DCHECK as well, or is there a reason to remove it?

I removed that because the check sounded pretty arbitrary. It's not clear to me
how we might end up in this situation and why checking render_process_id but not
render_view_id/render_frame_id.

I'm happy to put it back though.

https://codereview.chromium.org/636863003/diff/80001/chrome/browser/speech/ch...
File chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc
(right):

https://codereview.chromium.org/636863003/diff/80001/chrome/browser/speech/ch...
chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:54: void
AbortSpeechSessionsForFrameIO(int render_process_id, int render_frame_id) {
On 2014/12/01 23:49:43, Charlie Reis wrote:
> nit: OnIO

Done.

https://codereview.chromium.org/636863003/diff/80001/chrome/browser/speech/ch...
chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:236: //
process id and render view id to |tab_closed_callback_| after the process
On 2014/12/01 23:49:42, Charlie Reis wrote:
> This comment looks stale.

Done.

https://codereview.chromium.org/636863003/diff/80001/chrome/browser/speech/ch...
chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:240: //
Callback used to notify, on the thread specified by |callback_thread_| the
On 2014/12/01 23:49:42, Charlie Reis wrote:
> callback_thread_ looks stale.  This gets called on the UI thread, right?

Done.

https://codereview.chromium.org/636863003/diff/80001/chrome/browser/speech/ch...
File chrome/browser/speech/chrome_speech_recognition_manager_delegate.h (right):

https://codereview.chromium.org/636863003/diff/80001/chrome/browser/speech/ch...
chrome/browser/speech/chrome_speech_recognition_manager_delegate.h:57: //
Callback called by |tab_watcher_| on the IO thread to signal tab closure.
On 2014/12/01 23:49:43, Charlie Reis wrote:
> This is called on the UI thread and not the IO thread, correct?  (Otherwise we
> couldn't pass WebContents to it.)

Correct. Fixed.

https://codereview.chromium.org/636863003/diff/80001/content/browser/speech/s...
File content/browser/speech/speech_recognition_dispatcher_host.cc (right):

https://codereview.chromium.org/636863003/diff/80001/content/browser/speech/s...
content/browser/speech/speech_recognition_dispatcher_host.cc:27: void
SendMessageToFrame(int render_process_id,
On 2014/12/01 23:49:43, Charlie Reis wrote:
> Is this called from another thread?  All the callers seem pretty uniform, but
I
> can't tell if they're UI or IO.

Callers are on the IO thread. The method is thread agnostic just so I don't need
two methods (it calls itself in another thread). Would you prefer something more
explicit?

https://codereview.chromium.org/636863003/diff/80001/content/browser/speech/s...
content/browser/speech/speech_recognition_dispatcher_host.cc:39:
RenderFrameHost::FromID(render_process_id, render_frame_id)->Send(message);
On 2014/12/01 23:49:43, Charlie Reis wrote:
> We should be careful about FromID returning NULL, in case the tab was closed
> while the task was being posted from the IO thread.

Done.

https://codereview.chromium.org/636863003/diff/80001/content/browser/speech/s...
File content/browser/speech/speech_recognition_dispatcher_host.h (right):

https://codereview.chromium.org/636863003/diff/80001/content/browser/speech/s...
content/browser/speech/speech_recognition_dispatcher_host.h:34: : public
WebContentsObserver,
On 2014/12/01 23:49:43, Charlie Reis wrote:
> What benefits are we getting from making this a WebContentsObserver rather
than
> a BrowserMessageFilter?
> 
> I ask because it feels a bit awkward for IPC messages to arrive on the IO
> thread, get posted to the UI thread (by ChannelProxy) for routing (via
> WebContentsObserver), then get posted back to the IO thread here to do work.
> 
> Would it be less complex to leave it as a BrowserMessageFilter and always pass
> the RenderFrame's routing ID instead, similar to how it used to work with
> RenderViews?  Or am I missing something?

The only real benefit from using WebContentsObserver is |RenderFrameDeleted|
which allows us to abort the requests started by a frame after it is deleted. We
might be able to have the same result by other means.

WDYT?

[Charlie Reis, 2014-12-08 20:02:57]

Thanks!  See my comment about BrowserMessageFilter vs WCO below.

Minor nit: it helps if you upload the rebase in a separate patch set from the
actual changes, so that reviewers can easily look at the diffs between relevant
patch sets.

https://codereview.chromium.org/636863003/diff/80001/chrome/browser/speech/ch...
File chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc (left):

https://codereview.chromium.org/636863003/diff/80001/chrome/browser/speech/ch...
chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:362:
DCHECK_NE(context.render_process_id, 0);
On 2014/12/08 17:41:04, Mounir Lamouri wrote:
> On 2014/12/01 23:49:42, Charlie Reis wrote:
> > I see that the rest of this block has been moved inside CheckRenderViewType.

> > Can we keep this DCHECK as well, or is there a reason to remove it?
> 
> I removed that because the check sounded pretty arbitrary. It's not clear to
me
> how we might end up in this situation and why checking render_process_id but
not
> render_view_id/render_frame_id.
> 
> I'm happy to put it back though.

Looks like it's initialized to 0 in the context's constructor, so it's not
entirely arbitrary; let's keep it.  Adding a matching DCHECK for render_frame_id
is probably reasonable.

https://codereview.chromium.org/636863003/diff/80001/content/browser/speech/s...
File content/browser/speech/speech_recognition_dispatcher_host.cc (right):

https://codereview.chromium.org/636863003/diff/80001/content/browser/speech/s...
content/browser/speech/speech_recognition_dispatcher_host.cc:27: void
SendMessageToFrame(int render_process_id,
On 2014/12/08 17:41:04, Mounir Lamouri wrote:
> On 2014/12/01 23:49:43, Charlie Reis wrote:
> > Is this called from another thread?  All the callers seem pretty uniform,
but
> I
> > can't tell if they're UI or IO.
> 
> Callers are on the IO thread. The method is thread agnostic just so I don't
need
> two methods (it calls itself in another thread). Would you prefer something
more
> explicit?

Yeah, let's split it into two methods.  That makes things easier to reason
about: it's easy to tell that we expect this to be called from the IO thread,
with a need to hop to the UI thread.

https://codereview.chromium.org/636863003/diff/80001/content/browser/speech/s...
File content/browser/speech/speech_recognition_dispatcher_host.h (right):

https://codereview.chromium.org/636863003/diff/80001/content/browser/speech/s...
content/browser/speech/speech_recognition_dispatcher_host.h:34: : public
WebContentsObserver,
On 2014/12/08 17:41:04, Mounir Lamouri wrote:
> On 2014/12/01 23:49:43, Charlie Reis wrote:
> > What benefits are we getting from making this a WebContentsObserver rather
> than
> > a BrowserMessageFilter?
> > 
> > I ask because it feels a bit awkward for IPC messages to arrive on the IO
> > thread, get posted to the UI thread (by ChannelProxy) for routing (via
> > WebContentsObserver), then get posted back to the IO thread here to do work.
> > 
> > Would it be less complex to leave it as a BrowserMessageFilter and always
pass
> > the RenderFrame's routing ID instead, similar to how it used to work with
> > RenderViews?  Or am I missing something?
> 
> The only real benefit from using WebContentsObserver is |RenderFrameDeleted|
> which allows us to abort the requests started by a frame after it is deleted.
We
> might be able to have the same result by other means.
> 
> WDYT?

Ah, I see.

Yes, I think we can handle that more cleanly-- we can have a separate
WebContentsObserver class (on the UI thread) whose RenderFrameDeleted method
posts a task to call OnAbortAllRequests on the IO thread.  Then the rest of this
class can stay on the IO thread as a BrowserMessageFilter, which avoids the IO
-> UI -> IO overhead for the majority of its behavior.

[mlamouri (slow - plz ping), 2014-12-08 21:08:01]

All comments applied. PTAL.

https://codereview.chromium.org/636863003/diff/80001/chrome/browser/speech/ch...
File chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc (left):

https://codereview.chromium.org/636863003/diff/80001/chrome/browser/speech/ch...
chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:362:
DCHECK_NE(context.render_process_id, 0);
On 2014/12/08 20:02:57, Charlie Reis wrote:
> On 2014/12/08 17:41:04, Mounir Lamouri wrote:
> > On 2014/12/01 23:49:42, Charlie Reis wrote:
> > > I see that the rest of this block has been moved inside
CheckRenderViewType.
> 
> > > Can we keep this DCHECK as well, or is there a reason to remove it?
> > 
> > I removed that because the check sounded pretty arbitrary. It's not clear to
> me
> > how we might end up in this situation and why checking render_process_id but
> not
> > render_view_id/render_frame_id.
> > 
> > I'm happy to put it back though.
> 
> Looks like it's initialized to 0 in the context's constructor, so it's not
> entirely arbitrary; let's keep it.  Adding a matching DCHECK for
render_frame_id
> is probably reasonable.

Done.

https://codereview.chromium.org/636863003/diff/80001/content/browser/speech/s...
File content/browser/speech/speech_recognition_dispatcher_host.cc (right):

https://codereview.chromium.org/636863003/diff/80001/content/browser/speech/s...
content/browser/speech/speech_recognition_dispatcher_host.cc:27: void
SendMessageToFrame(int render_process_id,
On 2014/12/08 20:02:57, Charlie Reis wrote:
> On 2014/12/08 17:41:04, Mounir Lamouri wrote:
> > On 2014/12/01 23:49:43, Charlie Reis wrote:
> > > Is this called from another thread?  All the callers seem pretty uniform,
> but
> > I
> > > can't tell if they're UI or IO.
> > 
> > Callers are on the IO thread. The method is thread agnostic just so I don't
> need
> > two methods (it calls itself in another thread). Would you prefer something
> more
> > explicit?
> 
> Yeah, let's split it into two methods.  That makes things easier to reason
> about: it's easy to tell that we expect this to be called from the IO thread,
> with a need to hop to the UI thread.

Done.

https://codereview.chromium.org/636863003/diff/80001/content/browser/speech/s...
File content/browser/speech/speech_recognition_dispatcher_host.h (right):

https://codereview.chromium.org/636863003/diff/80001/content/browser/speech/s...
content/browser/speech/speech_recognition_dispatcher_host.h:34: : public
WebContentsObserver,
On 2014/12/08 20:02:57, Charlie Reis wrote:
> On 2014/12/08 17:41:04, Mounir Lamouri wrote:
> > On 2014/12/01 23:49:43, Charlie Reis wrote:
> > > What benefits are we getting from making this a WebContentsObserver rather
> > than
> > > a BrowserMessageFilter?
> > > 
> > > I ask because it feels a bit awkward for IPC messages to arrive on the IO
> > > thread, get posted to the UI thread (by ChannelProxy) for routing (via
> > > WebContentsObserver), then get posted back to the IO thread here to do
work.
> > > 
> > > Would it be less complex to leave it as a BrowserMessageFilter and always
> pass
> > > the RenderFrame's routing ID instead, similar to how it used to work with
> > > RenderViews?  Or am I missing something?
> > 
> > The only real benefit from using WebContentsObserver is |RenderFrameDeleted|
> > which allows us to abort the requests started by a frame after it is
deleted.
> We
> > might be able to have the same result by other means.
> > 
> > WDYT?
> 
> Ah, I see.
> 
> Yes, I think we can handle that more cleanly-- we can have a separate
> WebContentsObserver class (on the UI thread) whose RenderFrameDeleted method
> posts a task to call OnAbortAllRequests on the IO thread.  Then the rest of
this
> class can stay on the IO thread as a BrowserMessageFilter, which avoids the IO
> -> UI -> IO overhead for the majority of its behavior.
> 

Done. I haven't done anything with RenderFrameDeleted actually because
ChromeSpeechRecognitionManagerDelegate already closes the sessions when a tab is
closed and closing a session as soon as the frame is closed isn't something that
was done already so I guess it doesn't have to be done in this CL.

[Charlie Reis, 2014-12-09 00:52:34]

Thanks, that looks better.  A few questions below, but I think it's close.

https://codereview.chromium.org/636863003/diff/80001/content/browser/speech/s...
File content/browser/speech/speech_recognition_dispatcher_host.h (right):

https://codereview.chromium.org/636863003/diff/80001/content/browser/speech/s...
content/browser/speech/speech_recognition_dispatcher_host.h:34: : public
WebContentsObserver,
On 2014/12/08 21:08:00, Mounir Lamouri wrote:
> On 2014/12/08 20:02:57, Charlie Reis wrote:
> > On 2014/12/08 17:41:04, Mounir Lamouri wrote:
> > > On 2014/12/01 23:49:43, Charlie Reis wrote:
> > > > What benefits are we getting from making this a WebContentsObserver
rather
> > > than
> > > > a BrowserMessageFilter?
> > > > 
> > > > I ask because it feels a bit awkward for IPC messages to arrive on the
IO
> > > > thread, get posted to the UI thread (by ChannelProxy) for routing (via
> > > > WebContentsObserver), then get posted back to the IO thread here to do
> work.
> > > > 
> > > > Would it be less complex to leave it as a BrowserMessageFilter and
always
> > pass
> > > > the RenderFrame's routing ID instead, similar to how it used to work
with
> > > > RenderViews?  Or am I missing something?
> > > 
> > > The only real benefit from using WebContentsObserver is
|RenderFrameDeleted|
> > > which allows us to abort the requests started by a frame after it is
> deleted.
> > We
> > > might be able to have the same result by other means.
> > > 
> > > WDYT?
> > 
> > Ah, I see.
> > 
> > Yes, I think we can handle that more cleanly-- we can have a separate
> > WebContentsObserver class (on the UI thread) whose RenderFrameDeleted method
> > posts a task to call OnAbortAllRequests on the IO thread.  Then the rest of
> this
> > class can stay on the IO thread as a BrowserMessageFilter, which avoids the
IO
> > -> UI -> IO overhead for the majority of its behavior.
> > 
> 
> Done. I haven't done anything with RenderFrameDeleted actually because
> ChromeSpeechRecognitionManagerDelegate already closes the sessions when a tab
is
> closed and closing a session as soon as the frame is closed isn't something
that
> was done already so I guess it doesn't have to be done in this CL.

Is this a bug, though?  We're now tracking things per-frame, and a RenderFrame
might be deleted if we navigate cross-process (rather than closing the tab). 
What does it mean to leave the session around in that case?

https://codereview.chromium.org/636863003/diff/120001/chrome/browser/speech/c...
File chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc (left):

https://codereview.chromium.org/636863003/diff/120001/chrome/browser/speech/c...
chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:360: // Make
sure that initiators (extensions/web pages) properly set the
Let's preserve this comment, since it gives the motivation.

https://codereview.chromium.org/636863003/diff/120001/content/browser/speech/...
File content/browser/speech/speech_recognition_dispatcher_host.cc (right):

https://codereview.chromium.org/636863003/diff/120001/content/browser/speech/...
content/browser/speech/speech_recognition_dispatcher_host.cc:197:
render_process_id_, render_frame_id, request_id);
Here's another reason to keep the order consistent: we're passing
render_frame_id before request_id.

https://codereview.chromium.org/636863003/diff/120001/content/browser/speech/...
File content/browser/speech/speech_recognition_dispatcher_host.h (right):

https://codereview.chromium.org/636863003/diff/120001/content/browser/speech/...
content/browser/speech/speech_recognition_dispatcher_host.h:76: void
OnAbortRequest(int request_id, int render_frame_id);
Why reverse these parameters?  I would normally expect the frame's routing ID to
come before the request ID, since request IDs are specific to a frame.  (Or did
that change?)

Also, since they're both ints, it seems like swapping the order would make it to
end up with bugs where they're passed in the wrong order, without compiler help
for detecting it.

Same for OnStopCaptureRequest.

https://codereview.chromium.org/636863003/diff/120001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/636863003/diff/120001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:30: #include
"content/public/browser/browser_thread.h"
Unnecessary now?

https://codereview.chromium.org/636863003/diff/120001/extensions/shell/browse...
File extensions/shell/browser/shell_speech_recognition_manager_delegate.cc
(right):

https://codereview.chromium.org/636863003/diff/120001/extensions/shell/browse...
extensions/shell/browser/shell_speech_recognition_manager_delegate.cc:115: const
content::RenderViewHost* render_view_host =
We don't need |render_view_host| if we use WebContents::FromRenderFrameHost
below, right?  Same for the #include.

[mlamouri (slow - plz ping), 2015-01-19 11:20:23]

Review comments applied. I've also started something with regards to aborting
sessions when a RFH is killed. There might be some more work needed there with
threading but I will have to have a look next time I allocate time for this 20%.

https://codereview.chromium.org/636863003/diff/120001/chrome/browser/speech/c...
File chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc (left):

https://codereview.chromium.org/636863003/diff/120001/chrome/browser/speech/c...
chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:360: // Make
sure that initiators (extensions/web pages) properly set the
On 2014/12/09 at 00:52:33, Charlie Reis wrote:
> Let's preserve this comment, since it gives the motivation.

Done. Moved where the check happens.

https://codereview.chromium.org/636863003/diff/120001/content/browser/speech/...
File content/browser/speech/speech_recognition_dispatcher_host.h (right):

https://codereview.chromium.org/636863003/diff/120001/content/browser/speech/...
content/browser/speech/speech_recognition_dispatcher_host.h:76: void
OnAbortRequest(int request_id, int render_frame_id);
On 2014/12/09 at 00:52:33, Charlie Reis wrote:
> Why reverse these parameters?  I would normally expect the frame's routing ID
to come before the request ID, since request IDs are specific to a frame.  (Or
did that change?)
> 
> Also, since they're both ints, it seems like swapping the order would make it
to end up with bugs where they're passed in the wrong order, without compiler
help for detecting it.
> 
> Same for OnStopCaptureRequest.

Done.

https://codereview.chromium.org/636863003/diff/120001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/636863003/diff/120001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:30: #include
"content/public/browser/browser_thread.h"
On 2014/12/09 at 00:52:33, Charlie Reis wrote:
> Unnecessary now?

Removed.

https://codereview.chromium.org/636863003/diff/120001/extensions/shell/browse...
File extensions/shell/browser/shell_speech_recognition_manager_delegate.cc
(right):

https://codereview.chromium.org/636863003/diff/120001/extensions/shell/browse...
extensions/shell/browser/shell_speech_recognition_manager_delegate.cc:115: const
content::RenderViewHost* render_view_host =
On 2014/12/09 at 00:52:33, Charlie Reis wrote:
> We don't need |render_view_host| if we use WebContents::FromRenderFrameHost
below, right?  Same for the #include.

Right ;)