Bug 169216 - Expose WebRTC current/pending description getters
Summary: Expose WebRTC current/pending description getters
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-06 14:12 PST by youenn fablet
Modified: 2017-03-07 11:24 PST (History)
5 users (show)

See Also:


Attachments
Patch (11.76 KB, patch)
2017-03-07 08:19 PST, youenn fablet
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 2017-03-06 14:12:58 PST
Following on bug 168234, we should tie current/pending description getters to libwebrtc
Comment 1 youenn fablet 2017-03-07 08:19:06 PST
Created attachment 303654 [details]
Patch
Comment 2 Alex Christensen 2017-03-07 08:27:11 PST
Comment on attachment 303654 [details]
Patch

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

> LayoutTests/webrtc/descriptionGetters-expected.txt:2
> +PASS description getters when changing description from video to video & data channel 

I think we should print the description in the results so we can see what the description looks like and so we can see if we accidentally change the description format or something.
Comment 3 youenn fablet 2017-03-07 08:35:09 PST
The description will change all the time.
We could print out a sanitized SDP though.
We could also reuse the SDP parser from WebCore and add directly in the tests some SDP field checks.
Comment 4 youenn fablet 2017-03-07 08:42:17 PST
The SDP parser is at Source/WebCore/Modules/mediastream/sdp.js
It is a good idea to add some libwebrtc-specifc tests that will check more deeply the different parameters. I'll add some in LayoutTests/webrtc/libwebrtc
Comment 5 WebKit Commit Bot 2017-03-07 09:15:10 PST
Comment on attachment 303654 [details]
Patch

Clearing flags on attachment: 303654

Committed r213520: <http://trac.webkit.org/changeset/213520>
Comment 6 WebKit Commit Bot 2017-03-07 09:15:13 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 Carlos Alberto Lopez Perez 2017-03-07 10:46:38 PST
The test webrtc/descriptionGetters.html added on r213520 it is failing on GTK+ (We use OpenWebRTC instead of libwebrtc), and it seems this test is created to match the behaviour of the the current implementation based on libwebrtc.


On GTK+ we are skipping all tests under webrtc/libwebrtc.

So if when you add a test for something that depends on libwebrtc, and you add it under webrtc/libwebrtc, then on the GTK+ port we won't run into new unexpected failures.


So.. do you think its a good idea if we move this test from webrtc/descriptionGetters.html to webrtc/libwebrtc/descriptionGetters.html ?
Comment 8 youenn fablet 2017-03-07 10:51:47 PST
(In reply to comment #7)
> The test webrtc/descriptionGetters.html added on r213520 it is failing on
> GTK+ (We use OpenWebRTC instead of libwebrtc), and it seems this test is
> created to match the behaviour of the the current implementation based on
> libwebrtc.

Ah right, it might be better to put it in libwebrtc for now.
At the end of the day, we want that very same test, but matching the spec, and working with both libwebrtc and OpenWebRTC :)

> So.. do you think its a good idea if we move this test from
> webrtc/descriptionGetters.html to webrtc/libwebrtc/descriptionGetters.html ?

yep
Comment 9 Carlos Alberto Lopez Perez 2017-03-07 11:06:58 PST
(In reply to comment #8)
> (In reply to comment #7)
> > The test webrtc/descriptionGetters.html added on r213520 it is failing on
> > GTK+ (We use OpenWebRTC instead of libwebrtc), and it seems this test is
> > created to match the behaviour of the the current implementation based on
> > libwebrtc.
> 
> Ah right, it might be better to put it in libwebrtc for now.
> At the end of the day, we want that very same test, but matching the spec,
> and working with both libwebrtc and OpenWebRTC :)
> 

Yes :)

> > So.. do you think its a good idea if we move this test from
> > webrtc/descriptionGetters.html to webrtc/libwebrtc/descriptionGetters.html ?
> 
> yep

Ok.. I will move it now. Thanks!
Comment 10 Carlos Alberto Lopez Perez 2017-03-07 11:16:20 PST
Committed r213526: <http://trac.webkit.org/changeset/213526>
Comment 11 youenn fablet 2017-03-07 11:24:24 PST
Thanks!