Bug 206929 – Make sure MediaRecorder.requestData returns data with the new writer backend

• Component: WebRTC
• Status: RESOLVED FIXED

[youenn fablet]

Make sure MediaRecorder.requestData returns data with the new writer backend

[youenn fablet]

Created attachment 389134 [details]
Patch

[youenn fablet]

*** Bug 212275 has been marked as a duplicate of this bug. ***

[youenn fablet]

<rdar://problem/63601261>

[youenn fablet]

Created attachment 401889 [details]
Adding deprecation macros

[Darin Adler]

Comment on attachment 401889 [details]
Adding deprecation macros

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

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:489
> +            ALLOW_DEPRECATED_DECLARATIONS_BEGIN
> +            [m_writer flush];
> +            ALLOW_DEPRECATED_DECLARATIONS_END

Do we have to write code using a deprecated function? What’s the long term strategy for the deprecation? Why can’t we take advantage of it now?

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:496
> +                RefPtr<SharedBuffer> buffer;
> +                if (weakThis)
> +                    buffer = WTFMove(m_data);
> +
> +                completionHandler(WTFMove(buffer));

How about this version?

    if (!weakThis)
        return completionHandler({ });

    completionHandler(WTFMove(m_data));

Seems easier to read than the one with the local variable and double WTFMove? And has the same early exit here as above. I probably would have written it with nullptr and without using a "void return", but I figured we could stay consistent.

[youenn fablet]

(In reply to Darin Adler from comment #5)
> Comment on attachment 401889 [details]
> Adding deprecation macros
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401889&action=review
> 
> > Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:489
> > +            ALLOW_DEPRECATED_DECLARATIONS_BEGIN
> > +            [m_writer flush];
> > +            ALLOW_DEPRECATED_DECLARATIONS_END
> 
> Do we have to write code using a deprecated function? What’s the long term
> strategy for the deprecation? Why can’t we take advantage of it now?

The replacement method is not available yet everywhere.
Plan is to switch at some point.

> > Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:496
> > +                RefPtr<SharedBuffer> buffer;
> > +                if (weakThis)
> > +                    buffer = WTFMove(m_data);
> > +
> > +                completionHandler(WTFMove(buffer));
> 
> How about this version?
> 
>     if (!weakThis)
>         return completionHandler({ });
> 
>     completionHandler(WTFMove(m_data));
> 
> Seems easier to read than the one with the local variable and double
> WTFMove? And has the same early exit here as above. I probably would have
> written it with nullptr and without using a "void return", but I figured we
> could stay consistent.

I wrote the double move to ensure m_data gets cleared.
I'll do the early return.

[youenn fablet]

Created attachment 401986 [details]
Patch for landing

[youenn fablet]

Went with sd::exchange

[EWS]

Committed r263084: <https://trac.webkit.org/changeset/263084>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401986 [details].

[Darin Adler]

Comment on attachment 401889 [details]
Adding deprecation macros

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

>>> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:496
>>> +                completionHandler(WTFMove(buffer));
>> 
>> How about this version?
>> 
>>     if (!weakThis)
>>         return completionHandler({ });
>> 
>>     completionHandler(WTFMove(m_data));
>> 
>> Seems easier to read than the one with the local variable and double WTFMove? And has the same early exit here as above. I probably would have written it with nullptr and without using a "void return", but I figured we could stay consistent.
> 
> I wrote the double move to ensure m_data gets cleared.
> I'll do the early return.

If you want to ensure m_data gets cleared, then we should write std::exchange(m_data, {}) instead of WTFMove(m_data).

[youenn fablet]

(In reply to Darin Adler from comment #10)
> Comment on attachment 401889 [details]
> Adding deprecation macros
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401889&action=review
> 
> >>> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:496
> >>> +                completionHandler(WTFMove(buffer));
> >> 
> >> How about this version?
> >> 
> >>     if (!weakThis)
> >>         return completionHandler({ });
> >> 
> >>     completionHandler(WTFMove(m_data));
> >> 
> >> Seems easier to read than the one with the local variable and double WTFMove? And has the same early exit here as above. I probably would have written it with nullptr and without using a "void return", but I figured we could stay consistent.
> > 
> > I wrote the double move to ensure m_data gets cleared.
> > I'll do the early return.
> 
> If you want to ensure m_data gets cleared, then we should write
> std::exchange(m_data, {}) instead of WTFMove(m_data).

Yes, that is what I ended up landing.