Bug 95565

Summary: MediaStream API: Add Ice-related functionality to RTCPeerConnection
Product: WebKit Reporter: Tommy Widenflycht <tommyw>
Component: WebCore Misc.Assignee: Tommy Widenflycht <tommyw>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, eric.carlson, feature-media-reviews, fishd, gyuyoung.kim, jamesr, ojan, rakuco, tkent+wkapi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 80589    
Attachments:
Description Flags
Patch
none
Patch none

Description Tommy Widenflycht 2012-08-31 06:29:12 PDT
Contains the entire slice of functionality.
Comment 1 Tommy Widenflycht 2012-08-31 06:40:38 PDT
Created attachment 161688 [details]
Patch
Comment 2 WebKit Review Bot 2012-08-31 06:43:16 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Adam Barth 2012-09-01 00:42:53 PDT
Comment on attachment 161688 [details]
Patch

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

> Source/WebCore/Modules/mediastream/RTCIceCandidateEvent.idl:30
> +       readonly attribute RTCIceCandidate candidate;

Bad indent.  I think we have this in a bunch of event interfaces.

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:149
> +void RTCPeerConnection::addIceCandidate(PassRefPtr<RTCIceCandidate> prpIceCandidate, ExceptionCode& ec)

It's not clear to me why this function need to take a reference to the RTCIceCandidate

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:190
> +        return "new";

ASCIILiteral
Comment 4 WebKit Review Bot 2012-09-01 01:05:24 PDT
Comment on attachment 161688 [details]
Patch

Rejecting attachment 161688 [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:
Kit/chromium/third_party/yasm/source/patched-yasm --revision 134927 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
50>At revision 134927.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/13713530
Comment 5 Adam Barth 2012-09-01 01:09:45 PDT
Hunk #1 FAILED at 44.
1 out of 1 hunk FAILED -- saving rejects to file Tools/DumpRenderTree/chromium/MockWebRTCPeerConnectionHandler.h.rej
Comment 6 Tommy Widenflycht 2012-09-03 03:53:07 PDT
Comment on attachment 161688 [details]
Patch

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

>> Source/WebCore/Modules/mediastream/RTCIceCandidateEvent.idl:30
>> +       readonly attribute RTCIceCandidate candidate;
> 
> Bad indent.  I think we have this in a bunch of event interfaces.

Fixed.

>> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:149
>> +void RTCPeerConnection::addIceCandidate(PassRefPtr<RTCIceCandidate> prpIceCandidate, ExceptionCode& ec)
> 
> It's not clear to me why this function need to take a reference to the RTCIceCandidate

You are right. Fixed.

>> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:190
>> +        return "new";
> 
> ASCIILiteral

Done.
Comment 7 Tommy Widenflycht 2012-09-03 03:57:49 PDT
Created attachment 161894 [details]
Patch
Comment 8 WebKit Review Bot 2012-09-03 05:17:12 PDT
Comment on attachment 161894 [details]
Patch

Attachment 161894 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13753046

New failing tests:
http/tests/cache/subresource-expiration-1.html
http/tests/cache/stopped-revalidation.html
Comment 9 Tommy Widenflycht 2012-09-03 05:33:13 PDT
cr-linux LayoutTest failure is unrelated to my patch.
Comment 10 Adam Barth 2012-09-03 08:40:51 PDT
Comment on attachment 161894 [details]
Patch

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

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:186
> -    return "";
> +    return ASCIILiteral("");

You don't actually need this here.  The benefit of ASCIILiteral is that it avoid a strlen, a malloc, and a memcpy, none of which happen for the empty string.  :)
Comment 11 WebKit Review Bot 2012-09-03 09:07:46 PDT
Comment on attachment 161894 [details]
Patch

Clearing flags on attachment: 161894

Committed r127425: <http://trac.webkit.org/changeset/127425>
Comment 12 WebKit Review Bot 2012-09-03 09:07:51 PDT
All reviewed patches have been landed.  Closing bug.