Move mediastream module to oilpan transition types

Move mediastream module to oilpan

MediaStreamTrack::Observer was only used for MediaStream so I removed it in order to avoid inheriting GarbageCollected twice.

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

[sof, 2014-02-20 15:30:22]

https://codereview.chromium.org/173363002/diff/50001/Source/modules/mediastre...
File Source/modules/mediastream/MediaDeviceInfo.h (right):

https://codereview.chromium.org/173363002/diff/50001/Source/modules/mediastre...
Source/modules/mediastream/MediaDeviceInfo.h:55: typedef
PersistentHeapVector<RefPtrWillBeMember<MediaDeviceInfo> >
MediaDeviceInfoVector;
Prefix WillBe ?

https://codereview.chromium.org/173363002/diff/50001/Source/modules/mediastre...
File Source/modules/mediastream/MediaDevicesRequest.h (right):

https://codereview.chromium.org/173363002/diff/50001/Source/modules/mediastre...
Source/modules/mediastream/MediaDevicesRequest.h:44: class MediaDevicesRequest
FINAL : public RefCountedWillBeGarbageCollected<MediaDevicesRequest>, public
ActiveDOMObject {
Add *Finalized

https://codereview.chromium.org/173363002/diff/50001/Source/modules/mediastre...
File Source/modules/mediastream/MediaStreamTrackSourcesCallback.h (right):

https://codereview.chromium.org/173363002/diff/50001/Source/modules/mediastre...
Source/modules/mediastream/MediaStreamTrackSourcesCallback.h:37: virtual void
handleEvent(WillBeHeapVector<RefPtrWillBeMember<SourceInfo> > sources) = 0;
Would passing it by (const) reference still work?

https://codereview.chromium.org/173363002/diff/50001/Source/modules/mediastre...
File Source/modules/mediastream/NavigatorUserMediaError.h (right):

https://codereview.chromium.org/173363002/diff/50001/Source/modules/mediastre...
Source/modules/mediastream/NavigatorUserMediaError.h:35: class
NavigatorUserMediaError FINAL : public
RefCountedWillBeGarbageCollected<NavigatorUserMediaError>, public
ScriptWrappable {
Add *Finalized

https://codereview.chromium.org/173363002/diff/50001/Source/modules/mediastre...
File Source/modules/mediastream/RTCIceCandidate.h (right):

https://codereview.chromium.org/173363002/diff/50001/Source/modules/mediastre...
Source/modules/mediastream/RTCIceCandidate.h:46: class RTCIceCandidate FINAL :
public RefCountedWillBeGarbageCollected<RTCIceCandidate>, public ScriptWrappable
{
Add *Finalized

https://codereview.chromium.org/173363002/diff/50001/Source/modules/mediastre...
File Source/modules/mediastream/RTCStatsReport.h (right):

https://codereview.chromium.org/173363002/diff/50001/Source/modules/mediastre...
Source/modules/mediastream/RTCStatsReport.h:38: class RTCStatsReport : public
RefCountedWillBeGarbageCollected<RTCStatsReport>, public ScriptWrappable {
Add *Finalized

https://codereview.chromium.org/173363002/diff/50001/Source/modules/mediastre...
File Source/modules/mediastream/SourceInfo.h (right):

https://codereview.chromium.org/173363002/diff/50001/Source/modules/mediastre...
Source/modules/mediastream/SourceInfo.h:37: class SourceInfo : public
RefCountedWillBeGarbageCollected<SourceInfo>, public ScriptWrappable {
RefCountedWillBeGarbageCollectedFinalized

https://codereview.chromium.org/173363002/diff/50001/Source/modules/mediastre...
File Source/modules/mediastream/UserMediaRequest.h (right):

https://codereview.chromium.org/173363002/diff/50001/Source/modules/mediastre...
Source/modules/mediastream/UserMediaRequest.h:51: class UserMediaRequest FINAL :
public RefCountedWillBeGarbageCollected<UserMediaRequest>, public
ContextLifecycleObserver {
I think the observer needs finalization (cf
LifecycleObserver::~LifecycleObserver()), so add *Finalized here ?

[keishi, 2014-02-21 08:00:24]

https://codereview.chromium.org/173363002/diff/50001/Source/modules/mediastre...
File Source/modules/mediastream/MediaDeviceInfo.h (right):

https://codereview.chromium.org/173363002/diff/50001/Source/modules/mediastre...
Source/modules/mediastream/MediaDeviceInfo.h:55: typedef
PersistentHeapVector<RefPtrWillBeMember<MediaDeviceInfo> >
MediaDeviceInfoVector;
On 2014/02/20 15:30:23, sof wrote:
> Prefix WillBe ?

Done.

https://codereview.chromium.org/173363002/diff/50001/Source/modules/mediastre...
File Source/modules/mediastream/MediaDevicesRequest.h (right):

https://codereview.chromium.org/173363002/diff/50001/Source/modules/mediastre...
Source/modules/mediastream/MediaDevicesRequest.h:44: class MediaDevicesRequest
FINAL : public RefCountedWillBeGarbageCollected<MediaDevicesRequest>, public
ActiveDOMObject {
On 2014/02/20 15:30:23, sof wrote:
> Add *Finalized

Done.

https://codereview.chromium.org/173363002/diff/50001/Source/modules/mediastre...
File Source/modules/mediastream/MediaStreamTrackSourcesCallback.h (right):

https://codereview.chromium.org/173363002/diff/50001/Source/modules/mediastre...
Source/modules/mediastream/MediaStreamTrackSourcesCallback.h:37: virtual void
handleEvent(WillBeHeapVector<RefPtrWillBeMember<SourceInfo> > sources) = 0;
On 2014/02/20 15:30:23, sof wrote:
> Would passing it by (const) reference still work?

It won't match the function generated from the idl.

https://codereview.chromium.org/173363002/diff/50001/Source/modules/mediastre...
File Source/modules/mediastream/NavigatorUserMediaError.h (right):

https://codereview.chromium.org/173363002/diff/50001/Source/modules/mediastre...
Source/modules/mediastream/NavigatorUserMediaError.h:35: class
NavigatorUserMediaError FINAL : public
RefCountedWillBeGarbageCollected<NavigatorUserMediaError>, public
ScriptWrappable {
On 2014/02/20 15:30:23, sof wrote:
> Add *Finalized

Done.

https://codereview.chromium.org/173363002/diff/50001/Source/modules/mediastre...
File Source/modules/mediastream/RTCIceCandidate.h (right):

https://codereview.chromium.org/173363002/diff/50001/Source/modules/mediastre...
Source/modules/mediastream/RTCIceCandidate.h:46: class RTCIceCandidate FINAL :
public RefCountedWillBeGarbageCollected<RTCIceCandidate>, public ScriptWrappable
{
On 2014/02/20 15:30:23, sof wrote:
> Add *Finalized

Done.

https://codereview.chromium.org/173363002/diff/50001/Source/modules/mediastre...
File Source/modules/mediastream/RTCStatsReport.h (right):

https://codereview.chromium.org/173363002/diff/50001/Source/modules/mediastre...
Source/modules/mediastream/RTCStatsReport.h:38: class RTCStatsReport : public
RefCountedWillBeGarbageCollected<RTCStatsReport>, public ScriptWrappable {
On 2014/02/20 15:30:23, sof wrote:
> Add *Finalized

Done.

https://codereview.chromium.org/173363002/diff/50001/Source/modules/mediastre...
File Source/modules/mediastream/SourceInfo.h (right):

https://codereview.chromium.org/173363002/diff/50001/Source/modules/mediastre...
Source/modules/mediastream/SourceInfo.h:37: class SourceInfo : public
RefCountedWillBeGarbageCollected<SourceInfo>, public ScriptWrappable {
On 2014/02/20 15:30:23, sof wrote:
> RefCountedWillBeGarbageCollectedFinalized

Done.

https://codereview.chromium.org/173363002/diff/50001/Source/modules/mediastre...
File Source/modules/mediastream/UserMediaRequest.h (right):

https://codereview.chromium.org/173363002/diff/50001/Source/modules/mediastre...
Source/modules/mediastream/UserMediaRequest.h:51: class UserMediaRequest FINAL :
public RefCountedWillBeGarbageCollected<UserMediaRequest>, public
ContextLifecycleObserver {
On 2014/02/20 15:30:23, sof wrote:
> I think the observer needs finalization (cf
> LifecycleObserver::~LifecycleObserver()), so add *Finalized here ?

Done.

[haraken, 2014-02-21 10:47:04]

https://codereview.chromium.org/173363002/diff/190001/LayoutTests/fast/dom/sh...
File LayoutTests/fast/dom/shadow/shadowdom-for-input-appearance.html (right):

https://codereview.chromium.org/173363002/diff/190001/LayoutTests/fast/dom/sh...
LayoutTests/fast/dom/shadow/shadowdom-for-input-appearance.html:34:
document.querySelector('input[type=text]').focus();

What is this change for?

https://codereview.chromium.org/173363002/diff/190001/Source/bindings/v8/V8Bi...
File Source/bindings/v8/V8Binding.h (right):

https://codereview.chromium.org/173363002/diff/190001/Source/bindings/v8/V8Bi...
Source/bindings/v8/V8Binding.h:504: WillBeHeapVector<RefPtrWillBeMember<T> >
toRefPtrWillBeMemberNativeArrayUnchecked(v8::Local<v8::Value> v8Value, uint32_t
length, v8::Isolate* isolate, bool* success = 0)

Instead of adding a method for WillBeHeapVector<RefPtrWillBeMember>, you should
add a method for HeapVector<Member>. (I wonder why this CL compiles when the
OILPAN flag is disabled, since we will end up having two identical methods for
Vector<RefPtr> when the OILPAN flag is disabled.)

The point is that we need methods for both Vector<RefPtr> (which we already
have) and HeapVector<Member> (which we need in this CL).

https://codereview.chromium.org/173363002/diff/190001/Source/bindings/v8/V8Bi...
Source/bindings/v8/V8Binding.h:546: WillBeHeapVector<RefPtrWillBeMember<T> >
toRefPtrWillBeMemberNativeArray(v8::Handle<v8::Value> value, int argumentIndex,
v8::Isolate* isolate, bool* success = 0)

Ditto. You can add a method for HeapVector<Member>.

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
File Source/modules/mediastream/MediaDeviceInfo.h (right):

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
Source/modules/mediastream/MediaDeviceInfo.h:55: typedef
WillBePersistentHeapVector<RefPtrWillBeMember<MediaDeviceInfo> >
MediaDeviceInfoVector;

This vector is used only on stack, right? Then you can use
WillBeHeapVector<RefPtrWillBeMember>.

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
File Source/modules/mediastream/MediaDevicesRequest.h (right):

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
Source/modules/mediastream/MediaDevicesRequest.h:44: class MediaDevicesRequest
FINAL : public RefCountedWillBeGarbageCollectedFinalized<MediaDevicesRequest>,
public ActiveDOMObject {

This should be RefCountedWillBeRefCountedGarbageCollected, because
ActiveDOMObject requires ref()/deref(). Probably this CL compiles just because
MediaDevicesRequest doesn't need ActiveDOMObject::ref()/deref() by accident.

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
Source/modules/mediastream/MediaDevicesRequest.h:65: UserMediaController*
m_controller;

This raw pointer looks safe.

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
File Source/modules/mediastream/MediaStreamTrackSourcesCallback.h (right):

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
Source/modules/mediastream/MediaStreamTrackSourcesCallback.h:37: virtual void
handleEvent(WillBeHeapVector<RefPtrWillBeMember<SourceInfo> > sources) = 0;

Let's keep using SourceInfoVector.

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
File Source/modules/mediastream/RTCDataChannel.h (right):

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
Source/modules/mediastream/RTCDataChannel.h:97: ExecutionContext*
m_executionContext;

This raw pointer looks safe because it's cleared in stop().

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
File Source/modules/mediastream/RTCPeerConnection.h (right):

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
Source/modules/mediastream/RTCPeerConnection.h:154: MediaStreamVector
m_remoteStreams;

You need to trace these two HeapVectors.

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
File Source/modules/mediastream/RTCSessionDescription.h (right):

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
Source/modules/mediastream/RTCSessionDescription.h:45: class
RTCSessionDescription FINAL : public
RefCountedWillBeGarbageCollected<RTCSessionDescription>, public ScriptWrappable
{

This should be RefCountedWillBeGarbageCollectedFinalized since it has
ScriptWrappable.

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
File Source/modules/mediastream/SourceInfo.h (right):

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
Source/modules/mediastream/SourceInfo.h:55: typedef
WillBePersistentHeapVector<RefPtrWillBeMember<SourceInfo> > SourceInfoVector;

This is used in MediaStreamTrackSourcesRequest, which is not yet on the heap.
Probably you might want to consider moving MediaStreamTrackSourcesRequest to the
heap and change the vector to WillBeHeapVector<>.

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
File Source/modules/mediastream/UserMediaRequest.h (right):

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
Source/modules/mediastream/UserMediaRequest.h:83: UserMediaController*
m_controller;

This raw pointer looks safe because it's cleared in contextDestroyed().

https://codereview.chromium.org/173363002/diff/190001/Source/web/WebMediaDevi...
File Source/web/WebMediaDevicesRequest.cpp (right):

https://codereview.chromium.org/173363002/diff/190001/Source/web/WebMediaDevi...
Source/web/WebMediaDevicesRequest.cpp:78: MediaDeviceInfoVector
devices(webDevices.size());

We should make this work in OILPAN instead of adding #if branch.

If you need help, you can ask Erik for help :)

[keishi, 2014-02-25 07:45:11]

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
File Source/modules/mediastream/SourceInfo.h (right):

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
Source/modules/mediastream/SourceInfo.h:55: typedef
WillBePersistentHeapVector<RefPtrWillBeMember<SourceInfo> > SourceInfoVector;
On 2014/02/21 10:47:05, haraken wrote:
> 
> This is used in MediaStreamTrackSourcesRequest, which is not yet on the heap.
> Probably you might want to consider moving MediaStreamTrackSourcesRequest to
the
> heap and change the vector to WillBeHeapVector<>.

How do I move a class in platform dir to the heap? I can't include
heap/Handle.h.

[tkent, 2014-02-25 09:04:45]

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
File Source/modules/mediastream/SourceInfo.h (right):

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
Source/modules/mediastream/SourceInfo.h:55: typedef
WillBePersistentHeapVector<RefPtrWillBeMember<SourceInfo> > SourceInfoVector;
On 2014/02/25 07:45:12, keishi1 wrote:
> On 2014/02/21 10:47:05, haraken wrote:
> > 
> > This is used in MediaStreamTrackSourcesRequest, which is not yet on the
heap.
> > Probably you might want to consider moving MediaStreamTrackSourcesRequest to
> the
> > heap and change the vector to WillBeHeapVector<>.
> 
> How do I move a class in platform dir to the heap? I can't include
> heap/Handle.h.

I think we may add "+heap" to platform/DEPS if heap/ThreadState.h doesn't
contain WebCore class names.

[Nils Barth (inactive), 2014-02-26 04:29:47]

Quick not LGTM to prevent build breakage:
this needs to also implement the Python side
(or just the Python side, once we switch).
blink_presubmit will catch this if it's updated, since we now check Python
run-bindings-tests

Ok to land after implement Python and switch to Python in:
IDL compiler: switch build to Python compiler
https://codereview.chromium.org/166333002/

[keishi, 2014-02-26 06:14:58]

https://codereview.chromium.org/173363002/diff/190001/LayoutTests/fast/dom/sh...
File LayoutTests/fast/dom/shadow/shadowdom-for-input-appearance.html (right):

https://codereview.chromium.org/173363002/diff/190001/LayoutTests/fast/dom/sh...
LayoutTests/fast/dom/shadow/shadowdom-for-input-appearance.html:34:
document.querySelector('input[type=text]').focus();
On 2014/02/21 10:47:05, haraken wrote:
> 
> What is this change for?

Got mixed in. Removed.

https://codereview.chromium.org/173363002/diff/190001/Source/bindings/v8/V8Bi...
File Source/bindings/v8/V8Binding.h (right):

https://codereview.chromium.org/173363002/diff/190001/Source/bindings/v8/V8Bi...
Source/bindings/v8/V8Binding.h:504: WillBeHeapVector<RefPtrWillBeMember<T> >
toRefPtrWillBeMemberNativeArrayUnchecked(v8::Local<v8::Value> v8Value, uint32_t
length, v8::Isolate* isolate, bool* success = 0)
On 2014/02/21 10:47:05, haraken wrote:
> 
> Instead of adding a method for WillBeHeapVector<RefPtrWillBeMember>, you
should
> add a method for HeapVector<Member>. (I wonder why this CL compiles when the
> OILPAN flag is disabled, since we will end up having two identical methods for
> Vector<RefPtr> when the OILPAN flag is disabled.)
> 
> The point is that we need methods for both Vector<RefPtr> (which we already
> have) and HeapVector<Member> (which we need in this CL).

If I prepare two versions of toRefPtrNativeArray() I get the error:
error: call to 'toRefPtrNativeArray' is ambiguous

CL https://codereview.chromium.org/178363010

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
File Source/modules/mediastream/MediaDeviceInfo.h (right):

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
Source/modules/mediastream/MediaDeviceInfo.h:55: typedef
WillBePersistentHeapVector<RefPtrWillBeMember<MediaDeviceInfo> >
MediaDeviceInfoVector;
On 2014/02/21 10:47:05, haraken wrote:
> 
> This vector is used only on stack, right? Then you can use
> WillBeHeapVector<RefPtrWillBeMember>.

Done.

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
File Source/modules/mediastream/MediaDevicesRequest.h (right):

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
Source/modules/mediastream/MediaDevicesRequest.h:44: class MediaDevicesRequest
FINAL : public RefCountedWillBeGarbageCollectedFinalized<MediaDevicesRequest>,
public ActiveDOMObject {
On 2014/02/21 10:47:05, haraken wrote:
> 
> This should be RefCountedWillBeRefCountedGarbageCollected, because
> ActiveDOMObject requires ref()/deref(). Probably this CL compiles just because
> MediaDevicesRequest doesn't need ActiveDOMObject::ref()/deref() by accident.

Done.

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
File Source/modules/mediastream/MediaStreamTrackSourcesCallback.h (right):

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
Source/modules/mediastream/MediaStreamTrackSourcesCallback.h:37: virtual void
handleEvent(WillBeHeapVector<RefPtrWillBeMember<SourceInfo> > sources) = 0;
On 2014/02/21 10:47:05, haraken wrote:
> 
> Let's keep using SourceInfoVector.

Done.

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
File Source/modules/mediastream/RTCPeerConnection.h (right):

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
Source/modules/mediastream/RTCPeerConnection.h:154: MediaStreamVector
m_remoteStreams;
On 2014/02/21 10:47:05, haraken wrote:
> 
> You need to trace these two HeapVectors.

Done.

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
File Source/modules/mediastream/RTCSessionDescription.h (right):

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
Source/modules/mediastream/RTCSessionDescription.h:45: class
RTCSessionDescription FINAL : public
RefCountedWillBeGarbageCollected<RTCSessionDescription>, public ScriptWrappable
{
On 2014/02/21 10:47:05, haraken wrote:
> 
> This should be RefCountedWillBeGarbageCollectedFinalized since it has
> ScriptWrappable.

Done.

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
File Source/modules/mediastream/SourceInfo.h (right):

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
Source/modules/mediastream/SourceInfo.h:55: typedef
WillBePersistentHeapVector<RefPtrWillBeMember<SourceInfo> > SourceInfoVector;
On 2014/02/25 09:04:46, tkent wrote:
> On 2014/02/25 07:45:12, keishi1 wrote:
> > On 2014/02/21 10:47:05, haraken wrote:
> > > 
> > > This is used in MediaStreamTrackSourcesRequest, which is not yet on the
> heap.
> > > Probably you might want to consider moving MediaStreamTrackSourcesRequest
to
> > the
> > > heap and change the vector to WillBeHeapVector<>.
> > 
> > How do I move a class in platform dir to the heap? I can't include
> > heap/Handle.h.
> 
> I think we may add "+heap" to platform/DEPS if heap/ThreadState.h doesn't
> contain WebCore class names.

I had to add heap to the dependencies of platform.
And I'm still getting a linking error. Any idea why?

Undefined symbols for architecture i386:
  "WebCore::MediaStreamTrackSourcesRequest::s_gcInfo", referenced from:
      WebCore::GCInfoTrait<WebCore::MediaStreamTrackSourcesRequest>::get() in
libmodules.a(modules.MediaStreamTrackSourcesRequestImpl.o)
ld: symbol(s) not found for architecture i386
clang: error: linker command failed with exit code 1 (use -v to see invocation)
ninja: build stopped: subcommand failed.

[tkent, 2014-02-26 06:22:00]

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
File Source/modules/mediastream/SourceInfo.h (right):

https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
Source/modules/mediastream/SourceInfo.h:55: typedef
WillBePersistentHeapVector<RefPtrWillBeMember<SourceInfo> > SourceInfoVector;
On 2014/02/26 06:14:59, keishi1 wrote:
> On 2014/02/25 09:04:46, tkent wrote:
> > On 2014/02/25 07:45:12, keishi1 wrote:
> > > On 2014/02/21 10:47:05, haraken wrote:
> > > > 
> > > > This is used in MediaStreamTrackSourcesRequest, which is not yet on the
> > heap.
> > > > Probably you might want to consider moving
MediaStreamTrackSourcesRequest
> to
> > > the
> > > > heap and change the vector to WillBeHeapVector<>.
> > > 
> > > How do I move a class in platform dir to the heap? I can't include
> > > heap/Handle.h.
> > 
> > I think we may add "+heap" to platform/DEPS if heap/ThreadState.h doesn't
> > contain WebCore class names.
> 
> I had to add heap to the dependencies of platform.
> And I'm still getting a linking error. Any idea why?
> 
> Undefined symbols for architecture i386:
>   "WebCore::MediaStreamTrackSourcesRequest::s_gcInfo", referenced from:
>       WebCore::GCInfoTrait<WebCore::MediaStreamTrackSourcesRequest>::get() in
> libmodules.a(modules.MediaStreamTrackSourcesRequestImpl.o)
> ld: symbol(s) not found for architecture i386
> clang: error: linker command failed with exit code 1 (use -v to see
invocation)
> ninja: build stopped: subcommand failed.

Adding ../heap/blink_heap.gyp:blink_heap to platform/blink_platform.gyp as a
dependency?

[keishi, 2014-02-26 06:49:11]

On 2014/02/26 06:22:00, tkent wrote:
>
https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
> File Source/modules/mediastream/SourceInfo.h (right):
> 
>
https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
> Source/modules/mediastream/SourceInfo.h:55: typedef
> WillBePersistentHeapVector<RefPtrWillBeMember<SourceInfo> > SourceInfoVector;
> On 2014/02/26 06:14:59, keishi1 wrote:
> > On 2014/02/25 09:04:46, tkent wrote:
> > > On 2014/02/25 07:45:12, keishi1 wrote:
> > > > On 2014/02/21 10:47:05, haraken wrote:
> > > > > 
> > > > > This is used in MediaStreamTrackSourcesRequest, which is not yet on
the
> > > heap.
> > > > > Probably you might want to consider moving
> MediaStreamTrackSourcesRequest
> > to
> > > > the
> > > > > heap and change the vector to WillBeHeapVector<>.
> > > > 
> > > > How do I move a class in platform dir to the heap? I can't include
> > > > heap/Handle.h.
> > > 
> > > I think we may add "+heap" to platform/DEPS if heap/ThreadState.h doesn't
> > > contain WebCore class names.
> > 
> > I had to add heap to the dependencies of platform.
> > And I'm still getting a linking error. Any idea why?
> > 
> > Undefined symbols for architecture i386:
> >   "WebCore::MediaStreamTrackSourcesRequest::s_gcInfo", referenced from:
> >       WebCore::GCInfoTrait<WebCore::MediaStreamTrackSourcesRequest>::get()
in
> > libmodules.a(modules.MediaStreamTrackSourcesRequestImpl.o)
> > ld: symbol(s) not found for architecture i386
> > clang: error: linker command failed with exit code 1 (use -v to see
> invocation)
> > ninja: build stopped: subcommand failed.
> 
> Adding ../heap/blink_heap.gyp:blink_heap to platform/blink_platform.gyp as a
> dependency?

I already did that and that solved a lot of the errors but I still get the error
above.
https://codereview.chromium.org/173363002/diff/320001/Source/platform/blink_p...

I think maybe libmodules.a is lacking a dependency?

[Nils Barth (inactive), 2014-02-26 07:17:53]

Hi Keishi,
Could you please add a test case for any code generator changes?
(Details below.)

https://codereview.chromium.org/173363002/diff/320001/Source/bindings/scripts...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/173363002/diff/320001/Source/bindings/scripts...
Source/bindings/scripts/code_generator_v8.pm:5484: return
"RefPtrWillBeMember<$implClassName>";
Keishi, could you please add a test case?

Since this is "[WillBeGarbageCollected] on another interface"
(that's being used as an interface type: i.e., not on the current interface),
(correct?)
the relevant file is:
bindings/tests/idls/TestObjectPython.idl
...and you can put it after the other [WillBeGarbageCollected] test case.

This will ensure that we get updated test results, and
run-bindings-tests (r-b-t) catches changes.

You'll also need to update the Python CG (r-b-t enforces this),
or you can wait until we switch to Python (today or tomorrow)
and just work there.

Thanks!

[keishi, 2014-02-26 09:48:00]

On 2014/02/26 06:49:11, keishi1 wrote:
> On 2014/02/26 06:22:00, tkent wrote:
> >
>
https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
> > File Source/modules/mediastream/SourceInfo.h (right):
> > 
> >
>
https://codereview.chromium.org/173363002/diff/190001/Source/modules/mediastr...
> > Source/modules/mediastream/SourceInfo.h:55: typedef
> > WillBePersistentHeapVector<RefPtrWillBeMember<SourceInfo> >
SourceInfoVector;
> > On 2014/02/26 06:14:59, keishi1 wrote:
> > > On 2014/02/25 09:04:46, tkent wrote:
> > > > On 2014/02/25 07:45:12, keishi1 wrote:
> > > > > On 2014/02/21 10:47:05, haraken wrote:
> > > > > > 
> > > > > > This is used in MediaStreamTrackSourcesRequest, which is not yet on
> the
> > > > heap.
> > > > > > Probably you might want to consider moving
> > MediaStreamTrackSourcesRequest
> > > to
> > > > > the
> > > > > > heap and change the vector to WillBeHeapVector<>.
> > > > > 
> > > > > How do I move a class in platform dir to the heap? I can't include
> > > > > heap/Handle.h.
> > > > 
> > > > I think we may add "+heap" to platform/DEPS if heap/ThreadState.h
doesn't
> > > > contain WebCore class names.
> > > 
> > > I had to add heap to the dependencies of platform.
> > > And I'm still getting a linking error. Any idea why?
> > > 
> > > Undefined symbols for architecture i386:
> > >   "WebCore::MediaStreamTrackSourcesRequest::s_gcInfo", referenced from:
> > >       WebCore::GCInfoTrait<WebCore::MediaStreamTrackSourcesRequest>::get()
> in
> > > libmodules.a(modules.MediaStreamTrackSourcesRequestImpl.o)
> > > ld: symbol(s) not found for architecture i386
> > > clang: error: linker command failed with exit code 1 (use -v to see
> > invocation)
> > > ninja: build stopped: subcommand failed.
> > 
> > Adding ../heap/blink_heap.gyp:blink_heap to platform/blink_platform.gyp as a
> > dependency?
> 
> I already did that and that solved a lot of the errors but I still get the
error
> above.
>
https://codereview.chromium.org/173363002/diff/320001/Source/platform/blink_p...
> 
> I think maybe libmodules.a is lacking a dependency?

I rebased and the linking error has disappeared.

[haraken, 2014-02-26 13:58:36]

Overall, looks good.

- Please just check that 'RefPtr<X>' is gone from the code base for X that has
RefCountedGarbageCollected. You need to use the grep command since you cannot
rely on compile errors.

- This CL is using typedefs for on-heap collections. Given that the typedef is
not used in many places, we might want to remove the typedef and just use
WillBeHeapVector<RefPtrWillBeMember> directly. As we discussed in
https://codereview.chromium.org/181093003/, typedef makes underlying data
structure obscure and causes unexpected bugs. (I'm sorry I said that we should
use typedefs in previous comments.)

https://codereview.chromium.org/173363002/diff/320001/Source/bindings/scripts...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/173363002/diff/320001/Source/bindings/scripts...
Source/bindings/scripts/code_generator_v8.pm:5633: return
"(toRefPtrWillBeMemberNativeArray<${arrayOrSequenceType},
V8${arrayOrSequenceType}>($value, $argIndexOrZero, $getIsolate))";

Still I don't fully understand why we need toRefPtrWillBeMemberNativeArray() and
toRefPtrNativeArray(). What we need would be toMemberNativeArray() and
toRefPtrNativeArray(), wouldn't it? I think it's enough to have the Member
version and the RefPtr version in order to make both the oilpan build and the
non-oilpan build compilable.

https://codereview.chromium.org/173363002/diff/320001/Source/modules/mediastr...
File Source/modules/mediastream/MediaDeviceInfo.cpp (right):

https://codereview.chromium.org/173363002/diff/320001/Source/modules/mediastr...
Source/modules/mediastream/MediaDeviceInfo.cpp:33:
DEFINE_GC_INFO(MediaDeviceInfo);

You can drop DEFINE_GC_INFO. The same comment for other places in this CL.

https://codereview.chromium.org/173363002/diff/320001/Source/modules/mediastr...
File Source/modules/mediastream/MediaDeviceInfo.h (right):

https://codereview.chromium.org/173363002/diff/320001/Source/modules/mediastr...
Source/modules/mediastream/MediaDeviceInfo.h:38: DECLARE_GC_INFO;

You can drop DECLARE_GC_INFO now. The same comment for other places in this CL.

https://codereview.chromium.org/173363002/diff/320001/Source/modules/mediastr...
File Source/modules/mediastream/MediaDevicesRequest.h (right):

https://codereview.chromium.org/173363002/diff/320001/Source/modules/mediastr...
Source/modules/mediastream/MediaDevicesRequest.h:44: class MediaDevicesRequest
FINAL : public RefCountedWillBeRefCountedGarbageCollected<MediaDevicesRequest>,
public ActiveDOMObject {

Please just confirm that 'RefPtr<MediaDevicesRequest>' is gone from the code
base.

https://codereview.chromium.org/173363002/diff/320001/Source/modules/mediastr...
File Source/modules/mediastream/MediaStream.h (right):

https://codereview.chromium.org/173363002/diff/320001/Source/modules/mediastr...
Source/modules/mediastream/MediaStream.h:43: class MediaStream FINAL : public
RefCountedWillBeRefCountedGarbageCollected<MediaStream>, public ScriptWrappable,
public URLRegistrable, public MediaStreamDescriptorClient, public
EventTargetWithInlineData, public ContextLifecycleObserver {

Ditto. Please just check that 'RefPtr<MediaStream>' is gone.

https://codereview.chromium.org/173363002/diff/320001/Source/modules/mediastr...
File Source/modules/mediastream/MediaStreamTrackSourcesRequestImpl.cpp (right):

https://codereview.chromium.org/173363002/diff/320001/Source/modules/mediastr...
Source/modules/mediastream/MediaStreamTrackSourcesRequestImpl.cpp:74:
visitor->trace(m_protect);

You need to trace a superclass.

https://codereview.chromium.org/173363002/diff/320001/Source/modules/mediastr...
File Source/modules/mediastream/MediaStreamTrackSourcesRequestImpl.h (right):

https://codereview.chromium.org/173363002/diff/320001/Source/modules/mediastr...
Source/modules/mediastream/MediaStreamTrackSourcesRequestImpl.h:52: virtual void
trace(Visitor*);

Add OVERRIDE.

https://codereview.chromium.org/173363002/diff/320001/Source/modules/mediastr...
Source/modules/mediastream/MediaStreamTrackSourcesRequestImpl.h:63:
Member<MediaStreamTrackSourcesRequest> m_protect;

This should be RefPtrWillBeMember.

https://codereview.chromium.org/173363002/diff/320001/Source/modules/mediastr...
File Source/modules/mediastream/RTCStatsResponse.h (right):

https://codereview.chromium.org/173363002/diff/320001/Source/modules/mediastr...
Source/modules/mediastream/RTCStatsResponse.h:42: const
WillBePersistentHeapVector<RefPtrWillBeMember<RTCStatsReport> >& result() const
{ return m_result; }

We should return WillBeHeapVector<RefPtrWillBeMember<RTCStatsReport>.

I suggested to Slava that we can add operator* that retrieves HeapVector from
PersistentHeapVector: https://codereview.chromium.org/180003002/

https://codereview.chromium.org/173363002/diff/320001/Source/modules/mediastr...
Source/modules/mediastream/RTCStatsResponse.h:42: const
WillBePersistentHeapVector<RefPtrWillBeMember<RTCStatsReport> >& result() const
{ return m_result; }

Another way to fix this would be just to move RTCStatsResponseBase to the heap,
since now you can move platform objects to the heap. Then you can just use
WillBeHeapVector<RefPtrWillBeMember>.

[keishi, 2014-02-27 07:33:34]

https://codereview.chromium.org/173363002/diff/320001/Source/bindings/scripts...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/173363002/diff/320001/Source/bindings/scripts...
Source/bindings/scripts/code_generator_v8.pm:5484: return
"RefPtrWillBeMember<$implClassName>";
On 2014/02/26 07:17:54, Nils Barth wrote:
> Keishi, could you please add a test case?
> 
> Since this is "[WillBeGarbageCollected] on another interface"
> (that's being used as an interface type: i.e., not on the current interface),
> (correct?)
> the relevant file is:
> bindings/tests/idls/TestObjectPython.idl
> ...and you can put it after the other [WillBeGarbageCollected] test case.
> 
> This will ensure that we get updated test results, and
> run-bindings-tests (r-b-t) catches changes.
> 
> You'll also need to update the Python CG (r-b-t enforces this),
> or you can wait until we switch to Python (today or tomorrow)
> and just work there.
> 
> Thanks!

I'm not sure if it is right but I've added a test.

https://codereview.chromium.org/173363002/diff/320001/Source/bindings/scripts...
Source/bindings/scripts/code_generator_v8.pm:5633: return
"(toRefPtrWillBeMemberNativeArray<${arrayOrSequenceType},
V8${arrayOrSequenceType}>($value, $argIndexOrZero, $getIsolate))";
On 2014/02/26 13:58:37, haraken wrote:
> 
> Still I don't fully understand why we need toRefPtrWillBeMemberNativeArray()
and
> toRefPtrNativeArray(). What we need would be toMemberNativeArray() and
> toRefPtrNativeArray(), wouldn't it? I think it's enough to have the Member
> version and the RefPtr version in order to make both the oilpan build and the
> non-oilpan build compilable.

I misunderstood. Done.

https://codereview.chromium.org/173363002/diff/320001/Source/modules/mediastr...
File Source/modules/mediastream/MediaDeviceInfo.cpp (right):

https://codereview.chromium.org/173363002/diff/320001/Source/modules/mediastr...
Source/modules/mediastream/MediaDeviceInfo.cpp:33:
DEFINE_GC_INFO(MediaDeviceInfo);
On 2014/02/26 13:58:37, haraken wrote:
> 
> You can drop DEFINE_GC_INFO. The same comment for other places in this CL.

Done.

https://codereview.chromium.org/173363002/diff/320001/Source/modules/mediastr...
File Source/modules/mediastream/MediaDeviceInfo.h (right):

https://codereview.chromium.org/173363002/diff/320001/Source/modules/mediastr...
Source/modules/mediastream/MediaDeviceInfo.h:38: DECLARE_GC_INFO;
On 2014/02/26 13:58:37, haraken wrote:
> 
> You can drop DECLARE_GC_INFO now. The same comment for other places in this
CL.

Done.

https://codereview.chromium.org/173363002/diff/320001/Source/modules/mediastr...
File Source/modules/mediastream/MediaDevicesRequest.h (right):

https://codereview.chromium.org/173363002/diff/320001/Source/modules/mediastr...
Source/modules/mediastream/MediaDevicesRequest.h:44: class MediaDevicesRequest
FINAL : public RefCountedWillBeRefCountedGarbageCollected<MediaDevicesRequest>,
public ActiveDOMObject {
On 2014/02/26 13:58:37, haraken wrote:
> 
> Please just confirm that 'RefPtr<MediaDevicesRequest>' is gone from the code
> base.

Confirmed.

https://codereview.chromium.org/173363002/diff/320001/Source/modules/mediastr...
File Source/modules/mediastream/MediaStreamTrackSourcesRequestImpl.cpp (right):

https://codereview.chromium.org/173363002/diff/320001/Source/modules/mediastr...
Source/modules/mediastream/MediaStreamTrackSourcesRequestImpl.cpp:74:
visitor->trace(m_protect);
On 2014/02/26 13:58:37, haraken wrote:
> 
> You need to trace a superclass.

Done.

https://codereview.chromium.org/173363002/diff/320001/Source/modules/mediastr...
File Source/modules/mediastream/MediaStreamTrackSourcesRequestImpl.h (right):

https://codereview.chromium.org/173363002/diff/320001/Source/modules/mediastr...
Source/modules/mediastream/MediaStreamTrackSourcesRequestImpl.h:52: virtual void
trace(Visitor*);
On 2014/02/26 13:58:37, haraken wrote:
> 
> Add OVERRIDE.

Done.

https://codereview.chromium.org/173363002/diff/320001/Source/modules/mediastr...
Source/modules/mediastream/MediaStreamTrackSourcesRequestImpl.h:63:
Member<MediaStreamTrackSourcesRequest> m_protect;
On 2014/02/26 13:58:37, haraken wrote:
> 
> This should be RefPtrWillBeMember.

Done.

https://codereview.chromium.org/173363002/diff/320001/Source/modules/mediastr...
File Source/modules/mediastream/RTCStatsResponse.h (right):

https://codereview.chromium.org/173363002/diff/320001/Source/modules/mediastr...
Source/modules/mediastream/RTCStatsResponse.h:42: const
WillBePersistentHeapVector<RefPtrWillBeMember<RTCStatsReport> >& result() const
{ return m_result; }
On 2014/02/26 13:58:37, haraken wrote:
> 
> Another way to fix this would be just to move RTCStatsResponseBase to the
heap,
> since now you can move platform objects to the heap. Then you can just use
> WillBeHeapVector<RefPtrWillBeMember>.

Done.

[Nils Barth (inactive), 2014-02-27 11:20:52]

Thanks for adding a test case and Python code!
(I'll take a careful look later.)

The compiler's switched to Python as of r167983,
and once this CL lands:
IDL compiler: delete Perl compiler, remove unstable/ directory
https://codereview.chromium.org/181513006/
...there won't be the Perl CG anymore, so you'll be able to delete those.

https://codereview.chromium.org/173363002/diff/320001/Source/bindings/scripts...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/173363002/diff/320001/Source/bindings/scripts...
Source/bindings/scripts/code_generator_v8.pm:5484: return
"RefPtrWillBeMember<$implClassName>";
On 2014/02/27 07:33:34, keishi1 wrote:
> On 2014/02/26 07:17:54, Nils Barth wrote:
> > Keishi, could you please add a test case?
> > 
> > Since this is "[WillBeGarbageCollected] on another interface"
> > (that's being used as an interface type: i.e., not on the current
interface),
> > (correct?)
> > the relevant file is:
> > bindings/tests/idls/TestObjectPython.idl
> > ...and you can put it after the other [WillBeGarbageCollected] test case.
> > 
> > This will ensure that we get updated test results, and
> > run-bindings-tests (r-b-t) catches changes.
> > 
> > You'll also need to update the Python CG (r-b-t enforces this),
> > or you can wait until we switch to Python (today or tomorrow)
> > and just work there.
> > 
> > Thanks!
> 
> I'm not sure if it is right but I've added a test.

Thanks, looks generally ok;
I'll take a closer look later.

https://codereview.chromium.org/173363002/diff/340001/Source/bindings/tests/i...
File Source/bindings/tests/idls/TestObjectPython.idl (right):

https://codereview.chromium.org/173363002/diff/340001/Source/bindings/tests/i...
Source/bindings/tests/idls/TestObjectPython.idl:443: void
voidMethodSequenceTestInterfaceWillBeGarbageCollectedArg(sequence<TestInterfaceWillBeGarbageCollected>
sequenceTestInterfaceWillBeGarbageCollectedArg);
Could you put ' // [WillBeGarbageCollected]' at the end for consistency?

[haraken, 2014-02-27 15:32:42]

Almost LG!

I'd like to have one more reviewer on this CL. I'm not confident about the
correctness since I find new issues every time I review this CL :-)

https://codereview.chromium.org/173363002/diff/340001/Source/bindings/scripts...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/173363002/diff/340001/Source/bindings/scripts...
Source/bindings/scripts/code_generator_v8.pm:5443: my $isMember = shift;

The Perl generator is already gone, so you can remove this file.

https://codereview.chromium.org/173363002/diff/340001/Source/bindings/scripts...
File Source/bindings/scripts/unstable/v8_types.py (right):

https://codereview.chromium.org/173363002/diff/340001/Source/bindings/scripts...
Source/bindings/scripts/unstable/v8_types.py:251: def cpp_type(idl_type,
extended_attributes=None, used_as_argument=False, used_as_member=False):

Is |used_as_member| necessary?

https://codereview.chromium.org/173363002/diff/340001/Source/bindings/scripts...
Source/bindings/scripts/unstable/v8_types.py:287: return
cpp_template_type('WillBeHeapVector', cpp_type(this_array_or_sequence_type,
used_as_member=True))

I don't quite understand that you're adding |used_as_member| to cpp_type() but
passing |used_as_member| to cpp_template_type().

https://codereview.chromium.org/173363002/diff/340001/Source/bindings/v8/V8Bi...
File Source/bindings/v8/V8Binding.h (right):

https://codereview.chromium.org/173363002/diff/340001/Source/bindings/v8/V8Bi...
Source/bindings/v8/V8Binding.h:479: Vector<RefPtr<T> > result;

Would you add:

  if (success)
    *success = true;

?

https://codereview.chromium.org/173363002/diff/340001/Source/bindings/v8/V8Bi...
Source/bindings/v8/V8Binding.h:501: HeapVector<Member<T> > result;

Ditto.

https://codereview.chromium.org/173363002/diff/340001/Source/modules/mediastr...
File Source/modules/mediastream/MediaStream.h (right):

https://codereview.chromium.org/173363002/diff/340001/Source/modules/mediastr...
Source/modules/mediastream/MediaStream.h:102: MediaStreamTrackVector
m_audioTracks;

You need to trace |m_audioTracks|. Typedefs make underlying data structure
obscure and lead to trace-miss. Shall we expand MediaStreamTrackVector since the
typedef is just used in a couple of places?

The same comment for other typedefed vectors in this CL.

https://codereview.chromium.org/173363002/diff/340001/Source/modules/mediastr...
Source/modules/mediastream/MediaStream.h:103: MediaStreamTrackVector
m_videoTracks;

You need to trace |m_videoTracks|.

https://codereview.chromium.org/173363002/diff/340001/Source/platform/mediast...
File Source/platform/mediastream/MediaStreamTrackSourcesRequest.cpp (right):

https://codereview.chromium.org/173363002/diff/340001/Source/platform/mediast...
Source/platform/mediastream/MediaStreamTrackSourcesRequest.cpp:31: #include
"config.h"

Remove this file.

https://codereview.chromium.org/173363002/diff/340001/Source/platform/mediast...
File Source/platform/mediastream/RTCStatsResponseBase.h (right):

https://codereview.chromium.org/173363002/diff/340001/Source/platform/mediast...
Source/platform/mediastream/RTCStatsResponseBase.h:34: class
RTCStatsResponseBase : public
RefCountedWillBeGarbageCollected<RTCStatsResponseBase> {

This should be RefCountedWillBeGarbageCollectedFinalized, because
RTCStatsResponse inherits RTCStatsResponseBase and RTCStatsResponse has members
that need a destructor.

https://codereview.chromium.org/173363002/diff/340001/public/platform/WebRTCS...
File public/platform/WebRTCStatsResponse.h (right):

https://codereview.chromium.org/173363002/diff/340001/public/platform/WebRTCS...
public/platform/WebRTCStatsResponse.h:60: BLINK_PLATFORM_EXPORT operator
PassRefPtrWillBeRawPtr<WebCore::RTCStatsResponseBase>() const;

We're removing the implicit converter. Would you instead add getValue() that
does the conversion?

[Tommy Widenflycht, 2014-02-27 15:50:02]

In general how safe is this patch? If there is even small change that it
introduces behavioral changes and/or bugs we (the WebRTC team) would appreciate
if we could have a day to properly test it before it gets committed especially
if it might end up in a dev release.

On 2014/02/27 15:32:42, haraken wrote:
> 
>
https://codereview.chromium.org/173363002/diff/340001/Source/modules/mediastr...
> Source/modules/mediastream/MediaStream.h:102: MediaStreamTrackVector
> m_audioTracks;
> 
> You need to trace |m_audioTracks|. Typedefs make underlying data structure
> obscure and lead to trace-miss. Shall we expand MediaStreamTrackVector since
the
> typedef is just used in a couple of places?
> 
> The same comment for other typedefed vectors in this CL.
> 

This is fine by me

[haraken, 2014-02-27 23:41:40]

Thanks tommy!

> In general how safe is this patch? If there is even small change that it
> introduces behavioral changes and/or bugs we (the WebRTC team) would
appreciate
> if we could have a day to properly test it before it gets committed especially
> if it might end up in a dev release.

When the enable_oilpan flag is off, this CL changes nothing. In trunk, the
enable_oilpan flag is off.

When the enable_oilpan flag is on, this CL changes when media stream objects
die. As far as I see, there is no code that depends on the lifetime in
mediastream/, so this lifetime change won't change any behavior. We will confirm
that this really won't cause any issue in build bots where the enable_oilpan
flag is on. After confirming that it's OK, we're planning to enable oilpan for
mediastream/ in trunk.

Feel free to ping us if you have any concern.

[tkent, 2014-02-28 00:16:29]

https://codereview.chromium.org/173363002/diff/340001/Source/platform/DEPS
File Source/platform/DEPS (right):

https://codereview.chromium.org/173363002/diff/340001/Source/platform/DEPS#ne...
Source/platform/DEPS:9: "+heap",
I don't want to approve this change until we remove WebCore class names from
heap/ThreadState.h.  It's a conceptual layering violation.

[haraken, 2014-02-28 00:22:37]

https://codereview.chromium.org/173363002/diff/340001/Source/platform/DEPS
File Source/platform/DEPS (right):

https://codereview.chromium.org/173363002/diff/340001/Source/platform/DEPS#ne...
Source/platform/DEPS:9: "+heap",
On 2014/02/28 00:16:30, tkent wrote:
> I don't want to approve this change until we remove WebCore class names from
> heap/ThreadState.h.  It's a conceptual layering violation.

Although it might be a bit tough to completely avoid WebCore class names from
heap/ (e.g., in the future CL, typed heaps will need to say "Node"), I agree
that we should first land the CL that removes USED_FROM_MULTIPLE_THREADS.

[sof, 2014-03-02 20:10:53]

Re: heap vector support in the IDL compiler -- the handling of variadic methods
also needs to be changed, but perhaps not here.

(If this CL is delayed a bit, it'd be perhaps be an idea to split out the IDL
compiler-specific changes? Would be valuable to have this land sooner, as other
interfaces are running into the lack of heap vector support also.)

https://codereview.chromium.org/173363002/diff/340001/Source/bindings/scripts...
File Source/bindings/scripts/unstable/v8_types.py (right):

https://codereview.chromium.org/173363002/diff/340001/Source/bindings/scripts...
Source/bindings/scripts/unstable/v8_types.py:478: expression_format =
'(toMemberNativeArray<{array_or_sequence_type},
V8{array_or_sequence_type}>({v8_value}, {index}, info.GetIsolate()))'
To my eyes, it would read better if you instead created a local binding
(to_native_array_function, or something like that) which was bound to
'toMemberNativeArray' or 'torRefPtrNativeArray' & substituted that in here. Less
duplication.

(This comment applies to the other special cases added here.)

[Nils Barth (inactive), 2014-03-03 02:47:52]

Hi Keishi,
Apologies for delayed review.
From the CG, changes look mostly fine.

Changes to make:
* Rebase (remove Perl CG changes)
* Add callback test cases
* A minor style change
* More comments
(All indicated below.)

Once these are fixed, I'll be ok from CG POV,
though haraken had some questions.

https://codereview.chromium.org/173363002/diff/320001/Source/bindings/scripts...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/173363002/diff/320001/Source/bindings/scripts...
Source/bindings/scripts/code_generator_v8.pm:5484: return
"RefPtrWillBeMember<$implClassName>";
On 2014/02/27 07:33:34, keishi1 wrote:
> I'm not sure if it is right but I've added a test.

Current test case looks ok, but you need one more for callback interfaces.
To check test coverage, first look at changes to output,
and see that all your new code appears.

Currently this is:
https://codereview.chromium.org/173363002/diff/340001/Source/bindings/tests/r...
...and it does include 'WillBeHeapVector' and 'toRefPtrWillBeMemberNativeArray'.

(Also, this gives full test coverage of v8_types.py;
to get this output the code needs to cover all your new code,
and you're just adding a test, without changing existing output,
so this gives full test coverage.)

However, since you're also changing v8_callback_interface.py,
you need to add a test case to TestCallbackInterface.idl.

Could you put the following at the end of TestCallbackInterface.idl ?
(Same test case as you put in TestObjectPython.idl, but different
behavior for callback interfaces.)

void
voidMethodSequenceTestInterfaceWillBeGarbageCollectedArg(sequence<TestInterfaceWillBeGarbageCollected>
sequenceTestInterfaceWillBeGarbageCollectedArg); // [WillBeGarbageCollected]

Thanks!

https://codereview.chromium.org/173363002/diff/340001/Source/bindings/scripts...
File Source/bindings/scripts/unstable/v8_callback_interface.py (right):

https://codereview.chromium.org/173363002/diff/340001/Source/bindings/scripts...
Source/bindings/scripts/unstable/v8_callback_interface.py:76: if
usual_cpp_type.startswith('Vector') or
usual_cpp_type.startswith('WillBeHeapVector'):
You can use a tuple in startswith, as:
if usual_cpp_type.startswith(('Vector', 'WillBeHeapVector')):

Ref:
http://docs.python.org/2/library/stdtypes.html#str.startswith

https://codereview.chromium.org/173363002/diff/340001/Source/bindings/scripts...
File Source/bindings/scripts/unstable/v8_types.py (right):

https://codereview.chromium.org/173363002/diff/340001/Source/bindings/scripts...
Source/bindings/scripts/unstable/v8_types.py:251: def cpp_type(idl_type,
extended_attributes=None, used_as_argument=False, used_as_member=False):
On 2014/02/27 15:32:43, haraken wrote:
> Is |used_as_member| necessary?

Keishi, could you add some documentation to the docstring to explain
used_as_member?

Format (and suggested contents) is:

Args:
    used_as_member: bool, is idl_type a member of an array or sequence? (in C++
represented as a vector)

Reference:
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Commen...

https://codereview.chromium.org/173363002/diff/340001/Source/bindings/scripts...
Source/bindings/scripts/unstable/v8_types.py:287: return
cpp_template_type('WillBeHeapVector', cpp_type(this_array_or_sequence_type,
used_as_member=True))
On 2014/02/27 15:32:43, haraken wrote:
> I don't quite understand that you're adding |used_as_member| to cpp_type() but
> passing |used_as_member| to cpp_template_type().

? Keishi's code makes sense AFAICT;
it's using |used_as_member| in cpp_type to select with template to use;
it's not passed to cpp_template_type(), which just fills in "Template<Class>"

[Nils Barth (inactive), 2014-03-03 02:55:06]

On 2014/03/02 20:10:53, sof wrote:
> Re: heap vector support in the IDL compiler -- the handling of variadic
methods
> also needs to be changed, but perhaps not here.

That's a good point -- thanks Sigbjørn!

> (If this CL is delayed a bit, it'd be perhaps be an idea to split out the IDL
> compiler-specific changes? Would be valuable to have this land sooner, as
other
> interfaces are running into the lack of heap vector support also.)

As a rule we like to add features to the compiler when there's an actual use
case,
both to ensure it's being used, and to make the history clearer (i.e., to show
in
a real Blink use context, not just a test case).

So if other interfaces need heap vectors, feel free to grab the CG changes and
commit in a CL that lands faster (that'll just simplify this one).

Sounds sane?

[sof, 2014-03-05 07:35:23]

On 2014/03/03 02:55:06, Nils Barth wrote:
....
> So if other interfaces need heap vectors, feel free to grab the CG changes and
> commit in a CL that lands faster (that'll just simplify this one).
> 
> Sounds sane?

ok, i needed those changes for https://codereview.chromium.org/185413023/ , so
brought them in there.

[Nils Barth (inactive), 2014-03-05 07:52:14]

On 2014/03/05 07:35:23, sof wrote:
> On 2014/03/03 02:55:06, Nils Barth wrote:
> ....
> > So if other interfaces need heap vectors, feel free to grab the CG changes
and
> > commit in a CL that lands faster (that'll just simplify this one).
> > 
> > Sounds sane?
> 
> ok, i needed those changes for https://codereview.chromium.org/185413023/ , so
> brought them in there.

Thanks, that's exactly what I had in mind!
(...and thanks for updating here.)

[sof, 2014-03-06 14:29:02]

On 2014/03/05 07:52:14, Nils Barth wrote:
> On 2014/03/05 07:35:23, sof wrote:
> > On 2014/03/03 02:55:06, Nils Barth wrote:
> > ....
> > > So if other interfaces need heap vectors, feel free to grab the CG changes
> and
> > > commit in a CL that lands faster (that'll just simplify this one).
> > > 
> > > Sounds sane?
> > 
> > ok, i needed those changes for https://codereview.chromium.org/185413023/ ,
so
> > brought them in there.
> 
> Thanks, that's exactly what I had in mind!
> (...and thanks for updating here.)

Landed as r168607.

[haraken, 2014-03-28 01:06:27]

What's the current status of this CL? Is moving mediastream/ to oilpan blocked
by something?

[keishi, 2014-04-07 09:06:51]

Rebased.

I get the error when I disable oilpan. How should this be handled? haraken
previously said he doesn't want toRefPtrWillBeMemberNativeArray but I don't
think we can switch toRefPtrNativeArray and toMemberNativeArray depending on
ENABLE_OILPAN without it.

gen/blink/bindings/V8MediaStream.cpp:320:87: error: use of undeclared identifier
'toRefPtrWillBeMemberNativeArray'
    V8TRYCATCH_VOID(WillBeHeapVector<RefPtrWillBeMember<MediaStreamTrack> >,
tracks, (toRefPtrWillBeMemberNativeArray<MediaStreamTrack,
V8MediaStreamTrack>(info[0], 1, info.GetIsolate())));

https://codereview.chromium.org/173363002/diff/340001/Source/modules/mediastr...
File Source/modules/mediastream/MediaStream.h (right):

https://codereview.chromium.org/173363002/diff/340001/Source/modules/mediastr...
Source/modules/mediastream/MediaStream.h:102: MediaStreamTrackVector
m_audioTracks;
On 2014/02/27 15:32:43, haraken wrote:
> 
> You need to trace |m_audioTracks|. Typedefs make underlying data structure
> obscure and lead to trace-miss. Shall we expand MediaStreamTrackVector since
the
> typedef is just used in a couple of places?
> 
> The same comment for other typedefed vectors in this CL.

Done.

https://codereview.chromium.org/173363002/diff/340001/Source/modules/mediastr...
Source/modules/mediastream/MediaStream.h:103: MediaStreamTrackVector
m_videoTracks;
On 2014/02/27 15:32:43, haraken wrote:
> 
> You need to trace |m_videoTracks|.

Done.

https://codereview.chromium.org/173363002/diff/340001/Source/platform/mediast...
File Source/platform/mediastream/MediaStreamTrackSourcesRequest.cpp (right):

https://codereview.chromium.org/173363002/diff/340001/Source/platform/mediast...
Source/platform/mediastream/MediaStreamTrackSourcesRequest.cpp:31: #include
"config.h"
On 2014/02/27 15:32:43, haraken wrote:
> 
> Remove this file.

Done.

https://codereview.chromium.org/173363002/diff/340001/Source/platform/mediast...
File Source/platform/mediastream/RTCStatsResponseBase.h (right):

https://codereview.chromium.org/173363002/diff/340001/Source/platform/mediast...
Source/platform/mediastream/RTCStatsResponseBase.h:34: class
RTCStatsResponseBase : public
RefCountedWillBeGarbageCollected<RTCStatsResponseBase> {
On 2014/02/27 15:32:43, haraken wrote:
> 
> This should be RefCountedWillBeGarbageCollectedFinalized, because
> RTCStatsResponse inherits RTCStatsResponseBase and RTCStatsResponse has
members
> that need a destructor.

Done.

https://codereview.chromium.org/173363002/diff/340001/public/platform/WebRTCS...
File public/platform/WebRTCStatsResponse.h (right):

https://codereview.chromium.org/173363002/diff/340001/public/platform/WebRTCS...
public/platform/WebRTCStatsResponse.h:60: BLINK_PLATFORM_EXPORT operator
PassRefPtrWillBeRawPtr<WebCore::RTCStatsResponseBase>() const;
On 2014/02/27 15:32:43, haraken wrote:
> 
> We're removing the implicit converter. Would you instead add getValue() that
> does the conversion?

Done.

[haraken, 2014-04-07 09:17:59]

On 2014/04/07 09:06:51, keishi1 wrote:
> Rebased.
> 
> I get the error when I disable oilpan. How should this be handled? haraken
> previously said he doesn't want toRefPtrWillBeMemberNativeArray but I don't
> think we can switch toRefPtrNativeArray and toMemberNativeArray depending on
> ENABLE_OILPAN without it.
> 
> gen/blink/bindings/V8MediaStream.cpp:320:87: error: use of undeclared
identifier
> 'toRefPtrWillBeMemberNativeArray'
>     V8TRYCATCH_VOID(WillBeHeapVector<RefPtrWillBeMember<MediaStreamTrack> >,
> tracks, (toRefPtrWillBeMemberNativeArray<MediaStreamTrack,
> V8MediaStreamTrack>(info[0], 1, info.GetIsolate())));

ah sorry, now I understand the issue. It makes sense to add
toRefPtrWillBeMemberNativeArray.

[sof, 2014-04-07 14:22:16]

I didn't go through the previous feedback, so my apologies if there is overlap
with issues already raised (but not yet taken into account.)

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
File Source/modules/mediastream/MediaDeviceInfo.h (right):

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
Source/modules/mediastream/MediaDeviceInfo.h:37: class MediaDeviceInfo : public
RefCountedWillBeGarbageCollectedFinalized<MediaDeviceInfo> {
Add FINAL?

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
File Source/modules/mediastream/MediaStream.cpp (right):

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
Source/modules/mediastream/MediaStream.cpp:260: RefPtrWillBeRawPtr<MediaStream>
clonedStream = MediaStream::create(context, tracks);
No need to bind this to 'clonedStream' before returning, right?

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
File Source/modules/mediastream/RTCPeerConnection.cpp (right):

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
Source/modules/mediastream/RTCPeerConnection.cpp:211: stop();
Is this dtor call to stop() problematic?

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
Source/modules/mediastream/RTCPeerConnection.cpp:265:
RefPtrWillBeRawPtr<RTCSessionDescription> sessionDescription =
RTCSessionDescription::create(webSessionDescription);
Just return the result here also?

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
Source/modules/mediastream/RTCPeerConnection.cpp:290:
RefPtrWillBeRawPtr<RTCSessionDescription> desc =
RTCSessionDescription::create(webSessionDescription);
It would look tidier just to return this straight.

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
File Source/modules/mediastream/RTCStatsReport.h (right):

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
Source/modules/mediastream/RTCStatsReport.h:38: class RTCStatsReport : public
RefCountedWillBeGarbageCollectedFinalized<RTCStatsReport>, public
ScriptWrappable {
Add FINAL?

https://codereview.chromium.org/173363002/diff/420001/Source/platform/exporte...
File Source/platform/exported/WebRTCStatsResponse.cpp (right):

https://codereview.chromium.org/173363002/diff/420001/Source/platform/exporte...
Source/platform/exported/WebRTCStatsResponse.cpp:51:
PassRefPtrWillBeRawPtr<WebCore::RTCStatsResponseBase>
WebRTCStatsResponse::getValue() const
I think it was decided last week to stick with an explicit operator here instead
of getValue(); can you sync wrt that?

https://codereview.chromium.org/173363002/diff/420001/Source/platform/mediast...
File Source/platform/mediastream/MediaStreamCenter.h (right):

https://codereview.chromium.org/173363002/diff/420001/Source/platform/mediast...
Source/platform/mediastream/MediaStreamCenter.h:34: #include "heap/Handle.h"
platform/heap/Handle.h

https://codereview.chromium.org/173363002/diff/420001/Source/platform/mediast...
File Source/platform/mediastream/MediaStreamTrackSourcesRequest.h (right):

https://codereview.chromium.org/173363002/diff/420001/Source/platform/mediast...
Source/platform/mediastream/MediaStreamTrackSourcesRequest.h:29: #include
"heap/Handle.h"
platform/heap/Handle.h

https://codereview.chromium.org/173363002/diff/420001/Source/platform/mediast...
File Source/platform/mediastream/RTCStatsResponseBase.h (right):

https://codereview.chromium.org/173363002/diff/420001/Source/platform/mediast...
Source/platform/mediastream/RTCStatsResponseBase.h:41: virtual void
trace(Visitor*) { }
Worth considering making this a pure virtual ?

https://codereview.chromium.org/173363002/diff/420001/public/platform/WebRTCS...
File public/platform/WebRTCStatsRequest.h (right):

https://codereview.chromium.org/173363002/diff/420001/public/platform/WebRTCS...
public/platform/WebRTCStatsRequest.h:98: BLINK_PLATFORM_EXPORT
WebRTCStatsRequest(const PassRefPtr<WebCore::RTCStatsRequest>&);
Keep this as-is if RTCStatsRequest won't derive from a GCed class?

https://codereview.chromium.org/173363002/diff/420001/public/platform/WebRTCS...
File public/platform/WebRTCStatsResponse.h (right):

https://codereview.chromium.org/173363002/diff/420001/public/platform/WebRTCS...
public/platform/WebRTCStatsResponse.h:60: BLINK_PLATFORM_EXPORT
PassRefPtrWillBeRawPtr<WebCore::RTCStatsResponseBase> getValue() const;
Switch away from getValue() here also?

[haraken, 2014-04-08 04:42:34]

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
File Source/modules/mediastream/MediaDeviceInfo.h (right):

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
Source/modules/mediastream/MediaDeviceInfo.h:37: class MediaDeviceInfo : public
RefCountedWillBeGarbageCollectedFinalized<MediaDeviceInfo> {

Can this be RefCountedWillBeGarbageCollected ?

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
File Source/modules/mediastream/MediaDevicesRequest.h (right):

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
Source/modules/mediastream/MediaDevicesRequest.h:64: UserMediaController*
m_controller;

This raw pointer looks safe since it's cleared in ActiveDOMObject::stop().

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
File Source/modules/mediastream/MediaStream.cpp (right):

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
Source/modules/mediastream/MediaStream.cpp:155: (*iter)->removeObserver(this);

You cannot touch the track vector because the vector members are on-heap. I
think you need to make MediaStreamTrack::m_observers a vector of weak members.
Then you can remove the removeObserver code from MediaStream's destructor.

I think if you run layout tests in ASAN build with oilpan enabled, ASAN will
detect this kind of error.

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
Source/modules/mediastream/MediaStream.cpp:158: (*iter)->removeObserver(this);

Ditto. You need to remove this code by making MediaStreamTrack::m_observers a
vector of weak members.

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
File Source/modules/mediastream/MediaStreamTrack.h (right):

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
Source/modules/mediastream/MediaStreamTrack.h:103: Vector<Observer*>
m_observers;

As I commented in MediaStream.h, this should be a vector of weak members.

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
File Source/modules/mediastream/RTCPeerConnection.cpp (right):

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
Source/modules/mediastream/RTCPeerConnection.cpp:211: stop();

This is problematic because stop() touches m_dataChannels which are on heap.

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
File Source/modules/mediastream/UserMediaRequest.h (right):

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
Source/modules/mediastream/UserMediaRequest.h:83: UserMediaController*
m_controller;

This raw pointer looks safe since it's cleared in contextDestroyed().

https://codereview.chromium.org/173363002/diff/420001/public/platform/WebRTCS...
File public/platform/WebRTCStatsRequest.h (right):

https://codereview.chromium.org/173363002/diff/420001/public/platform/WebRTCS...
public/platform/WebRTCStatsRequest.h:98: BLINK_PLATFORM_EXPORT
WebRTCStatsRequest(const PassRefPtr<WebCore::RTCStatsRequest>&);
On 2014/04/07 14:22:16, sof wrote:
> Keep this as-is if RTCStatsRequest won't derive from a GCed class?

I wonder why this complies without the WTF:: namespace :)

https://codereview.chromium.org/173363002/diff/420001/public/platform/WebRTCS...
File public/platform/WebRTCStatsResponse.h (right):

https://codereview.chromium.org/173363002/diff/420001/public/platform/WebRTCS...
public/platform/WebRTCStatsResponse.h:60: BLINK_PLATFORM_EXPORT
PassRefPtrWillBeRawPtr<WebCore::RTCStatsResponseBase> getValue() const;
On 2014/04/07 14:22:16, sof wrote:
> Switch away from getValue() here also?

Yeah, sorry. Probably I mentioned in a previous review that we prefer getValue()
than the implicit value converter, but we changed our mind because we found that
having the implicit value converter is no problem in the latest oilpan's
infrastructure.

So let's keep the implicit value converter. Sorry about that.

[haraken, 2014-04-08 04:44:30]

This CL is huge and a bit hard to verify correctness. I'm OK with handling this
CL as is, but let's consider splitting this kind of big CL into pieces from next
time unless there is any particular reason you have to land it in one go.

[keishi, 2014-04-11 20:03:05]

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
File Source/modules/mediastream/MediaDeviceInfo.h (right):

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
Source/modules/mediastream/MediaDeviceInfo.h:37: class MediaDeviceInfo : public
RefCountedWillBeGarbageCollectedFinalized<MediaDeviceInfo> {
On 2014/04/07 14:22:16, sof wrote:
> Add FINAL?

Done.

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
Source/modules/mediastream/MediaDeviceInfo.h:37: class MediaDeviceInfo : public
RefCountedWillBeGarbageCollectedFinalized<MediaDeviceInfo> {
On 2014/04/08 04:42:35, haraken wrote:
> 
> Can this be RefCountedWillBeGarbageCollected ?

gc plugin gives an error.

../../third_party/WebKit/Source/modules/mediastream/MediaDeviceInfo.h:37:1:
error: [blink-gc] Class 'MediaDeviceInfo' requires finalization.
class MediaDeviceInfo FINAL : public
RefCountedWillBeGarbageCollected<MediaDeviceInfo> {
^
../../third_party/WebKit/Source/modules/mediastream/MediaDeviceInfo.h:51:5:
note: [blink-gc] Field 'm_webMediaDeviceInfo' requiring finalization declared
here:
    blink::WebMediaDeviceInfo m_webMediaDeviceInfo;
    ^
1 error generated.

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
File Source/modules/mediastream/MediaStream.cpp (right):

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
Source/modules/mediastream/MediaStream.cpp:155: (*iter)->removeObserver(this);
On 2014/04/08 04:42:35, haraken wrote:
> 
> You cannot touch the track vector because the vector members are on-heap. I
> think you need to make MediaStreamTrack::m_observers a vector of weak members.
> Then you can remove the removeObserver code from MediaStream's destructor.
> 
> I think if you run layout tests in ASAN build with oilpan enabled, ASAN will
> detect this kind of error.

Done.

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
Source/modules/mediastream/MediaStream.cpp:158: (*iter)->removeObserver(this);
On 2014/04/08 04:42:35, haraken wrote:
> 
> Ditto. You need to remove this code by making MediaStreamTrack::m_observers a
> vector of weak members.

Done.

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
Source/modules/mediastream/MediaStream.cpp:260: RefPtrWillBeRawPtr<MediaStream>
clonedStream = MediaStream::create(context, tracks);
On 2014/04/07 14:22:16, sof wrote:
> No need to bind this to 'clonedStream' before returning, right?

Done.

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
File Source/modules/mediastream/MediaStreamTrack.h (right):

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
Source/modules/mediastream/MediaStreamTrack.h:103: Vector<Observer*>
m_observers;
On 2014/04/08 04:42:35, haraken wrote:
> 
> As I commented in MediaStream.h, this should be a vector of weak members.

I had trouble making MediaStreamTrack::Observer garbage collected so I changed
this to a vector of MediaStreams. I was getting errors like:
 ../../third_party/WebKit/Source/modules/mediastream/MediaStream.cpp:63:53:
error: member 'operator new' found in multiple base classes of different types
    return adoptRefWillBeRefCountedGarbageCollected(new MediaStream(context,
audioTracks, videoTracks));

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
File Source/modules/mediastream/RTCPeerConnection.cpp (right):

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
Source/modules/mediastream/RTCPeerConnection.cpp:211: stop();
On 2014/04/08 04:42:35, haraken wrote:
> 
> This is problematic because stop() touches m_dataChannels which are on heap.

Done.

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
Source/modules/mediastream/RTCPeerConnection.cpp:265:
RefPtrWillBeRawPtr<RTCSessionDescription> sessionDescription =
RTCSessionDescription::create(webSessionDescription);
On 2014/04/07 14:22:16, sof wrote:
> Just return the result here also?

Done.

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
Source/modules/mediastream/RTCPeerConnection.cpp:290:
RefPtrWillBeRawPtr<RTCSessionDescription> desc =
RTCSessionDescription::create(webSessionDescription);
On 2014/04/07 14:22:16, sof wrote:
> It would look tidier just to return this straight.

Done.

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
File Source/modules/mediastream/RTCStatsReport.h (right):

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
Source/modules/mediastream/RTCStatsReport.h:38: class RTCStatsReport : public
RefCountedWillBeGarbageCollectedFinalized<RTCStatsReport>, public
ScriptWrappable {
On 2014/04/07 14:22:16, sof wrote:
> Add FINAL?

Done.

https://codereview.chromium.org/173363002/diff/420001/Source/platform/exporte...
File Source/platform/exported/WebRTCStatsResponse.cpp (right):

https://codereview.chromium.org/173363002/diff/420001/Source/platform/exporte...
Source/platform/exported/WebRTCStatsResponse.cpp:51:
PassRefPtrWillBeRawPtr<WebCore::RTCStatsResponseBase>
WebRTCStatsResponse::getValue() const
On 2014/04/07 14:22:16, sof wrote:
> I think it was decided last week to stick with an explicit operator here
instead
> of getValue(); can you sync wrt that?

Done.

https://codereview.chromium.org/173363002/diff/420001/Source/platform/mediast...
File Source/platform/mediastream/MediaStreamCenter.h (right):

https://codereview.chromium.org/173363002/diff/420001/Source/platform/mediast...
Source/platform/mediastream/MediaStreamCenter.h:34: #include "heap/Handle.h"
On 2014/04/07 14:22:16, sof wrote:
> platform/heap/Handle.h

Done.

https://codereview.chromium.org/173363002/diff/420001/Source/platform/mediast...
File Source/platform/mediastream/MediaStreamTrackSourcesRequest.h (right):

https://codereview.chromium.org/173363002/diff/420001/Source/platform/mediast...
Source/platform/mediastream/MediaStreamTrackSourcesRequest.h:29: #include
"heap/Handle.h"
On 2014/04/07 14:22:16, sof wrote:
> platform/heap/Handle.h

Done.

https://codereview.chromium.org/173363002/diff/420001/Source/platform/mediast...
File Source/platform/mediastream/RTCStatsResponseBase.h (right):

https://codereview.chromium.org/173363002/diff/420001/Source/platform/mediast...
Source/platform/mediastream/RTCStatsResponseBase.h:41: virtual void
trace(Visitor*) { }
On 2014/04/07 14:22:16, sof wrote:
> Worth considering making this a pure virtual ?

Done.

https://codereview.chromium.org/173363002/diff/420001/public/platform/WebRTCS...
File public/platform/WebRTCStatsRequest.h (right):

https://codereview.chromium.org/173363002/diff/420001/public/platform/WebRTCS...
public/platform/WebRTCStatsRequest.h:98: BLINK_PLATFORM_EXPORT
WebRTCStatsRequest(const PassRefPtr<WebCore::RTCStatsRequest>&);
On 2014/04/07 14:22:16, sof wrote:
> Keep this as-is if RTCStatsRequest won't derive from a GCed class?

Done.

https://codereview.chromium.org/173363002/diff/420001/public/platform/WebRTCS...
File public/platform/WebRTCStatsResponse.h (right):

https://codereview.chromium.org/173363002/diff/420001/public/platform/WebRTCS...
public/platform/WebRTCStatsResponse.h:60: BLINK_PLATFORM_EXPORT
PassRefPtrWillBeRawPtr<WebCore::RTCStatsResponseBase> getValue() const;
On 2014/04/07 14:22:16, sof wrote:
> Switch away from getValue() here also?

Done.

[haraken, 2014-04-14 04:42:42]

Double-checked. LGTM.

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
File Source/modules/mediastream/MediaStreamTrack.h (right):

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
Source/modules/mediastream/MediaStreamTrack.h:103: Vector<Observer*>
m_observers;
On 2014/04/11 20:03:06, keishi1 wrote:
> On 2014/04/08 04:42:35, haraken wrote:
> > 
> > As I commented in MediaStream.h, this should be a vector of weak members.
> 
> I had trouble making MediaStreamTrack::Observer garbage collected so I changed
> this to a vector of MediaStreams. I was getting errors like:
>  ../../third_party/WebKit/Source/modules/mediastream/MediaStream.cpp:63:53:
> error: member 'operator new' found in multiple base classes of different types
>     return adoptRefWillBeRefCountedGarbageCollected(new MediaStream(context,
> audioTracks, videoTracks));

Sounds reasonable!

https://codereview.chromium.org/173363002/diff/500001/Source/modules/mediastr...
File Source/modules/mediastream/MediaStreamTrack.h (right):

https://codereview.chromium.org/173363002/diff/500001/Source/modules/mediastr...
Source/modules/mediastream/MediaStreamTrack.h:97:
Vector<RawPtrWillBeWeakMember<MediaStream> > m_registeredMediaStreams;

This should be WillBeHeapVector. And trace it.

[keishi, 2014-04-14 05:56:16]

https://codereview.chromium.org/173363002/diff/500001/Source/modules/mediastr...
File Source/modules/mediastream/MediaStreamTrack.h (right):

https://codereview.chromium.org/173363002/diff/500001/Source/modules/mediastr...
Source/modules/mediastream/MediaStreamTrack.h:97:
Vector<RawPtrWillBeWeakMember<MediaStream> > m_registeredMediaStreams;
On 2014/04/14 04:42:43, haraken wrote:
> 
> This should be WillBeHeapVector. And trace it.

I'm getting build errors because there is a compile assert
WeDontSupportWeaknessInHeapVectorsOrDeques. How should this be handled?

[haraken, 2014-04-14 06:05:48]

https://codereview.chromium.org/173363002/diff/500001/Source/modules/mediastr...
File Source/modules/mediastream/MediaStreamTrack.h (right):

https://codereview.chromium.org/173363002/diff/500001/Source/modules/mediastr...
Source/modules/mediastream/MediaStreamTrack.h:97:
Vector<RawPtrWillBeWeakMember<MediaStream> > m_registeredMediaStreams;
On 2014/04/14 05:56:17, keishi1 wrote:
> On 2014/04/14 04:42:43, haraken wrote:
> > 
> > This should be WillBeHeapVector. And trace it.
> 
> I'm getting build errors because there is a compile assert
> WeDontSupportWeaknessInHeapVectorsOrDeques. How should this be handled?

Yes, we intentionally don't support weakness in vectors, because just clearing
out weakened entries in a vector won't give you expected behavior.

Can you use a WillBeHashSet<RawPtrWillBeWeakMember<MediaStream>> ?

[haraken, 2014-04-14 06:06:31]

On 2014/04/14 06:05:48, haraken wrote:
>
https://codereview.chromium.org/173363002/diff/500001/Source/modules/mediastr...
> File Source/modules/mediastream/MediaStreamTrack.h (right):
> 
>
https://codereview.chromium.org/173363002/diff/500001/Source/modules/mediastr...
> Source/modules/mediastream/MediaStreamTrack.h:97:
> Vector<RawPtrWillBeWeakMember<MediaStream> > m_registeredMediaStreams;
> On 2014/04/14 05:56:17, keishi1 wrote:
> > On 2014/04/14 04:42:43, haraken wrote:
> > > 
> > > This should be WillBeHeapVector. And trace it.
> > 
> > I'm getting build errors because there is a compile assert
> > WeDontSupportWeaknessInHeapVectorsOrDeques. How should this be handled?
> 
> Yes, we intentionally don't support weakness in vectors, because just clearing
> out weakened entries in a vector won't give you expected behavior.
> 
> Can you use a WillBeHashSet<RawPtrWillBeWeakMember<MediaStream>> ?

Correction: WillBeHeapHashSet<RawPtrWillBeWeakMember<MediaStream>>.

[Erik Corry, 2014-04-14 09:10:33]

https://codereview.chromium.org/173363002/diff/500001/Source/modules/mediastr...
File Source/modules/mediastream/MediaStreamTrack.h (right):

https://codereview.chromium.org/173363002/diff/500001/Source/modules/mediastr...
Source/modules/mediastream/MediaStreamTrack.h:97:
Vector<RawPtrWillBeWeakMember<MediaStream> > m_registeredMediaStreams;
On 2014/04/14 05:56:17, keishi1 wrote:
> On 2014/04/14 04:42:43, haraken wrote:
> > 
> > This should be WillBeHeapVector. And trace it.
> 
> I'm getting build errors because there is a compile assert
> WeDontSupportWeaknessInHeapVectorsOrDeques. How should this be handled?

It depends rather on what you want to happen when the weak pointers are deleted.
 Does the rest of the vector just move down, or are there null entries in the
vector. We don't know the answer, and both could give null pointer crashes if
the weak processing happens at an unexpected time, so we don't support weakness
in vectors at the moment.  If a consensus arises about how we want vectors to
behave with weakness we might introduce support.

I haven't landed HeapLinkedHashSet yet, but that provides a hashset that
preserves order and can handle weakness. That may be what you need.

[haraken, 2014-04-14 09:27:55]

https://codereview.chromium.org/173363002/diff/500001/Source/modules/mediastr...
File Source/modules/mediastream/MediaStreamTrack.cpp (right):

https://codereview.chromium.org/173363002/diff/500001/Source/modules/mediastr...
Source/modules/mediastream/MediaStreamTrack.cpp:209: size_t pos =
m_registeredMediaStreams.find(mediaStream);

As far as I see this code, it seems we *should* use a HashSet instead of a
Vector :)

https://codereview.chromium.org/173363002/diff/500001/Source/modules/mediastr...
File Source/modules/mediastream/MediaStreamTrack.h (right):

https://codereview.chromium.org/173363002/diff/500001/Source/modules/mediastr...
Source/modules/mediastream/MediaStreamTrack.h:97:
Vector<RawPtrWillBeWeakMember<MediaStream> > m_registeredMediaStreams;
On 2014/04/14 09:10:35, Erik Corry wrote:
> On 2014/04/14 05:56:17, keishi1 wrote:
> > On 2014/04/14 04:42:43, haraken wrote:
> > > 
> > > This should be WillBeHeapVector. And trace it.
> > 
> > I'm getting build errors because there is a compile assert
> > WeDontSupportWeaknessInHeapVectorsOrDeques. How should this be handled?
> 
> It depends rather on what you want to happen when the weak pointers are
deleted.
>  Does the rest of the vector just move down, or are there null entries in the
> vector. We don't know the answer, and both could give null pointer crashes if
> the weak processing happens at an unexpected time, so we don't support
weakness
> in vectors at the moment.  If a consensus arises about how we want vectors to
> behave with weakness we might introduce support.
> 
> I haven't landed HeapLinkedHashSet yet, but that provides a hashset that
> preserves order and can handle weakness. That may be what you need.

Does the order matters for m_registeredMediaStreams? If the order doesn't
matter, I guess you can simply use HeapHashSet<WeakMember>.

[keishi, 2014-04-21 07:14:39]

RTCPeerConnection.html is flaky and crashing locally.
http://jsbin.com/rowakivu/1
The crash log is gibberish and I am having trouble finding the problem.

https://codereview.chromium.org/173363002/diff/500001/Source/modules/mediastr...
File Source/modules/mediastream/MediaStreamTrack.h (right):

https://codereview.chromium.org/173363002/diff/500001/Source/modules/mediastr...
Source/modules/mediastream/MediaStreamTrack.h:97:
Vector<RawPtrWillBeWeakMember<MediaStream> > m_registeredMediaStreams;
On 2014/04/14 09:27:57, haraken wrote:
> On 2014/04/14 09:10:35, Erik Corry wrote:
> > On 2014/04/14 05:56:17, keishi1 wrote:
> > > On 2014/04/14 04:42:43, haraken wrote:
> > > > 
> > > > This should be WillBeHeapVector. And trace it.
> > > 
> > > I'm getting build errors because there is a compile assert
> > > WeDontSupportWeaknessInHeapVectorsOrDeques. How should this be handled?
> > 
> > It depends rather on what you want to happen when the weak pointers are
> deleted.
> >  Does the rest of the vector just move down, or are there null entries in
the
> > vector. We don't know the answer, and both could give null pointer crashes
if
> > the weak processing happens at an unexpected time, so we don't support
> weakness
> > in vectors at the moment.  If a consensus arises about how we want vectors
to
> > behave with weakness we might introduce support.
> > 
> > I haven't landed HeapLinkedHashSet yet, but that provides a hashset that
> > preserves order and can handle weakness. That may be what you need.
> 
> Does the order matters for m_registeredMediaStreams? If the order doesn't
> matter, I guess you can simply use HeapHashSet<WeakMember>.

I've looked into the usage and it seams like this can be HeapHashSet<WeakMember>

[sof, 2014-04-21 19:51:30]

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
File Source/modules/mediastream/RTCPeerConnection.cpp (right):

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
Source/modules/mediastream/RTCPeerConnection.cpp:211: stop();
On 2014/04/11 20:03:06, keishi1 wrote:
> On 2014/04/08 04:42:35, haraken wrote:
> > 
> > This is problematic because stop() touches m_dataChannels which are on heap.
> 
> Done.

Sorry if I missed it, but how do you arrange for RTCPeerConnection::stop() to be
called?

[tkent, 2014-04-24 04:21:48]

https://codereview.chromium.org/173363002/diff/540001/Source/modules/mediastr...
File Source/modules/mediastream/MediaStream.h (right):

https://codereview.chromium.org/173363002/diff/540001/Source/modules/mediastr...
Source/modules/mediastream/MediaStream.h:43: class MediaStream FINAL : public
RefCountedWillBeRefCountedGarbageCollected<MediaStream>, public ScriptWrappable,
public URLRegistrable, public MediaStreamDescriptorClient, public
EventTargetWithInlineData, public ContextLifecycleObserver {
Please explain a reason of MediaStreamTrack::Observer in the CL description.

nit: The super class list is very long.  We may wrap them.

https://codereview.chromium.org/173363002/diff/540001/Source/modules/mediastr...
File Source/modules/mediastream/RTCPeerConnection.cpp (right):

https://codereview.chromium.org/173363002/diff/540001/Source/modules/mediastr...
Source/modules/mediastream/RTCPeerConnection.cpp:211: ASSERT(m_stopped);
This change is not obvious.  Please explain it in the CL description.

[keishi, 2014-05-08 04:15:42]

ASSERT(m_stopped) was causing the test failure so I fixed that.

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
File Source/modules/mediastream/RTCPeerConnection.cpp (right):

https://codereview.chromium.org/173363002/diff/420001/Source/modules/mediastr...
Source/modules/mediastream/RTCPeerConnection.cpp:211: stop();
On 2014/04/21 19:51:31, sof wrote:
> On 2014/04/11 20:03:06, keishi1 wrote:
> > On 2014/04/08 04:42:35, haraken wrote:
> > > 
> > > This is problematic because stop() touches m_dataChannels which are on
heap.
> > 
> > Done.
> 
> Sorry if I missed it, but how do you arrange for RTCPeerConnection::stop() to
be
> called?

I think this call was unnecessary. RTCPeerConnection::hasPendingActivity() is
false until RTCPeerConnection::stop() is called so I think that means
RTCPeerConnection will not be GCed until stop() is called. 

ASSERT(m_stopped) was wrong because the RTCPeerConnection will be destroyed
right after creating in RTCPeerConnection::create.

https://codereview.chromium.org/173363002/diff/540001/Source/modules/mediastr...
File Source/modules/mediastream/MediaStream.h (right):

https://codereview.chromium.org/173363002/diff/540001/Source/modules/mediastr...
Source/modules/mediastream/MediaStream.h:43: class MediaStream FINAL : public
RefCountedWillBeRefCountedGarbageCollected<MediaStream>, public ScriptWrappable,
public URLRegistrable, public MediaStreamDescriptorClient, public
EventTargetWithInlineData, public ContextLifecycleObserver {
On 2014/04/24 04:21:49, tkent wrote:
> Please explain a reason of MediaStreamTrack::Observer in the CL description.
> 
> nit: The super class list is very long.  We may wrap them.

Done.

[keishi, 2014-05-08 04:15:53]

ASSERT(m_stopped) was causing the test failure so I fixed that. PTAL

[haraken, 2014-05-08 05:08:37]

LGTM with comments.

tommyw@: We're planning to land this CL, but would it be OK?

https://codereview.chromium.org/173363002/diff/560001/Source/modules/mediastr...
File Source/modules/mediastream/MediaDevicesRequest.cpp (right):

https://codereview.chromium.org/173363002/diff/560001/Source/modules/mediastr...
Source/modules/mediastream/MediaDevicesRequest.cpp:52: }

At first, I thought it would be better to add ASSERT(!m_controller), but we
won't be able to add it, because it's possible that the MediaDevicesRequest is
destructed immediately after created without stop() getting called.

https://codereview.chromium.org/173363002/diff/560001/Source/modules/mediastr...
File Source/modules/mediastream/MediaStream.h (right):

https://codereview.chromium.org/173363002/diff/560001/Source/modules/mediastr...
Source/modules/mediastream/MediaStream.h:87: virtual void trace(Visitor*);

Remove virtual.

https://codereview.chromium.org/173363002/diff/560001/Source/modules/mediastr...
File Source/modules/mediastream/MediaStreamTrack.h (right):

https://codereview.chromium.org/173363002/diff/560001/Source/modules/mediastr...
Source/modules/mediastream/MediaStreamTrack.h:97:
WillBeHeapHashSet<RawPtrWillBeWeakMember<MediaStream> >
m_registeredMediaStreams;

I'm sorry I mentioned that this should be a hash set of weak members, but
shouldn't this probably be a hash set of strong members? It looks like the
MediaStreamTrack and the registered MediaStreams should live and die together.

https://codereview.chromium.org/173363002/diff/560001/Source/modules/mediastr...
File Source/modules/mediastream/RTCPeerConnection.cpp (right):

https://codereview.chromium.org/173363002/diff/560001/Source/modules/mediastr...
Source/modules/mediastream/RTCPeerConnection.cpp:211:
ASSERT(m_dataChannels.isEmpty());
> I think this call was unnecessary. RTCPeerConnection::hasPendingActivity() is
> false until RTCPeerConnection::stop() is called so I think that means
> RTCPeerConnection will not be GCed until stop() is called. 

This is a bit fragile assumption, because you're assuming that a wrapper is
always created when RTCPeerConnection is created. (It looks like that is the
case, but it's a bit weird assumption.)

However, there is other code that have the same assumption, so I'm OK with this
CL. We need to improve the infrastructure of ActiveDOMObject.

Would you add a comment about the ASSERT?

https://codereview.chromium.org/173363002/diff/560001/Source/platform/mediast...
File Source/platform/mediastream/RTCStatsResponseBase.h (right):

https://codereview.chromium.org/173363002/diff/560001/Source/platform/mediast...
Source/platform/mediastream/RTCStatsResponseBase.h:41: virtual void
trace(Visitor*) = 0;

Pure virtual trace is forbidden (see an email from Ian a couple of days ago).
You need:

  virtual void trace(Visitor*) { }

zerny@: Would it be possible to add this verification to the plugin?

[zerny-chromium, 2014-05-08 08:02:35]

https://codereview.chromium.org/173363002/diff/560001/Source/platform/mediast...
File Source/platform/mediastream/RTCStatsResponseBase.h (right):

https://codereview.chromium.org/173363002/diff/560001/Source/platform/mediast...
Source/platform/mediastream/RTCStatsResponseBase.h:41: virtual void
trace(Visitor*) = 0;
On 2014/05/08 05:08:39, haraken wrote:
> 
> Pure virtual trace is forbidden (see an email from Ian a couple of days ago).
> You need:
> 
>   virtual void trace(Visitor*) { }
> 
> zerny@: Would it be possible to add this verification to the plugin?

Yes, I'll add it asap.

[keishi, 2014-05-09 01:53:53]

https://codereview.chromium.org/173363002/diff/560001/Source/modules/mediastr...
File Source/modules/mediastream/MediaStream.h (right):

https://codereview.chromium.org/173363002/diff/560001/Source/modules/mediastr...
Source/modules/mediastream/MediaStream.h:87: virtual void trace(Visitor*);
On 2014/05/08 05:08:39, haraken wrote:
> 
> Remove virtual.

Done.

https://codereview.chromium.org/173363002/diff/560001/Source/modules/mediastr...
File Source/modules/mediastream/MediaStreamTrack.h (right):

https://codereview.chromium.org/173363002/diff/560001/Source/modules/mediastr...
Source/modules/mediastream/MediaStreamTrack.h:97:
WillBeHeapHashSet<RawPtrWillBeWeakMember<MediaStream> >
m_registeredMediaStreams;
On 2014/05/08 05:08:39, haraken wrote:
> 
> I'm sorry I mentioned that this should be a hash set of weak members, but
> shouldn't this probably be a hash set of strong members? It looks like the
> MediaStreamTrack and the registered MediaStreams should live and die together.

Done.

https://codereview.chromium.org/173363002/diff/560001/Source/modules/mediastr...
File Source/modules/mediastream/RTCPeerConnection.cpp (right):

https://codereview.chromium.org/173363002/diff/560001/Source/modules/mediastr...
Source/modules/mediastream/RTCPeerConnection.cpp:211:
ASSERT(m_dataChannels.isEmpty());
On 2014/05/08 05:08:39, haraken wrote:
> > I think this call was unnecessary. RTCPeerConnection::hasPendingActivity()
is
> > false until RTCPeerConnection::stop() is called so I think that means
> > RTCPeerConnection will not be GCed until stop() is called. 
> 
> This is a bit fragile assumption, because you're assuming that a wrapper is
> always created when RTCPeerConnection is created. (It looks like that is the
> case, but it's a bit weird assumption.)
> 
> However, there is other code that have the same assumption, so I'm OK with
this
> CL. We need to improve the infrastructure of ActiveDOMObject.
> 
> Would you add a comment about the ASSERT?

Done.

https://codereview.chromium.org/173363002/diff/560001/Source/platform/mediast...
File Source/platform/mediastream/RTCStatsResponseBase.h (right):

https://codereview.chromium.org/173363002/diff/560001/Source/platform/mediast...
Source/platform/mediastream/RTCStatsResponseBase.h:41: virtual void
trace(Visitor*) = 0;
On 2014/05/08 05:08:39, haraken wrote:
> 
> Pure virtual trace is forbidden (see an email from Ian a couple of days ago).
> You need:
> 
>   virtual void trace(Visitor*) { }
> 
> zerny@: Would it be possible to add this verification to the plugin?

Done.

[keishi, 2014-05-22 02:01:19]

On 2014/05/08 05:08:37, haraken wrote:
> LGTM with comments.
> 
> tommyw@: We're planning to land this CL, but would it be OK?
> 
>
https://codereview.chromium.org/173363002/diff/560001/Source/modules/mediastr...
> File Source/modules/mediastream/MediaDevicesRequest.cpp (right):
> 
>
https://codereview.chromium.org/173363002/diff/560001/Source/modules/mediastr...
> Source/modules/mediastream/MediaDevicesRequest.cpp:52: }
> 
> At first, I thought it would be better to add ASSERT(!m_controller), but we
> won't be able to add it, because it's possible that the MediaDevicesRequest is
> destructed immediately after created without stop() getting called.
> 
>
https://codereview.chromium.org/173363002/diff/560001/Source/modules/mediastr...
> File Source/modules/mediastream/MediaStream.h (right):
> 
>
https://codereview.chromium.org/173363002/diff/560001/Source/modules/mediastr...
> Source/modules/mediastream/MediaStream.h:87: virtual void trace(Visitor*);
> 
> Remove virtual.
> 
>
https://codereview.chromium.org/173363002/diff/560001/Source/modules/mediastr...
> File Source/modules/mediastream/MediaStreamTrack.h (right):
> 
>
https://codereview.chromium.org/173363002/diff/560001/Source/modules/mediastr...
> Source/modules/mediastream/MediaStreamTrack.h:97:
> WillBeHeapHashSet<RawPtrWillBeWeakMember<MediaStream> >
> m_registeredMediaStreams;
> 
> I'm sorry I mentioned that this should be a hash set of weak members, but
> shouldn't this probably be a hash set of strong members? It looks like the
> MediaStreamTrack and the registered MediaStreams should live and die together.
> 
>
https://codereview.chromium.org/173363002/diff/560001/Source/modules/mediastr...
> File Source/modules/mediastream/RTCPeerConnection.cpp (right):
> 
>
https://codereview.chromium.org/173363002/diff/560001/Source/modules/mediastr...
> Source/modules/mediastream/RTCPeerConnection.cpp:211:
> ASSERT(m_dataChannels.isEmpty());
> > I think this call was unnecessary. RTCPeerConnection::hasPendingActivity()
is
> > false until RTCPeerConnection::stop() is called so I think that means
> > RTCPeerConnection will not be GCed until stop() is called. 
> 
> This is a bit fragile assumption, because you're assuming that a wrapper is
> always created when RTCPeerConnection is created. (It looks like that is the
> case, but it's a bit weird assumption.)
> 
> However, there is other code that have the same assumption, so I'm OK with
this
> CL. We need to improve the infrastructure of ActiveDOMObject.
> 
> Would you add a comment about the ASSERT?
> 
>
https://codereview.chromium.org/173363002/diff/560001/Source/platform/mediast...
> File Source/platform/mediastream/RTCStatsResponseBase.h (right):
> 
>
https://codereview.chromium.org/173363002/diff/560001/Source/platform/mediast...
> Source/platform/mediastream/RTCStatsResponseBase.h:41: virtual void
> trace(Visitor*) = 0;
> 
> Pure virtual trace is forbidden (see an email from Ian a couple of days ago).
> You need:
> 
>   virtual void trace(Visitor*) { }
> 
> zerny@: Would it be possible to add this verification to the plugin?

Pinging tommyw@. Would it be ok to land this?

[keishi, 2014-05-27 04:12:00]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2014-05-27 04:12:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/keishi@chromium.org/173363002/620001

[commit-bot: I haz the power, 2014-05-27 04:12:13]

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

[commit-bot: I haz the power, 2014-05-27 04:12:14]

A disapproval has been posted by following reviewers: nbarth@chromium.org.
Please make sure to get positive review before another CQ attempt.

[keishi, 2014-05-27 04:16:46]

On 2014/05/27 04:12:14, I haz the power (commit-bot) wrote:
> A disapproval has been posted by following reviewers:
mailto:nbarth@chromium.org.
> Please make sure to get positive review before another CQ attempt.

nbarth@ could you take a look?

[Nils Barth (inactive), 2014-05-27 04:41:08]

On 2014/05/27 04:16:46, keishi wrote:
> nbarth@ could you take a look?

LGTM; previous disapproval (3 months ago) was just to prevent build breakage
when switching to Python, which is now ancient history.

[keishi, 2014-05-27 04:43:52]

On 2014/05/27 04:41:08, Nils Barth wrote:
> On 2014/05/27 04:16:46, keishi wrote:
> > nbarth@ could you take a look?
> 
> LGTM; previous disapproval (3 months ago) was just to prevent build breakage
> when switching to Python, which is now ancient history.

Thanks.

[keishi, 2014-05-27 04:44:05]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2014-05-27 04:44:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/keishi@chromium.org/173363002/620001

[commit-bot: I haz the power, 2014-05-27 06:03:46]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/6213)

[tkent, 2014-05-27 06:08:37]

web and public lgtm

[commit-bot: I haz the power, 2014-05-27 06:20:39]

Change committed as 174837