[webkit-reviews] review granted: [Bug 173527] Merge MediaDevicesRequest and MediaDevicesEnumerationRequest to tighten up code and object lifetime : [Attachment 313249] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Darin Adler <darin at apple.com>'s request
for review:
Bug 173527: Merge MediaDevicesRequest and MediaDevicesEnumerationRequest to
tighten up code and object lifetime
https://bugs.webkit.org/show_bug.cgi?id=173527

Attachment 313249: Patch

https://bugs.webkit.org/attachment.cgi?id=313249&action=review




--- Comment #4 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 313249
  --> https://bugs.webkit.org/attachment.cgi?id=313249
Patch

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

> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:63
> +    auto* controller = UserMediaController::from(document.page());
> +    if (!controller) {
> +	   // FIXME: Should we resolve or reject the promise here instead of
leaving the website waiting?
> +	   return;
> +    }

The fact that UserMediaController::from can fail seems so opaque. What it is
really checking is if there is page.  It makes me think we should make
UserMediaController::from (and therefore Supplement::from) take a Page& and
return a Foo& make the caller check that page is not null instead of the foo.

> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:113
> +#if !PLATFORM(COCOA)
> +    UNUSED_PARAM(devices);
> +#else

This doesn't feel like a COCOA not COCOA divide, but rather a
ENABLE(ATYPICAL_DEVICE_FILTERING) thing.  Actually, it would be better if that
were a setting, all things considered, then it could be tested with different
values.  Thoughts for the future.

> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:139
> +    Vector<RefPtr<MediaDeviceInfo>> devices;

I would say this should be a Vector<> of Ref<MediaDeviceInfo>, but I would only
be impugning myself.  I really need to make Ref<> work with the bindings!

> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:150
> +	   if (id.isEmpty())
> +	       continue;

Seems like id could be computed first and this check could be done directly
after.

> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:152
> +	   devices.append(MediaDeviceInfo::create(&document, label, id,
groupId, deviceType));

Quite silly that MediaDeviceInfo::create takes the document by pointer.