Bug 216415 – End of media capture should not be reported before 3 seconds of the start of capture

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[youenn fablet]

End of media capture should not be reported before 3 seconds of the start of capture

[youenn fablet]

Created attachment 408554 [details]
Patch

[youenn fablet]

Created attachment 408684 [details]
Patch

[youenn fablet]

<rdar://problem/68512358>

[Radar WebKit Bug Importer]

<rdar://problem/68839151>

[Eric Carlson]

Comment on attachment 408684 [details]
Patch

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

> Source/WebKit/ChangeLog:8
> +        Add support for delaying of end of capture.

We're not delaying the end of capture, just the state transition. Also, I think it would be helpful to state why this is being changed. 

Maybe just: A capture indicator should be visible to the user for at least three seconds.

> Source/WebKit/ChangeLog:9
> +        A timer is scheduled when transitioning from no capture to capture.

s/transitioning from no capture to capture/transitioning from capture to no capture/

> Source/WebKit/UIProcess/API/C/WKPage.cpp:3004
> +void WKPageSetMediaCaptureReportingDelayForTesting(WKPageRef page, double delay)
> +{
> +    toImpl(page)->setMediaCaptureReportingDelay(Seconds(delay));
> +}

It seems a shame to add new C API

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:330
> +@property (nonatomic, setter=_setMediaCaptureReportingDelayForTesting:) double _mediaCaptureReportingDelayForTesting WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

Should this go in WKWebViewPrivateForTesting.h instead?

> Source/WebKit/UIProcess/WebPageProxy.cpp:9073
> +    bool isReportingCapture = m_reportedMediaCaptureState & MediaProducer::MediaCaptureMask;

`haveReportedCapture` might be better.

[youenn fablet]

Thanks for the review.
Will update the patch according all comments before landing.

(In reply to Eric Carlson from comment #5)
> Comment on attachment 408684 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408684&action=review
> 
> > Source/WebKit/ChangeLog:8
> > +        Add support for delaying of end of capture.
> 
> We're not delaying the end of capture, just the state transition. Also, I
> think it would be helpful to state why this is being changed. 
> 
> Maybe just: A capture indicator should be visible to the user for at least
> three seconds.

OK

> > Source/WebKit/UIProcess/API/C/WKPage.cpp:3004
> > +void WKPageSetMediaCaptureReportingDelayForTesting(WKPageRef page, double delay)
> > +{
> > +    toImpl(page)->setMediaCaptureReportingDelay(Seconds(delay));
> > +}
> 
> It seems a shame to add new C API

Yes, I initially went ObjC.
I added this one so that other ports do not have to add their own versions to continue running some of the existing tests.

> > Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:330
> > +@property (nonatomic, setter=_setMediaCaptureReportingDelayForTesting:) double _mediaCaptureReportingDelayForTesting WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> Should this go in WKWebViewPrivateForTesting.h instead?

Did not know about it, yes!

[youenn fablet]

Created attachment 408799 [details]
Patch for landing

[EWS]

Committed r267081: <https://trac.webkit.org/changeset/267081>

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