Bug 172200 – iOS WebRTC Media Capture should not allow camera capture from background tab

• Component: Media
• Status: RESOLVED FIXED

[youenn fablet]

iOS WebRTC Media Capture should not allow camera capture from background tab

[youenn fablet]

radar 32199028

[youenn fablet]

rdar://problem/32199028

[youenn fablet]

Created attachment 310324 [details]
Patch

[youenn fablet]

Created attachment 310348 [details]
Patch

[Build Bot]

Comment on attachment 310348 [details]
Patch

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

New failing tests:
platform/ios/mediastream/getUserMedia-disabled-in-background-tabs.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 310348 [details]
Patch

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

New failing tests:
http/tests/media/hls/video-controls-live-stream.html

[Build Bot]

Created attachment 310382 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Eric Carlson]

Comment on attachment 310348 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        On iOS, muting/unmuting the current video source according Document visibility.

Nit: "muting/unmuting" => "mute/unmute", "according" => "according to"

> Source/WebCore/dom/Document.cpp:6991
> +void Document::notifyVisibilityChangedToMediaCapture()
> +{
> +#if ENABLE(MEDIA_STREAM)
> +    RealtimeMediaSourceCenter::singleton().setVisibility(!hidden());
> +#endif
> +}

Nit: why is it necessary to put this one line call into a separate method?

[Eric Carlson]

Comment on attachment 310348 [details]
Patch

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

> LayoutTests/platform/ios/mediastream/getUserMedia-disabled-in-background-tabs.html:19
> +        let audioTrack = firstStream.getAudioTracks()[0];
> +        let videoTrack = firstStream.getVideoTracks()[0];

Where is firstStream set?

[youenn fablet]

Created attachment 310394 [details]
Patch for landing

[youenn fablet]

(In reply to Eric Carlson from comment #9)
> Comment on attachment 310348 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=310348&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        On iOS, muting/unmuting the current video source according Document visibility.
> 
> Nit: "muting/unmuting" => "mute/unmute", "according" => "according to"
> 
> > Source/WebCore/dom/Document.cpp:6991
> > +void Document::notifyVisibilityChangedToMediaCapture()
> > +{
> > +#if ENABLE(MEDIA_STREAM)
> > +    RealtimeMediaSourceCenter::singleton().setVisibility(!hidden());
> > +#endif
> > +}
> 
> Nit: why is it necessary to put this one line call into a separate method?

I did not want to pollute Document::visibilityStateChanged with too specific stuff.
That is why I went this way. I can go back and inline it directly though.

(In reply to Eric Carlson from comment #10)
> Comment on attachment 310348 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=310348&action=review
> 
> > LayoutTests/platform/ios/mediastream/getUserMedia-disabled-in-background-tabs.html:19
> > +        let audioTrack = firstStream.getAudioTracks()[0];
> > +        let videoTrack = firstStream.getVideoTracks()[0];
> 
> Where is firstStream set?

Fixed in the uploaded patch, I'll make sure it passes on the iOS simulator bot before committing the change.
Patch works on the device (manual testing)

[Build Bot]

Comment on attachment 310394 [details]
Patch for landing

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

New failing tests:
platform/ios/mediastream/getUserMedia-disabled-in-background-tabs.html

[Build Bot]

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

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

[youenn fablet]

Created attachment 310429 [details]
Patch for landing

[youenn fablet]

Let's land it as is so that we have the correct behavior in the regular case.
And let's fix the more infrequent case of fullscreen mode (where camera should not be stopped) on a follow-up

[WebKit Commit Bot]

Comment on attachment 310429 [details]
Patch for landing

Clearing flags on attachment: 310429

Committed r216999: <http://trac.webkit.org/changeset/216999>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

(In reply to WebKit Commit Bot from comment #17)
> Comment on attachment 310429 [details]
> Patch for landing
> 
> Clearing flags on attachment: 310429
> 
> Committed r216999: <http://trac.webkit.org/changeset/216999>

This change broke the Windows build:
https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/1574

WebCoreTestSupport.lib(JSInternals.obj) : error LNK2019: unresolved external symbol "public: void __thiscall WebCore::Internals::setPageVisibility(bool)" (?setPageVisibility@Internals@WebCore@@QAEX_N@Z) referenced in function "__int64 __cdecl WebCore::jsInternalsPrototypeFunctionSetPageVisibilityCaller(class JSC::ExecState *,class WebCore::JSInternals *,class JSC::ThrowScope &)" (?jsInternalsPrototypeFunctionSetPageVisibilityCaller@WebCore@@YA_JPAVExecState@JSC@@PAVJSInternals@1@AAVThrowScope@3@@Z) [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Tools\DumpRenderTree\DumpRenderTreeLib.vcxproj]

[youenn fablet]

Thanks Ryan, I'll fix it asap

(In reply to Ryan Haddad from comment #19)
> (In reply to WebKit Commit Bot from comment #17)
> > Comment on attachment 310429 [details]
> > Patch for landing
> > 
> > Clearing flags on attachment: 310429
> > 
> > Committed r216999: <http://trac.webkit.org/changeset/216999>
> 
> This change broke the Windows build:
> https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/
> 1574
> 
> WebCoreTestSupport.lib(JSInternals.obj) : error LNK2019: unresolved external
> symbol "public: void __thiscall WebCore::Internals::setPageVisibility(bool)"
> (?setPageVisibility@Internals@WebCore@@QAEX_N@Z) referenced in function
> "__int64 __cdecl
> WebCore::jsInternalsPrototypeFunctionSetPageVisibilityCaller(class
> JSC::ExecState *,class WebCore::JSInternals *,class JSC::ThrowScope &)"
> (?jsInternalsPrototypeFunctionSetPageVisibilityCaller@WebCore@@YA_JPAVExecSta
> te@JSC@@PAVJSInternals@1@AAVThrowScope@3@@Z)
> [C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\Tools\DumpRenderTree\DumpRenderTreeLib.
> vcxproj]

[youenn fablet]

Filed bug 172257

[youenn fablet]

Reopening to attach new patch.

[youenn fablet]

Created attachment 311708 [details]
Patch

[youenn fablet]

Test was improved as part of bug 172858.