Bug 152296 - webkitGetUserMedia built-in should use @then and not then
Summary: webkitGetUserMedia built-in should use @then and not then
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Wilander
Keywords: EasyFix, GoodFirstBug
Depends on:
Reported: 2015-12-15 00:05 PST by youenn fablet
Modified: 2016-01-05 05:58 PST (History)
3 users (show)

See Also:

Patch (3.94 KB, patch)
2016-01-04 11:41 PST, John Wilander
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2015-12-15 00:05:01 PST
webkitGetUserMedia is implemented as a JS built-in.
As such it should not use public promise APIs which may be controlled by user scripts.
@then should be used instead
Comment 1 John Wilander 2016-01-04 11:41:32 PST
Created attachment 268209 [details]
Comment 2 Brent Fulgham 2016-01-04 12:48:54 PST
Comment on attachment 268209 [details]

Very nice! r=me.
Comment 3 WebKit Commit Bot 2016-01-04 13:38:36 PST
Comment on attachment 268209 [details]

Clearing flags on attachment: 268209

Committed r194554: <http://trac.webkit.org/changeset/194554>
Comment 4 WebKit Commit Bot 2016-01-04 13:38:39 PST
All reviewed patches have been landed.  Closing bug.
Comment 5 youenn fablet 2016-01-05 00:20:21 PST
Good to see this fixed :)

FWIW, there are similar bugs in Source/WebCore/Modules//mediastream/RTCPeerConnection.js and RTCPeerConnectionInternals.js.
I do not know how testable WebRTC is currently though.

On another point, I think the added test could be improved a bit.

It is generally good to have a expected.txt containing a simple message like "PASS" or "FAIL".
Libraries such as testharness.js or js-test-pre.js/js-test-post.js may help on that.
Using console.log(...) may not be js-test best friend. This may trigger more often rebasing of the expected.txt file whenever doing edits on the test.

Related specifically to that test, setting "{ audio: false, video: false }"  leads to automatic rejection of the promise without any user interaction.

The test could be improved as follow:
- Add a testResult variable, equal to "PASS" by default
- testResult is set to "FAIL" in successCallback and new Promise.prototype.then
- In errorCallback (and successCallback), output the value of testResult
- Call webkitGetUserMedia with "{ audio: false, video: false }"

Since callbacks are asynchronously called, the test needs to be turned into an asynchronous one and ended when either errorCallback or successCallback is called. Note that it is important to handle properly successCallback to ensure the test does not timeout in error case.
Comment 6 youenn fablet 2016-01-05 05:58:14 PST
Comment on attachment 268209 [details]

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

> LayoutTests/ChangeLog:9
> +        * streams/webkitGetUserMedia-shadowing-then.html: Added.

I forgot this one: the location of the test is probably not appropriate.
Could you move it from LayoutTests/streams to LayoutTests/fast/mediastream?