Enable Oilpan by default in modules/speech/

Enable Oilpan by default in modules/speech/

R=dmazzoni@chromium.org,haraken@chromium.org,tommi@chromium.org,tkent@chromium.org
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176025

[sof, 2014-06-05 09:59:13]

Please take a look.

(Will of course ask for go-ahead from owners & other stakeholders.. just need to
locate who they are first :) )

[haraken, 2014-06-05 14:30:28]

LGTM in terms of oilpan. Let's wait for an approval of speech experts.

https://codereview.chromium.org/315133004/diff/60001/Source/modules/speech/Sp...
File Source/modules/speech/SpeechRecognitionEvent.cpp (left):

https://codereview.chromium.org/315133004/diff/60001/Source/modules/speech/Sp...
Source/modules/speech/SpeechRecognitionEvent.cpp:92:
ScriptWrappable::init(this);

wow...

https://codereview.chromium.org/315133004/diff/60001/Source/modules/speech/Sp...
File Source/modules/speech/SpeechSynthesisVoice.h (right):

https://codereview.chromium.org/315133004/diff/60001/Source/modules/speech/Sp...
Source/modules/speech/SpeechSynthesisVoice.h:55:
RefPtr<PlatformSpeechSynthesisVoice> m_platformVoice;

Nit: You might want to move this to the heap in a follow-up, but it's up to you.
There is no big benefit to move PlatformSpeechSynthesisVoice to the heap.

[sof, 2014-06-05 16:30:43]

I went one step too far with the speech events, forgetting that Event still
needs to be ref-ptr'ed non-Oilpan, and made their constructor methods return a
bare pointer.

Latest patchset changes their create()s back to having a transition type as
return type.

https://codereview.chromium.org/315133004/diff/60001/Source/modules/speech/Sp...
File Source/modules/speech/SpeechSynthesisVoice.h (right):

https://codereview.chromium.org/315133004/diff/60001/Source/modules/speech/Sp...
Source/modules/speech/SpeechSynthesisVoice.h:55:
RefPtr<PlatformSpeechSynthesisVoice> m_platformVoice;
On 2014/06/05 14:30:28, haraken wrote:
> 
> Nit: You might want to move this to the heap in a follow-up, but it's up to
you.
> There is no big benefit to move PlatformSpeechSynthesisVoice to the heap.

Thanks. I simply assumed (based on its name) this was a wrapping some browser
process object representing the voice, but not so.

[dmazzoni, 2014-06-05 23:43:07]

lgtm for speech synthesis, I don't know the recognition code.

https://codereview.chromium.org/315133004/diff/70001/Source/web/SpeechRecogni...
File Source/web/SpeechRecognitionClientProxy.cpp (right):

https://codereview.chromium.org/315133004/diff/70001/Source/web/SpeechRecogni...
Source/web/SpeechRecognitionClientProxy.cpp:79: SpeechRecognition*
recognition(handle);
This looks odd to me.

[haraken, 2014-06-06 01:11:16]

LGTM.

https://codereview.chromium.org/315133004/diff/70001/Source/modules/speech/Sp...
File Source/modules/speech/SpeechSynthesis.h (right):

https://codereview.chromium.org/315133004/diff/70001/Source/modules/speech/Sp...
Source/modules/speech/SpeechSynthesis.h:89: OwnPtr<PlatformSpeechSynthesizer>
m_platformSpeechSynthesizer;

PlatformSpeechSynthesizer would be another target to be moved to the heap, since
PlatformSpeechSynthesizer has a back pointer to the SpeechSynthesis object. (A
follow-up is fine.)

https://codereview.chromium.org/315133004/diff/70001/Source/modules/speech/Sp...
File Source/modules/speech/SpeechSynthesisUtterance.h (right):

https://codereview.chromium.org/315133004/diff/70001/Source/modules/speech/Sp...
Source/modules/speech/SpeechSynthesisUtterance.h:84:
RefPtr<PlatformSpeechSynthesisUtterance> m_platformUtterance;

And PlatformSpeechSynthesisUtterance would be another target as well.

[sof, 2014-06-06 06:21:09]

https://codereview.chromium.org/315133004/diff/70001/Source/web/SpeechRecogni...
File Source/web/SpeechRecognitionClientProxy.cpp (right):

https://codereview.chromium.org/315133004/diff/70001/Source/web/SpeechRecogni...
Source/web/SpeechRecognitionClientProxy.cpp:79: SpeechRecognition*
recognition(handle);
On 2014/06/05 23:43:06, dmazzoni wrote:
> This looks odd to me.

Would assigning 'handle' make it look less so? (just trying to understand what
prompted the comment.)

[sof, 2014-06-06 07:25:59]

