Fixed a memory leak in SpeechRecognitionManagerImpl causing ChromeSpeechInputManagerDelegate to be …

Fixed a memory leak in SpeechRecognitionManagerImpl causing ChromeSpeechInputManagerDelegate to be never freed.


BUG=123116, 116954
TEST=Run Valgrind tests.

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

[Primiano Tucci (use gerrit), 2012-04-13 10:16:40]

Made a minor stylistic fix, seems that ".get()" is superfluous since scoped_ptr
has an overriden == operator.

[Primiano Tucci (use gerrit), 2012-04-13 10:37:48]

Just a further question:
since SpeechRecognitionManagerImpl is a singleton (since, indeed, we expect to
have at most one instance in chromium), I would naturally expect that also the
ChromeSpeechRecognitionManagerDelegate (that is a delegate for the manager) to
be a singleton, since it would make no particular sense having more than one
instance.
I'd propose to turn ChromeSpeechRecognitionManagerDelegate into a singleton and
just change the 
ChromeContentBrowserClient::GetSpeechRecognitionManagerDelegate() method in
order to return the singleton instance, rather than instantiating a new one.
What do you think?

[jam, 2012-04-13 18:19:54]

http://codereview.chromium.org/10071040/diff/4003/content/browser/speech/spee...
File content/browser/speech/speech_recognition_manager_impl.cc (right):

http://codereview.chromium.org/10071040/diff/4003/content/browser/speech/spee...
content/browser/speech/speech_recognition_manager_impl.cc:175: if (delegate_ !=
NULL) {
by convention we don't do if (foo != NULL), but just if (foo). in case of scoped
ptrs, if (foo.get())

[jam, 2012-04-13 18:20:54]

On 2012/04/13 10:37:48, Primiano Tucci wrote:
> Just a further question:
> since SpeechRecognitionManagerImpl is a singleton (since, indeed, we expect to
> have at most one instance in chromium), I would naturally expect that also the
> ChromeSpeechRecognitionManagerDelegate (that is a delegate for the manager) to
> be a singleton, since it would make no particular sense having more than one
> instance.
> I'd propose to turn ChromeSpeechRecognitionManagerDelegate into a singleton
and
> just change the 
> ChromeContentBrowserClient::GetSpeechRecognitionManagerDelegate() method in
> order to return the singleton instance, rather than instantiating a new one.
> What do you think?

i think it's better if we don't make embedders's delegate implementation tied to
what we have in content now. we might change that in the future, and this way
gives us more flexibility.

[Primiano Tucci (use gerrit), 2012-04-16 08:47:37]

http://codereview.chromium.org/10071040/diff/4003/content/browser/speech/spee...
File content/browser/speech/speech_recognition_manager_impl.cc (right):

http://codereview.chromium.org/10071040/diff/4003/content/browser/speech/spee...
content/browser/speech/speech_recognition_manager_impl.cc:175: if (delegate_ !=
NULL) {
On 2012/04/13 18:19:54, John Abd-El-Malek wrote:
> by convention we don't do if (foo != NULL), but just if (foo). in case of
scoped
> ptrs, if (foo.get())

Ah, I am sorry, I was not aware. I fixed it.
Can I just ask the rationale behind this convention?
I opted for != NULL just because it would work keeping the same semantic
regardless the type of pointer (smart or not) being used.

[Satish, 2012-04-16 09:00:32]

lgtm

[commit-bot: I haz the power, 2012-04-16 09:16:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/primiano@chromium.org/10071040/9001

[commit-bot: I haz the power, 2012-04-16 11:44:44]

Try job failure for 10071040-9001 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-04-16 11:51:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/primiano@chromium.org/10071040/9001

[commit-bot: I haz the power, 2012-04-16 14:20:56]

Try job failure for 10071040-9001 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[jam, 2012-04-17 03:19:03]

On 2012/04/16 08:47:37, Primiano Tucci wrote:
>
http://codereview.chromium.org/10071040/diff/4003/content/browser/speech/spee...
> File content/browser/speech/speech_recognition_manager_impl.cc (right):
> 
>
http://codereview.chromium.org/10071040/diff/4003/content/browser/speech/spee...
> content/browser/speech/speech_recognition_manager_impl.cc:175: if (delegate_
!=
> NULL) {
> On 2012/04/13 18:19:54, John Abd-El-Malek wrote:
> > by convention we don't do if (foo != NULL), but just if (foo). in case of
> scoped
> > ptrs, if (foo.get())
> 
> Ah, I am sorry, I was not aware. I fixed it.
> Can I just ask the rationale behind this convention?

I'm not 100% sure what the reason is, perhaps it's just for simplicity. i.e. we
don't write if (boolean_variable == true)


> I opted for != NULL just because it would work keeping the same semantic
> regardless the type of pointer (smart or not) being used.

it seems that this is orthogonal. whether you use smart pointer or not, the left
side of the operator is the only thing that's affected. i.e. if (naked_ptr) vs
if (smart_pointer.get()). with both of these you can add a "!= NULL" but that
doesn't add anything

[Primiano Tucci (use gerrit), 2012-04-17 08:20:48]

> it seems that this is orthogonal. whether you use smart pointer or not, the
left
> side of the operator is the only thing that's affected. i.e. if (naked_ptr) vs
> if (smart_pointer.get()). with both of these you can add a "!= NULL" but that
> doesn't add anything

Hmm my point was that "if (x != NULL)" seemed to me a bit more "universal",
since it works both in the case of a naked or a smart pointer. So, in the case
of future changes to the pointer type, no changes are required on the
corresponding "if" lines.
On the other side, "if (x)" works only with a naked pointer (and similarly "if
(x.get())" does only with a smart one)