For example new RTCIceCandidate({ candidate: "foo", sdpMid: null }) succeeds and sdpMid becomes the string "null".
Created attachment 281562 [details] Proposed patch
Comment on attachment 281562 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=281562&action=review > Source/WebCore/ChangeLog:8 > + Prevent explicit null and undefined values to be converted to "null" and "undefined" strings. Nit: "values to be converted to" -> "values from being converted to"
Comment on attachment 281562 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=281562&action=review > Source/WebCore/Modules/mediastream/RTCIceCandidate.cpp:59 > + if (dictionary.getWithUndefinedOrNullCheck("sdpMLineIndex", sdpMLineIndexString)) { > + if (!sdpMLineIndexString.isNull()) { I think you can join the 2 ifs in just one with an and condition. Also check if getWithUndefinedOrNullCheck returns false if undefined or null, maybe we do not need the second condition.
Created attachment 281642 [details] Updated patch
(In reply to comment #2) > Comment on attachment 281562 [details] > Proposed patch Thanks for the reviews > View in context: > https://bugs.webkit.org/attachment.cgi?id=281562&action=review > > > Source/WebCore/ChangeLog:8 > > + Prevent explicit null and undefined values to be converted to "null" and "undefined" strings. > > Nit: "values to be converted to" -> "values from being converted to" Fixed. (In reply to comment #3) > Comment on attachment 281562 [details] > Proposed patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=281562&action=review > > > Source/WebCore/Modules/mediastream/RTCIceCandidate.cpp:59 > > + if (dictionary.getWithUndefinedOrNullCheck("sdpMLineIndex", sdpMLineIndexString)) { > > + if (!sdpMLineIndexString.isNull()) { > > I think you can join the 2 ifs in just one with an and condition. Also check > if getWithUndefinedOrNullCheck returns false if undefined or null, maybe we > do not need the second condition. Dug a bit deeper and getWithUndefinedOrNullCheck() returns false if the dictionary member was undefined or false; rendering the second check is unnecessary. Fixed.
Comment on attachment 281642 [details] Updated patch LGTM, required to handle the null values correctly for this nullable attributes.
Comment on attachment 281642 [details] Updated patch Thanks Alex and Eric!
Comment on attachment 281642 [details] Updated patch Rejecting attachment 281642 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 281642, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: LayoutTests :040000 040000 53d9c5daad840e7c8d4b08e899f964d25323f0c8 f3f1d4fa1265a983a518943e7e3a0f794477e6bd M Source Current branch master is up to date. ERROR: Not all changes have been committed into SVN, however the committed ones (if any) seem to be successfully integrated into the working tree. Please see the above messages for details. Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Updating OpenSource Current branch master is up to date. Full output: http://webkit-queues.webkit.org/results/1536289
Comment on attachment 281642 [details] Updated patch Let's try cq again (now that it's fixed)
Comment on attachment 281642 [details] Updated patch Clearing flags on attachment: 281642 Committed r202267: <http://trac.webkit.org/changeset/202267>
All reviewed patches have been landed. Closing bug.