Speech Recognition API: Safeguarding against page closes during startup

It can happen that when a page with an Speech Recognition enabled input goes away directly after pressing the speech icon that the bubble is using a stale pointer; so instead of cashing a WebContents pointer I look it up every time instead.

BUG=330660
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252269

[Tommy Widenflycht, 2014-02-10 18:18:32]

Reviewers, can you take an initial look? 
I need to make the same changes to OS X and Linux before committing.

[Primiano Tucci (use gerrit), 2014-02-10 21:43:36]

LGTM % 1 nit.

P.S. Also, the commit message format should be 1 line + newline + description.
But I guess you were going to refine it later.

Just one note, I tried to repro the bug in the latest canary (Version
34.0.1832.1 canary SyzyASan) on Win 7 x64 and couldn't manage to reproduce.

https://codereview.chromium.org/148343008/diff/1/chrome/browser/ui/views/spee...
File chrome/browser/ui/views/speech_recognition_bubble_views.cc (right):

https://codereview.chromium.org/148343008/diff/1/chrome/browser/ui/views/spee...
chrome/browser/ui/views/speech_recognition_bubble_views.cc:387: if (bubble_ &&
web_contents)
nit: You could fit all in one line, without the web_contents variable. E.g.,
if (bubble_ && GetWebContents())

[tommi (sloooow) - chröme, 2014-02-10 22:32:53]

lgtm

https://codereview.chromium.org/148343008/diff/1/chrome/browser/speech/speech...
File chrome/browser/speech/speech_recognition_bubble.cc (right):

https://codereview.chromium.org/148343008/diff/1/chrome/browser/speech/speech...
chrome/browser/speech/speech_recognition_bubble.cc:152: DCHECK(web_contents);
nit: since you check web_contents immediately after, this DCHECK isn't
necessary.
DCHECKs in Chrome state assumptions that are otherwise not checked which may be
slightly different from blink.

https://codereview.chromium.org/148343008/diff/1/chrome/browser/ui/views/spee...
File chrome/browser/ui/views/speech_recognition_bubble_views.cc (right):

https://codereview.chromium.org/148343008/diff/1/chrome/browser/ui/views/spee...
chrome/browser/ui/views/speech_recognition_bubble_views.cc:387: if (bubble_ &&
web_contents)
On 2014/02/10 21:43:37, Primiano Tucci wrote:
> nit: You could fit all in one line, without the web_contents variable. E.g.,
> if (bubble_ && GetWebContents())

+1 that also avoids calling GetWebContents if bubble_ == NULL ;-)

[Tommy Widenflycht, 2014-02-12 11:35:42]

https://codereview.chromium.org/148343008/diff/1/chrome/browser/ui/views/spee...
File chrome/browser/ui/views/speech_recognition_bubble_views.cc (right):

https://codereview.chromium.org/148343008/diff/1/chrome/browser/ui/views/spee...
chrome/browser/ui/views/speech_recognition_bubble_views.cc:387: if (bubble_ &&
web_contents)
On 2014/02/10 21:43:37, Primiano Tucci wrote:
> nit: You could fit all in one line, without the web_contents variable. E.g.,
> if (bubble_ && GetWebContents())

Done.

https://codereview.chromium.org/148343008/diff/1/chrome/browser/ui/views/spee...
chrome/browser/ui/views/speech_recognition_bubble_views.cc:387: if (bubble_ &&
web_contents)
On 2014/02/10 22:32:53, tommi wrote:
> On 2014/02/10 21:43:37, Primiano Tucci wrote:
> > nit: You could fit all in one line, without the web_contents variable. E.g.,
> > if (bubble_ && GetWebContents())
> 
> +1 that also avoids calling GetWebContents if bubble_ == NULL ;-)

Done.

[Tommy Widenflycht, 2014-02-12 15:36:36]

sky: Can you have a look at the chrome/browser/ui files, please?

[sky, 2014-02-12 18:12:18]

https://codereview.chromium.org/148343008/diff/120001/chrome/browser/speech/s...
File chrome/browser/speech/speech_recognition_bubble.h (right):

https://codereview.chromium.org/148343008/diff/120001/chrome/browser/speech/s...
chrome/browser/speech/speech_recognition_bubble.h:64: // This should ONLY be
called from for tests
Isn't there a way to make the tests go through the same Create? I don't like
double the functions like this.

[Tommy Widenflycht, 2014-02-13 12:40:46]

On 2014/02/12 18:12:18, sky wrote:
>
https://codereview.chromium.org/148343008/diff/120001/chrome/browser/speech/s...
> File chrome/browser/speech/speech_recognition_bubble.h (right):
> 
>
https://codereview.chromium.org/148343008/diff/120001/chrome/browser/speech/s...
> chrome/browser/speech/speech_recognition_bubble.h:64: // This should ONLY be
> called from for tests
> Isn't there a way to make the tests go through the same Create? I don't like
> double the functions like this.

