Bug 159965 – Purge all uses of PassRefPtr in WebCore/Modules

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Gyuyoung Kim]

Final clean up all uses of PassRefPtr in Modules.

[Gyuyoung Kim]

Created attachment 284090 [details]
Patch

[Build Bot]

Comment on attachment 284090 [details]
Patch

Attachment 284090 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1711223

New failing tests:
fast/mediastream/MediaStreamTrackEvent-constructor.html

[Build Bot]

Created attachment 284094 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Gyuyoung Kim]

Created attachment 285094 [details]
Patch

[Gyuyoung Kim]

Created attachment 285106 [details]
Patch

[Build Bot]

Comment on attachment 285106 [details]
Patch

Attachment 285106 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1798768

New failing tests:
fast/mediastream/MediaStreamTrackEvent-constructor.html

[Build Bot]

Created attachment 285109 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Gyuyoung Kim]

Created attachment 285190 [details]
Patch

[Alex Christensen]

Comment on attachment 285190 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285190&action=review

> Source/WebCore/Modules/indieui/UIRequestEvent.cpp:45
>  UIRequestEvent::UIRequestEvent(const AtomicString& type, const UIRequestEventInit& initializer)
>      : UIEvent(type, initializer)
> -    , m_receiver(initializer.receiver)
> +    , m_receiver(*initializer.receiver)

This is bad.  I think it might be possible to completely get rid of UIRequestEventInit, though.

> Source/WebCore/Modules/indieui/UIRequestEvent.h:47
> -    EventTarget* receiver() const { return m_receiver.get(); }
> +    EventTarget* receiver() const { return const_cast<EventTarget*>(m_receiver.ptr()); }

Would it be helpful to make this return an EventTarget& ?

[Darin Adler]

Comment on attachment 285190 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285190&action=review

>> Source/WebCore/Modules/indieui/UIRequestEvent.cpp:45
>> +    , m_receiver(*initializer.receiver)
> 
> This is bad.  I think it might be possible to completely get rid of UIRequestEventInit, though.

We can’t get rid of UIRequestEventInit; eventually it will just be handled like any other dictionary.

What we should do, though, is change the argument to be UIRequestEventInit&& and then it’s fine to move values out of that that structure into the event we are creating.

>> Source/WebCore/Modules/indieui/UIRequestEvent.h:47
>> +    EventTarget* receiver() const { return const_cast<EventTarget*>(m_receiver.ptr()); }
> 
> Would it be helpful to make this return an EventTarget& ?

Yes.

[Alex Christensen]

We should either make the RefPtr in UIRequestEventInit a Ref or keep m_receiver as a Ref

[Darin Adler]