+tommyw: would you be able to look at the speech recognition specific parts?
(see
https://groups.google.com/a/chromium.org/forum/#!topic/oilpan-reviews/1JuCGgq...
for context.)

[haraken, 2014-06-06 07:28:07]

> +tommyw: would you be able to look at the speech recognition specific parts?
> (see
>
https://groups.google.com/a/chromium.org/forum/#!topic/oilpan-reviews/1JuCGgq...
> for context.)

(I think tommyw is ooo for a while.)

[sof, 2014-06-06 07:44:46]

thanks, good to know.

+tommi: would you be able to look at the speech recognition specific parts
instead (or suggest someone else)? :)

[tommi (sloooow) - chröme, 2014-06-06 10:06:17]

lgtm

[sof, 2014-06-06 12:39:22]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-06-06 12:39:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sigbjornf@opera.com/315133004/70001

[commit-bot: I haz the power, 2014-06-06 13:48:29]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/7144)

[commit-bot: I haz the power, 2014-06-06 13:50:46]

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

[commit-bot: I haz the power, 2014-06-06 13:50:47]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/7156)

[sof, 2014-06-06 13:56:19]

On 2014/06/06 13:50:47, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   blink_presubmit on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/7156)

Needs public/ + Source/web/ approval.

tkent: could you take a look when you next have some time? thanks.

[tkent, 2014-06-09 01:28:46]

https://codereview.chromium.org/315133004/diff/70001/Source/web/WebSpeechGram...
File Source/web/WebSpeechGrammar.cpp (right):

https://codereview.chromium.org/315133004/diff/70001/Source/web/WebSpeechGram...
Source/web/WebSpeechGrammar.cpp:44: WebSpeechGrammar::WebSpeechGrammar(const
WebCore::SpeechGrammar* value)
This is not a mechanical change.  The argument type should be
|WebCore::SpeechGrammer*| or |WebCore::SpeechGrammer* const|.

|const PassRefPtrWillBeRawPtr<T>| means PassRefPtrWIllBeRawPtr is const.  |const
T*| means T is const.

[sof, 2014-06-09 12:24:32]

https://codereview.chromium.org/315133004/diff/70001/Source/web/WebSpeechGram...
File Source/web/WebSpeechGrammar.cpp (right):

https://codereview.chromium.org/315133004/diff/70001/Source/web/WebSpeechGram...
Source/web/WebSpeechGrammar.cpp:44: WebSpeechGrammar::WebSpeechGrammar(const
WebCore::SpeechGrammar* value)
On 2014/06/09 01:28:46, tkent wrote:
> This is not a mechanical change.  The argument type should be
> |WebCore::SpeechGrammer*| or |WebCore::SpeechGrammer* const|.
> 
> |const PassRefPtrWillBeRawPtr<T>| means PassRefPtrWIllBeRawPtr is const. 
|const
> T*| means T is const.

This is a good issue to bring up as we remove transition types & let go of
(smart) pointer wrapper types. We need to come to some agreement of what the
conversion constructors and copy assignment operator should take as arguments.

The use of 'const' previously served the purpose of not allowing the wrapper
object to be modified by these operators+constructors. We don't have those
wrappers any longer to contend with here.

I think the conversion constructor should now have the type as given here. The
use&placement of const isn't arbitrary - i.e., it should not be allowed to
modify the argument's value lest there is a good reason for allowing such. Your
suggestion either allows arbitrary modification of the pointed-to value, or
simply prevent modification of the pointer value (but not its contents.) Could
you explain the motivation for preferring either of those two?

[sof, 2014-06-09 12:28:16]

On 2014/06/09 12:24:32, sof wrote:
>
https://codereview.chromium.org/315133004/diff/70001/Source/web/WebSpeechGram...
> File Source/web/WebSpeechGrammar.cpp (right):
> 
>
https://codereview.chromium.org/315133004/diff/70001/Source/web/WebSpeechGram...
> Source/web/WebSpeechGrammar.cpp:44: WebSpeechGrammar::WebSpeechGrammar(const
> WebCore::SpeechGrammar* value)
> On 2014/06/09 01:28:46, tkent wrote:
> > This is not a mechanical change.  The argument type should be
> > |WebCore::SpeechGrammer*| or |WebCore::SpeechGrammer* const|.
> > 
> > |const PassRefPtrWillBeRawPtr<T>| means PassRefPtrWIllBeRawPtr is const. 
> |const
> > T*| means T is const.
> 
> This is a good issue to bring up as we remove transition types & let go of
> (smart) pointer wrapper types. We need to come to some agreement of what the
> conversion constructors and copy assignment operator should take as arguments.
> 
> The use of 'const' previously served the purpose of not allowing the wrapper
> object to be modified by these operators+constructors. We don't have those
> wrappers any longer to contend with here.
> 
> I think the conversion constructor should now have the type as given here. The
> use&placement of const isn't arbitrary - i.e., it should not be allowed to
> modify the argument's value lest there is a good reason for allowing such.
Your
> suggestion either allows arbitrary modification of the pointed-to value, or
> simply prevent modification of the pointer value (but not its contents.) Could
> you explain the motivation for preferring either of those two?