I don't like it more than you but unfortunately the browser_test environment
only gives out WebContent pointers through the tab_strip as far as I could find
out.

And FYI, this code will be deleted in a release or two (just marked it as
deprecated).

[sky, 2014-02-13 17:59:11]

If you have a WebContents can't you get the ids from it?

  -Scott

On Thu, Feb 13, 2014 at 4:40 AM,  <tommyw@chromium.org> wrote:
> On 2014/02/12 18:12:18, sky wrote:
>
>
https://codereview.chromium.org/148343008/diff/120001/chrome/browser/speech/s...
>>
>> File chrome/browser/speech/speech_recognition_bubble.h (right):
>
>
>
>
https://codereview.chromium.org/148343008/diff/120001/chrome/browser/speech/s...
>>
>> chrome/browser/speech/speech_recognition_bubble.h:64: // This should ONLY
>> be
>> called from for tests
>> Isn't there a way to make the tests go through the same Create? I don't
>> like
>> double the functions like this.
>
>
> I don't like it more than you but unfortunately the browser_test environment
> only gives out WebContent pointers through the tab_strip as far as I could
> find
> out.
>
> And FYI, this code will be deleted in a release or two (just marked it as
> deprecated).
>
> https://codereview.chromium.org/148343008/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Tommy Widenflycht, 2014-02-14 12:51:21]

On 2014/02/13 17:59:11, sky wrote:
> If you have a WebContents can't you get the ids from it?
> 
>   -Scott
>

Turns out I could :)

[sky, 2014-02-14 16:37:56]

I think these comments apply to all your implementations.

https://codereview.chromium.org/148343008/diff/280001/chrome/browser/ui/views...
File chrome/browser/ui/views/speech_recognition_bubble_views.cc (right):

https://codereview.chromium.org/148343008/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/speech_recognition_bubble_views.cc:386: if (bubble_ &&
GetWebContents())
Why is Hide gated by a WebContents? Doesn't this mean if the webcontents went
away the bubble would be stuck?

https://codereview.chromium.org/148343008/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/speech_recognition_bubble_views.cc:403: int
render_process_id, int render_view_id,
nit: when you wrap, one param per line.

[Tommy Widenflycht, 2014-02-17 09:46:33]

https://codereview.chromium.org/148343008/diff/280001/chrome/browser/ui/views...
File chrome/browser/ui/views/speech_recognition_bubble_views.cc (right):

https://codereview.chromium.org/148343008/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/speech_recognition_bubble_views.cc:386: if (bubble_ &&
GetWebContents())
On 2014/02/14 16:37:56, sky wrote:
> Why is Hide gated by a WebContents? Doesn't this mean if the webcontents went
> away the bubble would be stuck?

Maybe that was a bit of overkill but the bubble is cleaned away in the
destructor anyway. Removed gate.

https://codereview.chromium.org/148343008/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/speech_recognition_bubble_views.cc:403: int
render_process_id, int render_view_id,
On 2014/02/14 16:37:56, sky wrote:
> nit: when you wrap, one param per line.

Done.

[sky, 2014-02-18 17:29:44]

https://codereview.chromium.org/148343008/diff/400001/chrome/browser/ui/views...
File chrome/browser/ui/views/speech_recognition_bubble_views.cc (right):

https://codereview.chromium.org/148343008/diff/400001/chrome/browser/ui/views...
chrome/browser/ui/views/speech_recognition_bubble_views.cc:391: if (bubble_ &&
GetWebContents())
How come the other implementations have an || and this has a && ?

[Tommy Widenflycht, 2014-02-19 08:50:25]

https://codereview.chromium.org/148343008/diff/400001/chrome/browser/ui/views...
File chrome/browser/ui/views/speech_recognition_bubble_views.cc (right):

https://codereview.chromium.org/148343008/diff/400001/chrome/browser/ui/views...
chrome/browser/ui/views/speech_recognition_bubble_views.cc:391: if (bubble_ &&
GetWebContents())
On 2014/02/18 17:29:45, sky wrote:
> How come the other implementations have an || and this has a && ?

Because they do an early exit.

[sky, 2014-02-19 14:59:04]

Ah, I see it. Sorry. LGTM

[Tommy Widenflycht, 2014-02-20 12:34:19]

The CQ bit was checked by tommyw@chromium.org

[commit-bot: I haz the power, 2014-02-20 12:51:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommyw@chromium.org/148343008/400001

[commit-bot: I haz the power, 2014-02-20 16:56:58]

Change committed as 252269