(In reply to comment #11)
> We should either make the RefPtr in UIRequestEventInit a Ref or keep
> m_receiver as a Ref

You mean keep m_receiver as a RefPtr.

The issue here is whether it is legal to pass a null when creating a UIRequestEvent from JavaScript. The specification <https://dvcs.w3.org/hg/IndieUI/raw-file/default/src/indie-ui-events.html> says:

    EventTarget receiver = null;

This is ambiguous. How can a non-nullable type have a default value of null? I think the default value is an error.

[Gyuyoung Kim]

Created attachment 285213 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #10)
> >> Source/WebCore/Modules/indieui/UIRequestEvent.h:47
> >> +    EventTarget* receiver() const { return const_cast<EventTarget*>(m_receiver.ptr()); }
> > 
> > Would it be helpful to make this return an EventTarget& ?
> 
> Yes.

Done.



(In reply to comment #12)
> (In reply to comment #11)
> > We should either make the RefPtr in UIRequestEventInit a Ref or keep
> > m_receiver as a Ref
> 
> You mean keep m_receiver as a RefPtr.
> 
> The issue here is whether it is legal to pass a null when creating a
> UIRequestEvent from JavaScript. The specification
> <https://dvcs.w3.org/hg/IndieUI/raw-file/default/src/indie-ui-events.html>
> says:
> 
>     EventTarget receiver = null;
> 
> This is ambiguous. How can a non-nullable type have a default value of null?
> I think the default value is an error.

I wonder what should I do to fix this issue ? You guys want to change m_receiver to RefPtr, or keep it as Ref.

[Chris Dumez]

Comment on attachment 285213 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285213&action=review

> Source/WebCore/Modules/indieui/UIRequestEvent.cpp:45
> +    , m_receiver(*initializer.receiver)

Uh, this is wrong. Currently, initializer.receiver will always be null as far as I can tell. There is a bug in UIRequestEvent.idl and someone forgot to add the [InitializedByEventConstructor] IDL attribute next to the "receiver" attribute. Therefore, the generated bindings will never initialize UIRequestEventInit::receiver.
Note that even if someone would fix the bug and add [InitializedByEventConstructor], I believe the receiver could sometimes be null (even though the attribute is not marked as nullable) because of a limitation in our bindings generator. So I think m_receiver should be a RefPtr<>.

Also, someone should probably fix the IDL in a follow-up if we care at all about it (I don't know who uses INDIEUI.

[Gyuyoung Kim]

Created attachment 285292 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #15)
> Comment on attachment 285213 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=285213&action=review
> 
> > Source/WebCore/Modules/indieui/UIRequestEvent.cpp:45
> > +    , m_receiver(*initializer.receiver)
> 
> Uh, this is wrong. Currently, initializer.receiver will always be null as
> far as I can tell. There is a bug in UIRequestEvent.idl and someone forgot
> to add the [InitializedByEventConstructor] IDL attribute next to the
> "receiver" attribute. Therefore, the generated bindings will never
> initialize UIRequestEventInit::receiver.
> Note that even if someone would fix the bug and add
> [InitializedByEventConstructor], I believe the receiver could sometimes be
> null (even though the attribute is not marked as nullable) because of a
> limitation in our bindings generator. So I think m_receiver should be a
> RefPtr<>.

Hey Chris, thank you for your review. I use RefPtr for m_receiver.

[Chris Dumez]

Comment on attachment 285292 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285292&action=review

> Source/WebCore/Modules/indieui/UIRequestEvent.h:47
> +    EventTarget& receiver() const { return *m_receiver; }

This isn't safe since m_receiver can be null.

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1304
> +void SourceBuffer::sourceBufferPrivateDidReceiveSample(SourceBufferPrivate*, MediaSample* sample)

sample should be a Ref<>&& given the call sites and given that it is dereferenced unconditionally in this function.

> Source/WebCore/Modules/mediastream/MediaTrackConstraints.cpp:55
> +    return Vector<RefPtr<MediaTrackConstraint>>();

return { };

> Source/WebCore/Modules/mediastream/RTCDataChannelEvent.cpp:50
>  RTCDataChannel* RTCDataChannelEvent::channel() const

Is it a lot more work to make this getter non-const? If it isn't, I would suggest doing this rather than a const_cast below.

> Source/WebCore/html/HTMLMediaElement.cpp:1849
> +void HTMLMediaElement::textTrackAddCue(TextTrack* track, TextTrackCue* cue)

Looks like this should take a reference as cue is dereferenced unconditionally below.

> Source/WebCore/html/HTMLMediaElement.cpp:1864
> +void HTMLMediaElement::textTrackRemoveCue(TextTrack*, TextTrackCue* cue)

Should be a reference.

> Source/WebCore/platform/efl/GamepadsEfl.cpp:229
>              into->set(i, 0);

nullptr

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:571
>          RefPtr<MediaSample> mediaSample = MediaSampleAVFObjC::create(sampleBuffer, trackID);

auto. (so it is a Ref<>)

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:573
> +        m_client->sourceBufferPrivateDidReceiveSample(this, mediaSample.get());

WTFMove(mediaSample)

> Source/WebCore/platform/mediastream/mac/RealtimeMediaSourceCenterMac.cpp:85
> +        audioSources = AVCaptureDeviceManager::singleton().bestSourcesForTypeAndConstraints(RealtimeMediaSource::Type::Audio, audioConstraints.get());

We know audioConstraints cannot be null so we could pass a reference here.

> Source/WebCore/platform/mediastream/mac/RealtimeMediaSourceCenterMac.cpp:96
> +        videoSources = AVCaptureDeviceManager::singleton().bestSourcesForTypeAndConstraints(RealtimeMediaSource::Type::Video, videoConstraints.get());

We know videoConstraints cannot be null so we could pass a reference here.

> Source/WebCore/platform/mock/mediasource/MockSourceBufferPrivate.cpp:195
> +    m_client->sourceBufferPrivateDidReceiveSample(this, MockMediaSample::create(sampleBox).get());

no .get() here.

[Chris Dumez]

Comment on attachment 285292 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285292&action=review

> Source/WebCore/Modules/mediastream/CaptureDeviceManager.cpp:91
> +Vector<RefPtr<RealtimeMediaSource>> CaptureDeviceManager::bestSourcesForTypeAndConstraints(RealtimeMediaSource::Type type, MediaConstraints* constraints)

constraints should be a MediaConstraints&.

> Source/WebCore/Modules/mediastream/CaptureDeviceManager.cpp:106
> +        if (RefPtr<RealtimeMediaSource> captureSource = sourceWithUID(captureDevice.m_persistentDeviceID, type, constraints))

&contraints

> Source/WebCore/Modules/mediastream/CaptureDeviceManager.h:43
> +    virtual Vector<RefPtr<RealtimeMediaSource>> bestSourcesForTypeAndConstraints(RealtimeMediaSource::Type, MediaConstraints*);

MediaConstraints&

[Chris Dumez]

Comment on attachment 285292 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285292&action=review

> Source/WebCore/Modules/mediastream/SourceInfo.cpp:36
> +Ref<SourceInfo> SourceInfo::create(TrackSourceInfo& trackSourceInfo)

Should probably be a Ref<>&&

> Source/WebCore/Modules/mediastream/SourceInfo.cpp:41
> +SourceInfo::SourceInfo(TrackSourceInfo& trackSourceInfo)

Should probably be a Ref<>&&

> Source/WebCore/Modules/mediastream/SourceInfo.cpp:42
>      : m_trackSourceInfo(trackSourceInfo)

WTFMove()

> Source/WebCore/Modules/mediastream/SourceInfo.h:43
> +    static Ref<SourceInfo> create(TrackSourceInfo&);

Should probably be a Ref<>&&

> Source/WebCore/Modules/mediastream/SourceInfo.h:50
> +    SourceInfo(TrackSourceInfo&);

Should probably be a Ref<>&&

[Gyuyoung Kim]

Comment on attachment 285292 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285292&action=review

>> Source/WebCore/Modules/mediastream/RTCDataChannelEvent.cpp:50
>>  RTCDataChannel* RTCDataChannelEvent::channel() const
> 
> Is it a lot more work to make this getter non-const? If it isn't, I would suggest doing this rather than a const_cast below.

There were too many build errors when changing  it with "const RTCDataChannel* channel()". That's why I use const_cast<>.

[Gyuyoung Kim]

Created attachment 285301 [details]
Patch

[Gyuyoung Kim]

Created attachment 285309 [details]
Patch

[Build Bot]

Comment on attachment 285309 [details]
Patch

Attachment 285309 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1810094

New failing tests:
fast/scrolling/ios/scrollTo-at-page-load.html

[Build Bot]

Created attachment 285312 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.5

[Chris Dumez]

(In reply to comment #21)
> Comment on attachment 285292 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=285292&action=review
> 
> >> Source/WebCore/Modules/mediastream/RTCDataChannelEvent.cpp:50
> >>  RTCDataChannel* RTCDataChannelEvent::channel() const
> > 
> > Is it a lot more work to make this getter non-const? If it isn't, I would suggest doing this rather than a const_cast below.
> 
> There were too many build errors when changing  it with "const
> RTCDataChannel* channel()". That's why I use const_cast<>.

This is not what I suggested. I suggested to drop the const:
RTCDataChannel* channel();

[Gyuyoung Kim]

Created attachment 285403 [details]
Patch

[Gyuyoung Kim]

Created attachment 285420 [details]
Patch

[Gyuyoung Kim]

Created attachment 285421 [details]
Patch

[Chris Dumez]

Comment on attachment 285421 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285421&action=review

r=me

> Source/WebCore/platform/mock/mediasource/MockSourceBufferPrivate.cpp:195
> +    m_client->sourceBufferPrivateDidReceiveSample(this, MockMediaSample::create(sampleBox).get());

I don't like this .get() here. Why do we need it? create() returns a Ref<> which can be implicitly converted into a C++ reference. Therefore, I believe this .get() is redundant.

[Gyuyoung Kim]

Created attachment 285489 [details]
Patch for landing

[Gyuyoung Kim]

Created attachment 285518 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 285518 [details]
Patch for landing

Clearing flags on attachment: 285518

Committed r204239: <http://trac.webkit.org/changeset/204239>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.