Bug 80699 - MediaStream API (JSEP): Introducing IceCandidate
Summary: MediaStream API (JSEP): Introducing IceCandidate
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Tommy Widenflycht
URL:
Keywords:
Depends on:
Blocks: 80589
  Show dependency treegraph
 
Reported: 2012-03-09 06:28 PST by Tommy Widenflycht
Modified: 2012-03-14 12:53 PDT (History)
6 users (show)

See Also:


Attachments
Patch (20.30 KB, patch)
2012-03-09 06:35 PST, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (20.85 KB, patch)
2012-03-12 03:07 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (20.85 KB, patch)
2012-03-13 06:37 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (20.93 KB, patch)
2012-03-14 02:52 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tommy Widenflycht 2012-03-09 06:28:54 PST
Patch #2 in a series of patches to change the PeerConnection from ROAP to JSEP, see bug 80589 for more information.
Adding the JS object IceCandidate and its WebCore/platform sibling IceCandidateDescriptor. This object will be created both from JS and the embedder.
Comment 1 Tommy Widenflycht 2012-03-09 06:35:51 PST
Created attachment 131035 [details]
Patch
Comment 2 Gustavo Noronha (kov) 2012-03-09 08:06:54 PST
Comment on attachment 131035 [details]
Patch

Attachment 131035 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11906598
Comment 3 Adam Barth 2012-03-09 09:41:38 PST
Comment on attachment 131035 [details]
Patch

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

> Source/WebCore/Modules/mediastream/IceCandidate.h:54
> +    String label();
> +    String candidateLine();
> +
> +    String toSdp();

Should these be "const String&" ?

> Source/WebCore/platform/mediastream/IceCandidateDescriptor.h:48
> +    String label() { return m_label; }
> +    String candidateLine() { return m_candidateLine; }

const String&
Comment 4 Adam Barth 2012-03-09 09:42:47 PST
Comment on attachment 131035 [details]
Patch

Looks like you've got build problem on GTK.  This patch generally looks fine, but I'd like to resolve Bug 80692 (or at least come to agreement there with Adam) before we start landing these patches.
Comment 5 Tommy Widenflycht 2012-03-12 03:07:08 PDT
Created attachment 131306 [details]
Patch
Comment 6 Tommy Widenflycht 2012-03-12 03:08:55 PDT
Comment on attachment 131035 [details]
Patch

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

>> Source/WebCore/Modules/mediastream/IceCandidate.h:54
>> +    String toSdp();
> 
> Should these be "const String&" ?

toSdp will return a generated string and therefore can't return a const String& but the two accessors have been fixed.

>> Source/WebCore/platform/mediastream/IceCandidateDescriptor.h:48
>> +    String candidateLine() { return m_candidateLine; }
> 
> const String&

Fixed.
Comment 7 Tommy Widenflycht 2012-03-12 03:10:37 PDT
Sure thing, just uploaded an uploaded an patch to see if I fixed GTK (which btw I can't build either on my Ubuntu box or Mac for some strange reason)

(In reply to comment #4)
> (From update of attachment 131035 [details])
> Looks like you've got build problem on GTK.  This patch generally looks fine, but I'd like to resolve Bug 80692 (or at least come to agreement there with Adam) before we start landing these patches.
Comment 8 Adam Barth 2012-03-12 11:31:28 PDT
Comment on attachment 131306 [details]
Patch

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

> Source/WebCore/platform/mediastream/IceCandidateDescriptor.cpp:58
> +    return MediaStreamCenter::instance().doConstructSdp(this);

doConstructSdp -> constructSdp (WebKit doesn't use the prefix "do".)
Comment 9 Adam Barth 2012-03-12 11:32:31 PDT
We can move forward with these patches why we give Adam a chance to respond on Bug 80692 as long as we don't have a dependency on the final names for the PeerConnection object.
Comment 10 Tommy Widenflycht 2012-03-13 06:37:43 PDT
Created attachment 131602 [details]
Patch
Comment 11 Tommy Widenflycht 2012-03-13 06:38:58 PDT
Comment on attachment 131306 [details]
Patch

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

>> Source/WebCore/platform/mediastream/IceCandidateDescriptor.cpp:58
>> +    return MediaStreamCenter::instance().doConstructSdp(this);
> 
> doConstructSdp -> constructSdp (WebKit doesn't use the prefix "do".)

Fixed.
Comment 12 Adam Barth 2012-03-13 10:38:05 PDT
Thanks.
Comment 13 WebKit Review Bot 2012-03-13 12:14:15 PDT
Comment on attachment 131602 [details]
Patch

Rejecting attachment 131602 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
 to file Source/WebCore/WebCore.gypi.rej
patching file Source/WebCore/platform/mediastream/IceCandidateDescriptor.cpp
patching file Source/WebCore/platform/mediastream/IceCandidateDescriptor.h
patching file Source/WebCore/platform/mediastream/MediaStreamCenter.cpp
patching file Source/WebCore/platform/mediastream/MediaStreamCenter.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Adam Barth']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/11942657
Comment 14 Tommy Widenflycht 2012-03-14 02:52:17 PDT
Created attachment 131815 [details]
Patch
Comment 15 Tommy Widenflycht 2012-03-14 02:53:37 PDT
Just merge fixes of the makefiles, no code changes att all.
Comment 16 WebKit Review Bot 2012-03-14 12:53:41 PDT
Comment on attachment 131815 [details]
Patch

Clearing flags on attachment: 131815

Committed r110735: <http://trac.webkit.org/changeset/110735>
Comment 17 WebKit Review Bot 2012-03-14 12:53:47 PDT
All reviewed patches have been landed.  Closing bug.