Bug 172859 – getUserMedia is prompting too often

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[youenn fablet]

It should not reprompt when access is already given to the page

[youenn fablet]

Created attachment 311832 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/32589950>

[Radar WebKit Bug Importer]

<rdar://problem/32589947>

[youenn fablet]

Created attachment 312089 [details]
Patch

[Build Bot]

Comment on attachment 312089 [details]
Patch

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

New failing tests:
fast/mediastream/getUserMedia-grant-persistency.html
http/tests/media/media-stream/get-user-media-prompt.html

[Build Bot]

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

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

[youenn fablet]

Looking at the test failures, the patch seems good.
I will need to rebase one test and change another one which is too sensitive.

[youenn fablet]

Created attachment 312151 [details]
Patch

[Eric Carlson]

Comment on attachment 312151 [details]
Patch

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

> Source/WebKit2/ChangeLog:14
> +        A granted request is now keeping its mainFrameID.

Nit: "is now keeping" => "now keeps"

> Source/WebKit2/ChangeLog:15
> +        Whenever the document of the main frame is changing, the granted requests for that mainFrameID will be removed.

Nit: "main frame is changing" => "main frame changes"

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:241
> -        if (!m_page.isValid())
> +        if (!m_page.isValid() || !m_page.mainFrame())

Why is this change necessary?

> LayoutTests/fast/mediastream/getUserMedia-grant-persistency.html:48
> +    }).then(() => {
> +        if (!window.internals)
> +            return;
> +        assert_true(internals.pageMediaState().includes("HasActiveVideoCaptureDevice"), "Check active video");
> +        assert_true(internals.pageMediaState().includes("HasActiveAudioCaptureDevice"), "Check active audio");
> +    });

This seems wrong. Why would we allow a call to getUserMedia not triggered by a user gesture to un-mute capture?

> LayoutTests/fast/mediastream/getUserMedia-grant-persistency.html:59
> +    }).then(() => {
> +    }).then(() => {

Nit: are both of these needed?

> LayoutTests/http/tests/media/media-stream/get-user-media-prompt-expected.txt:20
>  ** Request a stream with video and audio, the user should be prompted again **

Nit: this comment should be updated, the user is NOT prompted again.

[youenn fablet]

Created attachment 312187 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 312187 [details]
Patch for landing

Rejecting attachment 312187 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 312187, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/3888548

[youenn fablet]

Created attachment 312220 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 312220 [details]
Patch for landing

Clearing flags on attachment: 312220

Committed r217910: <http://trac.webkit.org/changeset/217910>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.