Oops, forgot to add - the use of const_cast<> when constructing the
WebPrivatePtr<> here is unfortunate, but a separate issue. Coming up with a set
of assign() overloads for heap-origined private pointers that allow RawPtr<> and
(const) pointers to co-exist is proving tricky.

[tkent, 2014-06-10 03:19:14]

https://codereview.chromium.org/315133004/diff/70001/Source/web/WebSpeechGram...
File Source/web/WebSpeechGrammar.cpp (right):

https://codereview.chromium.org/315133004/diff/70001/Source/web/WebSpeechGram...
Source/web/WebSpeechGrammar.cpp:44: WebSpeechGrammar::WebSpeechGrammar(const
WebCore::SpeechGrammar* value)
On 2014/06/09 12:24:32, sof wrote:
> On 2014/06/09 01:28:46, tkent wrote:
> > This is not a mechanical change.  The argument type should be
> > |WebCore::SpeechGrammer*| or |WebCore::SpeechGrammer* const|.
> > 
> > |const PassRefPtrWillBeRawPtr<T>| means PassRefPtrWIllBeRawPtr is const. 
> |const
> > T*| means T is const.
> 
> This is a good issue to bring up as we remove transition types & let go of
> (smart) pointer wrapper types. We need to come to some agreement of what the
> conversion constructors and copy assignment operator should take as arguments.
> 
> The use of 'const' previously served the purpose of not allowing the wrapper
> object to be modified by these operators+constructors. We don't have those
> wrappers any longer to contend with here.
> 
> I think the conversion constructor should now have the type as given here. The
> use&placement of const isn't arbitrary - i.e., it should not be allowed to
> modify the argument's value lest there is a good reason for allowing such.
Your
> suggestion either allows arbitrary modification of the pointed-to value, or
> simply prevent modification of the pointer value (but not its contents.) Could
> you explain the motivation for preferring either of those two?

I understand your point.  But it's not the topic of this CL.
WebFoo wrapper classes have function members which update the corresponding core
classes.  So passing |const Foo*| is not practical.

[sof, 2014-06-10 06:28:41]

On 2014/06/10 03:19:14, tkent wrote:
>
https://codereview.chromium.org/315133004/diff/70001/Source/web/WebSpeechGram...
> File Source/web/WebSpeechGrammar.cpp (right):
> 
>
https://codereview.chromium.org/315133004/diff/70001/Source/web/WebSpeechGram...
> Source/web/WebSpeechGrammar.cpp:44: WebSpeechGrammar::WebSpeechGrammar(const
> WebCore::SpeechGrammar* value)
> On 2014/06/09 12:24:32, sof wrote:
> > On 2014/06/09 01:28:46, tkent wrote:
> > > This is not a mechanical change.  The argument type should be
> > > |WebCore::SpeechGrammer*| or |WebCore::SpeechGrammer* const|.
> > > 
> > > |const PassRefPtrWillBeRawPtr<T>| means PassRefPtrWIllBeRawPtr is const. 
> > |const
> > > T*| means T is const.
> > 
> > This is a good issue to bring up as we remove transition types & let go of
> > (smart) pointer wrapper types. We need to come to some agreement of what the
> > conversion constructors and copy assignment operator should take as
arguments.
> > 
> > The use of 'const' previously served the purpose of not allowing the wrapper
> > object to be modified by these operators+constructors. We don't have those
> > wrappers any longer to contend with here.
> > 
> > I think the conversion constructor should now have the type as given here.
The
> > use&placement of const isn't arbitrary - i.e., it should not be allowed to
> > modify the argument's value lest there is a good reason for allowing such.
> Your
> > suggestion either allows arbitrary modification of the pointed-to value, or
> > simply prevent modification of the pointer value (but not its contents.)
Could
> > you explain the motivation for preferring either of those two?
> 
> I understand your point.  But it's not the topic of this CL.
> WebFoo wrapper classes have function members which update the corresponding
core
> classes.  So passing |const Foo*| is not practical.

Alright, dropped 'const's from view.

[tkent, 2014-06-10 06:34:52]

lgtm

[sof, 2014-06-10 06:42:42]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-06-10 06:43:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sigbjornf@opera.com/315133004/90001

[commit-bot: I haz the power, 2014-06-10 07:35:45]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-06-10 07:35:58]

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

