Bug 97540

Created attachment 165564 [details] Patch

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 on attachment 165564 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165564&action=review > Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:59 > + bool flag = false; flag = ? I guess the name ok is already taken... > Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:83 > + RefPtr<UserMediaRequest> request = adoptRef(new UserMediaRequest(context, controller, audio.release(), video.release(), successCallback, errorCallback)); > + > return request.release(); You can merge these lines and delete the variable |request| if you like. > Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:102 > + return (bool)m_audio; static_cast<bool>(m_audio) WebKit uses C++ style casts. Actually, is that even necessary? Does the compiler complain if you leave off the explicit cast?

Comment on attachment 165564 [details] Patch Tommy, would you be willing to fix these nits in a followup patch? I see that you're already asleep and I want to save you the timezone round-trip.

Comment on attachment 165564 [details] Patch Clearing flags on attachment: 165564 Committed r129517: <http://trac.webkit.org/changeset/129517>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by 97582

Comment on attachment 165564 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165564&action=review >> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:59 >> + bool flag = false; > > flag = ? I guess the name ok is already taken... Changed names to make more sense. "name" -> "mediaType" and "flag" -> "mediaRequested". Here I am checking if the mediaType is a Dictionary or a boolean, and extracting the correct value. >> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:83 >> return request.release(); > > You can merge these lines and delete the variable |request| if you like. Done. >> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:102 >> + return (bool)m_audio; > > static_cast<bool>(m_audio) > > WebKit uses C++ style casts. > > Actually, is that even necessary? Does the compiler complain if you leave off the explicit cast? I have not used c-style casts for ages and recently I started adding them, sigh. I'm sorry. You are right, there is no need for a cast at all here.

(In reply to comment #7) > Re-opened since this is blocked by 97582 Caused by a missing WEBKIT_EXPORT in WebMediaConstraints.h. Fixed.

Created attachment 165746 [details] Patch

Comment on attachment 165746 [details] Patch Attachment 165746 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14032377 New failing tests: http/tests/workers/terminate-during-sync-operation.html

Since the Mac build hasn't enabled MEDIA_STREAMS I am pretty sure my patch has nothing to do with this failure. (In reply to comment #11) > (From update of attachment 165746 [details]) > Attachment 165746 [details] did not pass mac-ews (mac): > Output: http://queues.webkit.org/results/14032377 > > New failing tests: > http/tests/workers/terminate-during-sync-operation.html

> Since the Mac build hasn't enabled MEDIA_STREAMS I am pretty sure my patch has nothing to do with this failure. Yeah, running the tests on mac is new. I think that's just a flaky test.

Comment on attachment 165746 [details] Patch Rejecting attachment 165746 [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 154708 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' 51>At revision 154708. ________ 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/14036464

Created attachment 165954 [details] Patch for landing

Comment on attachment 165954 [details] Patch for landing Clearing flags on attachment: 165954 Committed r129749: <http://trac.webkit.org/changeset/129749>