Bug 165736 - Remove uses of Dictionary in WebRTC IDL files
Summary: Remove uses of Dictionary in WebRTC IDL files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Local Build
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on: 165367
Blocks:
  Show dependency treegraph
 
Reported: 2016-12-10 15:14 PST by Darin Adler
Modified: 2016-12-11 18:50 PST (History)
6 users (show)

See Also:


Attachments
Patch (117.72 KB, patch)
2016-12-10 17:21 PST, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (118.35 KB, patch)
2016-12-10 18:01 PST, Darin Adler
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-yosemite (862.06 KB, application/zip)
2016-12-10 19:08 PST, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (1.10 MB, application/zip)
2016-12-10 19:11 PST, Build Bot
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (14.09 MB, application/zip)
2016-12-10 19:21 PST, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-yosemite (1.57 MB, application/zip)
2016-12-10 21:18 PST, Build Bot
no flags Details
Patch (127.21 KB, patch)
2016-12-11 06:06 PST, Darin Adler
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (1.26 MB, application/zip)
2016-12-11 07:16 PST, Build Bot
no flags Details
Patch (152.61 KB, patch)
2016-12-11 10:30 PST, Darin Adler
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2016-12-10 15:14:14 PST
Got this partly done.
Comment 1 Darin Adler 2016-12-10 17:21:33 PST
Created attachment 296833 [details]
Patch
Comment 2 Darin Adler 2016-12-10 18:01:23 PST
Created attachment 296835 [details]
Patch
Comment 3 Build Bot 2016-12-10 19:08:27 PST
Comment on attachment 296835 [details]
Patch

Attachment 296835 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2692776

New failing tests:
imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/rtcpeerconnection-constructor.html
Comment 4 Build Bot 2016-12-10 19:08:32 PST
Created attachment 296840 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 5 Build Bot 2016-12-10 19:11:35 PST
Comment on attachment 296835 [details]
Patch

Attachment 296835 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2692783

New failing tests:
fast/mediastream/RTCSessionDescription.html
fast/mediastream/RTCPeerConnection-getConfiguration.html
fast/mediastream/RTCIceCandidate.html
imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/rtcpeerconnection-constructor.html
fast/mediastream/RTCPeerConnection.html
Comment 6 Build Bot 2016-12-10 19:11:39 PST
Created attachment 296841 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 7 Build Bot 2016-12-10 19:21:55 PST
Comment on attachment 296835 [details]
Patch

Attachment 296835 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2692788

New failing tests:
imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/rtcpeerconnection-constructor.html
Comment 8 Build Bot 2016-12-10 19:21:58 PST
Created attachment 296842 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 9 Build Bot 2016-12-10 21:17:58 PST
Comment on attachment 296835 [details]
Patch

Attachment 296835 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2693492

New failing tests:
imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/rtcpeerconnection-constructor.html
Comment 10 Build Bot 2016-12-10 21:18:01 PST
Created attachment 296848 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 11 Darin Adler 2016-12-11 06:06:52 PST
Created attachment 296859 [details]
Patch
Comment 12 Build Bot 2016-12-11 07:16:43 PST
Comment on attachment 296859 [details]
Patch

Attachment 296859 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2697000

New failing tests:
fast/mediastream/RTCSessionDescription.html
fast/mediastream/RTCPeerConnection-getConfiguration.html
fast/mediastream/RTCIceCandidate.html
fast/mediastream/RTCPeerConnection.html
Comment 13 Build Bot 2016-12-11 07:16:48 PST
Created attachment 296862 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 14 Sam Weinig 2016-12-11 09:34:09 PST
Comment on attachment 296859 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1364
> +                    # FIXME: Replace with a more general solution, presumably using C++ code in JSDOMConvert rather than more special cases here.
> +                    my $nullCheck = "dictionary.${key}";
> +                    my $value = "*dictionary.${key}";
> +                    if ($codeGenerator->IsStringType($member->type)) {
> +                        $nullCheck = "!dictionary.${key}.isNull()";
> +                        $value = "dictionary.${key}";
> +                    }
> +                    $result .= "    if ($nullCheck) {\n";
> +                    $result .= "        auto ${key}Value = toJS<$IDLType>(state, globalObject, $value);\n";

The general solution is:

                    $result .= "    if (!${IDLType}::isNullValue(dictionary.${key})) {\n";
                    $result .= "        auto ${key}Value = toJS<${IDLType}>(state, globalObject, ${IDLType}::extractValueFromNullable(dictionary.${key}));\n";
Comment 15 Darin Adler 2016-12-11 09:44:33 PST
(In reply to comment #14)
> The general solution is:

Thanks. I will switch to that.
Comment 16 Darin Adler 2016-12-11 10:30:50 PST
Created attachment 296869 [details]
Patch
Comment 17 Darin Adler 2016-12-11 15:48:21 PST
I think this patch is done. The failure on GTk seems to be something wrong with the bot.
Comment 18 Sam Weinig 2016-12-11 18:03:23 PST
Comment on attachment 296869 [details]
Patch

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

> Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:62
> +using MediaDescriptionVector = Vector<PeerMediaDescription>;
> +using RtpTransceiverVector = Vector<RefPtr<RTCRtpTransceiver>>;

I'm not generally a fan of these type aliases that just shorten the type name of a Vector, not sure they add much, but I see you are just moving them.
Comment 19 Darin Adler 2016-12-11 18:23:47 PST
Comment on attachment 296869 [details]
Patch

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

>> Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:62
>> +using RtpTransceiverVector = Vector<RefPtr<RTCRtpTransceiver>>;
> 
> I'm not generally a fan of these type aliases that just shorten the type name of a Vector, not sure they add much, but I see you are just moving them.

In a lot of my recent patches I have been removing these entirely. In this case I was just happy to get them out of the header and didn’t follow through to see how often they were used to get rid of them completely.
Comment 20 WebKit Commit Bot 2016-12-11 18:50:39 PST
Comment on attachment 296869 [details]
Patch

Clearing flags on attachment: 296869

Committed r209695: <http://trac.webkit.org/changeset/209695>
Comment 21 WebKit Commit Bot 2016-12-11 18:50:45 PST
All reviewed patches have been landed.  Closing bug.