Bug 149527 - Clean up user media request internal API
Summary: Clean up user media request internal API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-09-24 09:51 PDT by Eric Carlson
Modified: 2015-09-25 19:23 PDT (History)
2 users (show)

See Also:


Attachments
Proposed patch. (32.71 KB, patch)
2015-09-24 12:12 PDT, Eric Carlson
thorton: review+
Details | Formatted Diff | Diff
Updated patch. (37.56 KB, patch)
2015-09-24 14:10 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Another updated patch (36.80 KB, patch)
2015-09-24 14:53 PDT, Eric Carlson
commit-queue: commit-queue-
Details | Formatted Diff | Diff
YAUP (40.71 KB, patch)
2015-09-25 08:56 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Another try at updating the GTK build for the changes. (41.08 KB, patch)
2015-09-25 13:37 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
One more time... (41.46 KB, patch)
2015-09-25 14:54 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
One more time - redux (43.13 KB, patch)
2015-09-25 18:46 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2015-09-24 09:51:18 PDT
Clean up user media request internal API
Comment 1 Radar WebKit Bug Importer 2015-09-24 09:51:59 PDT
<rdar://problem/22839956>
Comment 2 Eric Carlson 2015-09-24 12:12:36 PDT
Created attachment 261884 [details]
Proposed patch.
Comment 3 Tim Horton 2015-09-24 12:36:41 PDT
Comment on attachment 261884 [details]
Proposed patch.

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

> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:128
> +        m_audioDeviceUIDs.append(audioTrack->persistentId());

while you're changing this, you should capitalize the D!

> Source/WebCore/Modules/mediastream/UserMediaRequest.h:96
> +    String m_videoDeviceUIDAllowed;

something that ends in -allowed sounds like it would be a bool. should this be m_allowedVideoDeviceUID?

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:41
> +PassRefPtr<UserMediaPermissionRequestProxy> UserMediaPermissionRequestManagerProxy::createRequest(uint64_t userMediaID, const Vector<String>& deviceUIDsAudio, const Vector<String>& deviceUIDsVideo)

what are the 's'es for? should this be audioDeviceUIDs?
Comment 4 Eric Carlson 2015-09-24 12:43:42 PDT
(In reply to comment #3)
> Comment on attachment 261884 [details]
> Proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=261884&action=review
> 
> > Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:128
> > +        m_audioDeviceUIDs.append(audioTrack->persistentId());
> 
> while you're changing this, you should capitalize the D!
> 

Fixed.

> > Source/WebCore/Modules/mediastream/UserMediaRequest.h:96
> > +    String m_videoDeviceUIDAllowed;
> 
> something that ends in -allowed sounds like it would be a bool. should this
> be m_allowedVideoDeviceUID?
> 

Hmm, then it would match the allowedVideoDeviceUID() accessor too!

Fixed.

> > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:41
> > +PassRefPtr<UserMediaPermissionRequestProxy> UserMediaPermissionRequestManagerProxy::createRequest(uint64_t userMediaID, const Vector<String>& deviceUIDsAudio, const Vector<String>& deviceUIDsVideo)
> 
> what are the 's'es for? should this be audioDeviceUIDs?

Fixed.
Comment 5 Eric Carlson 2015-09-24 14:10:39 PDT
Created attachment 261895 [details]
Updated patch.
Comment 6 Eric Carlson 2015-09-24 14:53:18 PDT
Created attachment 261896 [details]
Another updated patch
Comment 7 WebKit Commit Bot 2015-09-24 16:11:11 PDT
Comment on attachment 261896 [details]
Another updated patch

Rejecting attachment 261896 [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', 'validate-changelog', '--check-oops', '--non-interactive', 261896, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Tim Thorton found in /Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog does not appear to be a valid reviewer according to contributors.json.
/Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.webkit.org/results/209499
Comment 8 Eric Carlson 2015-09-25 08:56:54 PDT
Created attachment 261924 [details]
YAUP
Comment 9 Eric Carlson 2015-09-25 13:37:54 PDT
Created attachment 261935 [details]
Another try at updating the GTK build for the changes.
Comment 10 Eric Carlson 2015-09-25 14:54:19 PDT
Created attachment 261944 [details]
One more time...
Comment 11 Eric Carlson 2015-09-25 18:46:44 PDT
Created attachment 261959 [details]
One more time - redux
Comment 12 WebKit Commit Bot 2015-09-25 19:23:22 PDT
Comment on attachment 261959 [details]
One more time - redux

Clearing flags on attachment: 261959

Committed r190262: <http://trac.webkit.org/changeset/190262>
Comment 13 WebKit Commit Bot 2015-09-25 19:23:26 PDT
All reviewed patches have been landed.  Closing bug.