webkitGetUserMedia built-in should use @then and not then

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

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

[John Wilander]

Created attachment 268209 [details]
Patch

[Brent Fulgham]

Comment on attachment 268209 [details]
Patch

Very nice! r=me.

[WebKit Commit Bot]

Comment on attachment 268209 [details]
Patch

Clearing flags on attachment: 268209

Committed r194554: <http://trac.webkit.org/changeset/194554>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

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.

[youenn fablet]

Comment on attachment 268209 [details]
Patch

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?