Remove usage of {,un}setPendingActivity from SpeechRecognition

Remove usage of {,un}setPendingActivity from SpeechRecognition

{,un}setPendingActivity relies on RefCounted, which is going to be deprecated in oilpan. This CL removes usage of these methods in SpeechRecognition.cpp, and provide hasPendingActivity() implementation as replacement

BUG=354845
TEST=fast/speech

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169746

[haraken, 2014-03-21 14:34:14]

https://codereview.chromium.org/208273002/diff/1/Source/modules/speech/Speech...
File Source/modules/speech/SpeechRecognition.cpp (right):

https://codereview.chromium.org/208273002/diff/1/Source/modules/speech/Speech...
Source/modules/speech/SpeechRecognition.cpp:130: m_started = false;

Before this CL, we didn't unset pending activity when didReceiveError is called.
However, after this CL, we unset pending activity when didReceiveError is
called.

Is this change OK? Can you add a test that covers this behavior?

[kouhei (in TOK), 2014-03-21 14:53:31]

On 2014/03/21 14:34:14, haraken wrote:
>
https://codereview.chromium.org/208273002/diff/1/Source/modules/speech/Speech...
> File Source/modules/speech/SpeechRecognition.cpp (right):
> 
>
https://codereview.chromium.org/208273002/diff/1/Source/modules/speech/Speech...
> Source/modules/speech/SpeechRecognition.cpp:130: m_started = false;
> 
> Before this CL, we didn't unset pending activity when didReceiveError is
called.
> However, after this CL, we unset pending activity when didReceiveError is
> called.
> 
> Is this change OK? Can you add a test that covers this behavior?

speech/scripted/speechrecognition-errors.html already covers the behavior.
It is guaranteed that no further event is created after this error event. See
comments on
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...

[haraken, 2014-03-21 14:55:42]

On 2014/03/21 14:53:31, kouhei wrote:
> On 2014/03/21 14:34:14, haraken wrote:
> >
>
https://codereview.chromium.org/208273002/diff/1/Source/modules/speech/Speech...
> > File Source/modules/speech/SpeechRecognition.cpp (right):
> > 
> >
>
https://codereview.chromium.org/208273002/diff/1/Source/modules/speech/Speech...
> > Source/modules/speech/SpeechRecognition.cpp:130: m_started = false;
> > 
> > Before this CL, we didn't unset pending activity when didReceiveError is
> called.
> > However, after this CL, we unset pending activity when didReceiveError is
> > called.
> > 
> > Is this change OK? Can you add a test that covers this behavior?
> 
> speech/scripted/speechrecognition-errors.html already covers the behavior.
> It is guaranteed that no further event is created after this error event. See
> comments on
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...

Then why didn't we call unsetPendingActivity() in didReceiveError(), before your
CL?

Also, would you cc SpeechRecognition folks?

[kouhei (in TOK), 2014-03-21 14:59:28]

On 2014/03/21 14:55:42, haraken wrote:
> On 2014/03/21 14:53:31, kouhei wrote:
> > On 2014/03/21 14:34:14, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/208273002/diff/1/Source/modules/speech/Speech...
> > > File Source/modules/speech/SpeechRecognition.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/208273002/diff/1/Source/modules/speech/Speech...
> > > Source/modules/speech/SpeechRecognition.cpp:130: m_started = false;
> > > 
> > > Before this CL, we didn't unset pending activity when didReceiveError is
> > called.
> > > However, after this CL, we unset pending activity when didReceiveError is
> > > called.
> > > 
> > > Is this change OK? Can you add a test that covers this behavior?
> > 
> > speech/scripted/speechrecognition-errors.html already covers the behavior.
> > It is guaranteed that no further event is created after this error event.
See
> > comments on
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...
> 
> Then why didn't we call unsetPendingActivity() in didReceiveError(), before
your
> CL?
I have no idea. It is possible that this leaked.

> Also, would you cc SpeechRecognition folks?

Sorry I couldn't find an active maintainer of this code.

[haraken, 2014-03-21 15:06:49]

On 2014/03/21 14:59:28, kouhei wrote:
> On 2014/03/21 14:55:42, haraken wrote:
> > On 2014/03/21 14:53:31, kouhei wrote:
> > > On 2014/03/21 14:34:14, haraken wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/208273002/diff/1/Source/modules/speech/Speech...
> > > > File Source/modules/speech/SpeechRecognition.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/208273002/diff/1/Source/modules/speech/Speech...
> > > > Source/modules/speech/SpeechRecognition.cpp:130: m_started = false;
> > > > 
> > > > Before this CL, we didn't unset pending activity when didReceiveError is
> > > called.
> > > > However, after this CL, we unset pending activity when didReceiveError
is
> > > > called.
> > > > 
> > > > Is this change OK? Can you add a test that covers this behavior?
> > > 
> > > speech/scripted/speechrecognition-errors.html already covers the behavior.
> > > It is guaranteed that no further event is created after this error event.
> See
> > > comments on
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...
> > 
> > Then why didn't we call unsetPendingActivity() in didReceiveError(), before
> your
> > CL?
> I have no idea. It is possible that this leaked.
> 
> > Also, would you cc SpeechRecognition folks?
> 
> Sorry I couldn't find an active maintainer of this code.

ok... The new code looks more reasonable to me than the previous code, since the
pending activity should be cleared when an error is received. LGTM from my side.

abarth@: Would you mind taking another look? There is no active maintainer in
this area.

[abarth-chromium, 2014-03-21 15:51:00]

lgtm

[kouhei (in TOK), 2014-03-21 16:12:53]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2014-03-21 16:13:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/208273002/1

[commit-bot: I haz the power, 2014-03-21 16:15:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-21 16:15:24]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg
  tryserver.blink on win_blink_compile_dbg

[kouhei (in TOK), 2014-03-21 16:16:08]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2014-03-21 16:16:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/208273002/1

[commit-bot: I haz the power, 2014-03-21 17:16:49]

Change committed as 169746