[WIP]Add WebPrivateHeapPtr and use it for speech platform layer.

[WIP]Add WebPrivateHeapPtr and use it for speech platform layer.

Add the ability for a platform layer object to wrap a heap reference,
using a Persistent. If Oilpan is disabled, it effectively
'degenerates' into a WebPrivatePtr.

R=
BUG=

[haraken, 2014-02-17 08:00:51]

This looks like a good approach! Ping us once it's ready for review.

Once this CL lands, we can use WebPrivateHeapPtr in the clipboard CL
(https://codereview.chromium.org/169323002).

[sof, 2014-02-17 08:19:33]

On 2014/02/17 08:00:51, haraken wrote:
> This looks like a good approach! Ping us once it's ready for review.
> 
> Once this CL lands, we can use WebPrivateHeapPtr in the clipboard CL
> (https://codereview.chromium.org/169323002).

I can definitely do that. If anyone has already gone down this path
and found it a blind (&dark) alley, please chime in before I go
too far :)

[haraken, 2014-02-17 09:04:57]

Overall the approach looks reasonable to me.

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

https://codereview.chromium.org/169343002/diff/1/Source/modules/speech/Speech...
Source/modules/speech/SpeechRecognition.cpp:196: visitor->trace(m_grammars);

You need to trace m_finalResults.

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

https://codereview.chromium.org/169343002/diff/1/Source/modules/speech/Speech...
Source/modules/speech/SpeechRecognitionResultList.cpp:51: #if 0

Remove this.

https://codereview.chromium.org/169343002/diff/1/Source/web/SpeechInputClient...
File Source/web/SpeechInputClientImpl.cpp (right):

https://codereview.chromium.org/169343002/diff/1/Source/web/SpeechInputClient...
Source/web/SpeechInputClientImpl.cpp:101: webcoreResults[i] =
static_cast<WTF::PassRefPtrWillBeRawPtr<WebCore::SpeechInputResult>
>(results[i]);

This cast looks weird. Can we remove this somehow? (I haven't yet looked at why
the cast is needed.)

https://codereview.chromium.org/169343002/diff/1/Source/web/SpeechRecognition...
File Source/web/SpeechRecognitionClientProxy.cpp (right):

https://codereview.chromium.org/169343002/diff/1/Source/web/SpeechRecognition...
Source/web/SpeechRecognitionClientProxy.cpp:105: RefPtr<SpeechRecognition>
recognition = PassRefPtr<SpeechRecognition>(handle);

Shouldn't this be RefPtrWillBeRawPtr<SpeechRecognition> ?

https://codereview.chromium.org/169343002/diff/1/Source/web/SpeechRecognition...
Source/web/SpeechRecognitionClientProxy.cpp:107:
Vector<RefPtrWillBeRawPtr<SpeechRecognitionResult> >
finalResultsVector(newFinalResults.size());

This should be WillBeHeapVector<RefPtrWillBeMember>.

https://codereview.chromium.org/169343002/diff/1/Source/web/SpeechRecognition...
Source/web/SpeechRecognitionClientProxy.cpp:109: finalResultsVector[i] =
static_cast<PassRefPtrWillBeRawPtr<SpeechRecognitionResult>
>(newFinalResults[i]);

Can we remove this cast?

https://codereview.chromium.org/169343002/diff/1/Source/web/SpeechRecognition...
Source/web/SpeechRecognitionClientProxy.cpp:111:
Vector<RefPtrWillBeRawPtr<SpeechRecognitionResult> >
interimResultsVector(currentInterimResults.size());

This should be WillBeHeapVector<RefPtrWillBeMember>.

https://codereview.chromium.org/169343002/diff/1/Source/web/SpeechRecognition...
Source/web/SpeechRecognitionClientProxy.cpp:113: interimResultsVector[i] =
static_cast<PassRefPtrWillBeRawPtr<SpeechRecognitionResult>
>(currentInterimResults[i]);

Can we remove this cast?

https://codereview.chromium.org/169343002/diff/1/Source/web/WebSpeechRecognit...
File Source/web/WebSpeechRecognitionResult.cpp (right):

https://codereview.chromium.org/169343002/diff/1/Source/web/WebSpeechRecognit...
Source/web/WebSpeechRecognitionResult.cpp:70:
WebSpeechRecognitionResult::operator
PassRefPtrWillBeRawPtr<WebCore::SpeechRecognitionResult>() const

I'm afraid that this implicit conversion will lead to unexpected error. Shall we
remove this? (We've removed this kind of implicit conversions from the
experimental branch.)

https://codereview.chromium.org/169343002/diff/1/public/platform/WebPrivateHe...
File public/platform/WebPrivateHeapPtr.h (right):

https://codereview.chromium.org/169343002/diff/1/public/platform/WebPrivateHe...
public/platform/WebPrivateHeapPtr.h:2: * Copyright (c) 2014, Opera Software ASA.
All rights reserved.

Can you use the 4-line copyright?

https://codereview.chromium.org/169343002/diff/1/public/platform/WebPrivateHe...
public/platform/WebPrivateHeapPtr.h:35: #if INSIDE_BLINK

What's the difference between INSIDE_BLINK and BLINK_IMPLEMENTATION ?

https://codereview.chromium.org/169343002/diff/1/public/platform/WebPrivateHe...
public/platform/WebPrivateHeapPtr.h:45: // to WebPrivatePtr and the object will
be reference counted.

"heap" is ambiguous. We want to use "garbage-collected heap" or "oilpan" for
clarity.

https://codereview.chromium.org/169343002/diff/1/public/platform/WebPrivateHe...
public/platform/WebPrivateHeapPtr.h:82: assign(new WebCore::Persistent<T>(p));

How about calling 'new Persistent' in assign? assign can take a raw pointer.

https://codereview.chromium.org/169343002/diff/1/public/web/WebSpeechGrammar.h
File public/web/WebSpeechGrammar.h (right):

https://codereview.chromium.org/169343002/diff/1/public/web/WebSpeechGrammar....
public/web/WebSpeechGrammar.h:52: WebSpeechGrammar(const
WTF::PassRefPtrWillBeRawPtr<WebCore::SpeechGrammar>&);

Add explicit.

https://codereview.chromium.org/169343002/diff/1/public/web/WebSpeechGrammar....
public/web/WebSpeechGrammar.h:53: WebSpeechGrammar& operator=(const
WTF::PassRefPtrWillBeRawPtr<WebCore::SpeechGrammar>&);

Add explicit.

https://codereview.chromium.org/169343002/diff/1/public/web/WebSpeechGrammar....
public/web/WebSpeechGrammar.h:57: WebPrivateHeapPtr<WebCore::SpeechGrammar>
m_private;

Shall we call this WebPrivateGarbageCollectedPtr ? "Heap" sounds ambiguous.

[sof, 2014-02-17 09:23:21]

Great, thanks for the valuable feedback.

I'm splitting out WebPrivateGarbageCollectedPtr (nee WebPrivateHeapPtr) into a
separate CL & will take the comments into account there.

https://codereview.chromium.org/169343002/diff/1/public/platform/WebPrivateHe...
File public/platform/WebPrivateHeapPtr.h (right):

https://codereview.chromium.org/169343002/diff/1/public/platform/WebPrivateHe...
public/platform/WebPrivateHeapPtr.h:82: assign(new WebCore::Persistent<T>(p));
On 2014/02/17 09:04:57, haraken wrote:
> 
> How about calling 'new Persistent' in assign? assign can take a raw pointer.

Yes, let's do that.

https://codereview.chromium.org/169343002/diff/1/public/web/WebSpeechGrammar.h
File public/web/WebSpeechGrammar.h (right):

https://codereview.chromium.org/169343002/diff/1/public/web/WebSpeechGrammar....
public/web/WebSpeechGrammar.h:57: WebPrivateHeapPtr<WebCore::SpeechGrammar>
m_private;
On 2014/02/17 09:04:57, haraken wrote:
> 
> Shall we call this WebPrivateGarbageCollectedPtr ? "Heap" sounds ambiguous.

Alright; I'm splitting out this wrapper class into a CL of its own.

[sof, 2014-02-17 10:22:25]

https://codereview.chromium.org/169343002/diff/1/public/platform/WebPrivateHe...
File public/platform/WebPrivateHeapPtr.h (right):

https://codereview.chromium.org/169343002/diff/1/public/platform/WebPrivateHe...
public/platform/WebPrivateHeapPtr.h:35: #if INSIDE_BLINK
On 2014/02/17 09:04:57, haraken wrote:
> 
> What's the difference between INSIDE_BLINK and BLINK_IMPLEMENTATION ?

Good question; I can't discern a pattern within this directory, but the former
is more common.

If one is considered preferable for new files, please let me know & I'll adapt.

[tkent, 2014-02-17 23:42:46]

https://codereview.chromium.org/169343002/diff/1/public/platform/WebPrivateHe...
File public/platform/WebPrivateHeapPtr.h (right):

https://codereview.chromium.org/169343002/diff/1/public/platform/WebPrivateHe...
public/platform/WebPrivateHeapPtr.h:35: #if INSIDE_BLINK
On 2014/02/17 10:22:26, sof wrote:
> On 2014/02/17 09:04:57, haraken wrote:
> > 
> > What's the difference between INSIDE_BLINK and BLINK_IMPLEMENTATION ?
> 
> Good question; I can't discern a pattern within this directory, but the former
> is more common.
> 
> If one is considered preferable for new files, please let me know & I'll
adapt.

INSIDE_BLINK means that it's in either of webkit.dll, blink_common.dll,
blink_heap.dll, or blink_platform.dll.
BLINK_IMPLEMENTATION means that it's in webkit.dll.  We have other macros like
BLINK_COMMON_IMPLEMENTATION, HEAP_IMPLEMENTATION, and
BLINK_PLATFORM_IMPLEMENTATION.

INSIDE_BLINK is appropriate here because it's ok to refer to Handle.h and
PassRefPtr.h in any Blink DLLs.

[sof, 2014-02-18 10:14:16]

https://codereview.chromium.org/169343002/diff/1/public/platform/WebPrivateHe...
File public/platform/WebPrivateHeapPtr.h (right):

https://codereview.chromium.org/169343002/diff/1/public/platform/WebPrivateHe...
public/platform/WebPrivateHeapPtr.h:35: #if INSIDE_BLINK
On 2014/02/17 23:42:46, tkent wrote:
> On 2014/02/17 10:22:26, sof wrote:
> > On 2014/02/17 09:04:57, haraken wrote:
> > > 
> > > What's the difference between INSIDE_BLINK and BLINK_IMPLEMENTATION ?
> > 
> > Good question; I can't discern a pattern within this directory, but the
former
> > is more common.
> > 
> > If one is considered preferable for new files, please let me know & I'll
> adapt.
> 
> INSIDE_BLINK means that it's in either of webkit.dll, blink_common.dll,
> blink_heap.dll, or blink_platform.dll.
> BLINK_IMPLEMENTATION means that it's in webkit.dll.  We have other macros like
> BLINK_COMMON_IMPLEMENTATION, HEAP_IMPLEMENTATION, and
> BLINK_PLATFORM_IMPLEMENTATION.
> 
> INSIDE_BLINK is appropriate here because it's ok to refer to Handle.h and
> PassRefPtr.h in any Blink DLLs.
> 

Excellent clarification & info, many thanks :)

On that last point, would it be acceptable to let public/WebPrivatePtr.h
#include heap/Handle.h? (cf. https://codereview.chromium.org/168963003/#msg30
... well, it's not WebWillBePrivateGarbageCollectedPtr.h any longer, but
WebPrivatePtr.h)

[abarth-chromium, 2014-02-18 17:38:50]

On 2014/02/18 10:14:16, sof wrote:
> On that last point, would it be acceptable to let public/WebPrivatePtr.h
> #include heap/Handle.h? (cf. https://codereview.chromium.org/168963003/#msg30
> ... well, it's not WebWillBePrivateGarbageCollectedPtr.h any longer, but
> WebPrivatePtr.h)

I haven't looked at this in detail, but generally speaking we can include Source
headers behind INSIDE_BLINK ifdefs.  The headers are visible to the various
Blink DLLs, but not to non-Blink DLLs.