[commit-bot: I haz the power, 2014-06-10 07:35:59]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[sof, 2014-06-10 07:39:36]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-06-10 07:40:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sigbjornf@opera.com/315133004/90001

[commit-bot: I haz the power, 2014-06-10 07:40:52]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-06-10 07:41:06]

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

[commit-bot: I haz the power, 2014-06-10 07:41:07]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[sof, 2014-06-10 08:29:08]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-06-10 08:29:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sigbjornf@opera.com/315133004/90001

[commit-bot: I haz the power, 2014-06-10 08:30:39]

Change committed as 175872

[Pat Meenan, 2014-06-11 15:30:11]

A revert of this CL has been created in
https://codereview.chromium.org/330463002/ by pmeenan@chromium.org.

The reason for reverting is: Suspected cause of crash when running Speedometer
on the perf bots: https://code.google.com/p/chromium/issues/detail?id=383338.

[sof, 2014-06-12 11:13:41]

With the fix for https://code.google.com/p/chromium/issues/detail?id=345061 in
place, relanding.

[sof, 2014-06-12 11:14:51]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-06-12 11:15:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sigbjornf@opera.com/315133004/90001

[commit-bot: I haz the power, 2014-06-12 11:16:19]

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

[commit-bot: I haz the power, 2014-06-12 11:16:21]

Failed to apply patch for Source/modules/speech/SpeechRecognition.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/modules/speech/SpeechRecognition.cpp
  Hunk #1 FAILED at 37.
  1 out of 2 hunks FAILED -- saving rejects to file
Source/modules/speech/SpeechRecognition.cpp.rej

Patch:       Source/modules/speech/SpeechRecognition.cpp
Index: Source/modules/speech/SpeechRecognition.cpp
diff --git a/Source/modules/speech/SpeechRecognition.cpp
b/Source/modules/speech/SpeechRecognition.cpp
index
eed96414d64de564a7d88c8183ef74e10ac1ce5b..09b1535198c4cd21fab02fcebb0d7d235d96b348
100644
--- a/Source/modules/speech/SpeechRecognition.cpp
+++ b/Source/modules/speech/SpeechRecognition.cpp
@@ -37,11 +37,11 @@
 
 namespace WebCore {
 
-PassRefPtrWillBeRawPtr<SpeechRecognition>
SpeechRecognition::create(ExecutionContext* context)
+SpeechRecognition* SpeechRecognition::create(ExecutionContext* context)
 {
-    RefPtrWillBeRawPtr<SpeechRecognition>
speechRecognition(adoptRefWillBeRefCountedGarbageCollected(new
SpeechRecognition(context)));
+    SpeechRecognition* speechRecognition = adoptRefCountedGarbageCollected(new
SpeechRecognition(context));
     speechRecognition->suspendIfNeeded();
-    return speechRecognition.release();
+    return speechRecognition;
 }
 
 void SpeechRecognition::start(ExceptionState& exceptionState)
@@ -105,21 +105,21 @@ void SpeechRecognition::didEndAudio()
     dispatchEvent(Event::create(EventTypeNames::audioend));
 }
 
-void SpeechRecognition::didReceiveResults(const
WillBeHeapVector<RefPtrWillBeMember<SpeechRecognitionResult> >& newFinalResults,
const WillBeHeapVector<RefPtrWillBeMember<SpeechRecognitionResult> >&
currentInterimResults)
+void SpeechRecognition::didReceiveResults(const
HeapVector<Member<SpeechRecognitionResult> >& newFinalResults, const
HeapVector<Member<SpeechRecognitionResult> >& currentInterimResults)
 {
     unsigned long resultIndex = m_finalResults.size();
 
     for (size_t i = 0; i < newFinalResults.size(); ++i)
         m_finalResults.append(newFinalResults[i]);
 
-    WillBeHeapVector<RefPtrWillBeMember<SpeechRecognitionResult> > results =
m_finalResults;
+    HeapVector<Member<SpeechRecognitionResult> > results = m_finalResults;
     for (size_t i = 0; i < currentInterimResults.size(); ++i)
         results.append(currentInterimResults[i]);
 
     dispatchEvent(SpeechRecognitionEvent::createResult(resultIndex, results));
 }
 
-void
SpeechRecognition::didReceiveNoMatch(PassRefPtrWillBeRawPtr<SpeechRecognitionResult>
result)
+void SpeechRecognition::didReceiveNoMatch(SpeechRecognitionResult* result)
 {
     dispatchEvent(SpeechRecognitionEvent::createNoMatch(result));
 }

[sof, 2014-06-12 12:21:02]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-06-12 12:22:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sigbjornf@opera.com/315133004/110001

[commit-bot: I haz the power, 2014-06-12 13:27:56]

Change committed as 176025