[webkit-reviews] review granted: [Bug 187850] [MediaCapabilities] Platform integration : [Attachment 346480] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 187850: [MediaCapabilities] Platform integration
https://bugs.webkit.org/show_bug.cgi?id=187850

Attachment 346480: Patch

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




--- Comment #8 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 346480
  --> https://bugs.webkit.org/attachment.cgi?id=346480
Patch

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

> Source/WebCore/Modules/mediacapabilities/MediaCapabilities.cpp:202
> +	   MediaEngineConfigurationFactory::DecodingConfigurationCallback
callback = [promise = WTFMove(promise)]
(RefPtr<MediaEngineDecodingConfiguration> engineConfiguration) mutable {

Nit: I think you can use "auto" here.

> Source/WebCore/Modules/mediacapabilities/MediaCapabilities.cpp:252
> +	   MediaEngineConfigurationFactory::EncodingConfigurationCallback
callback = [promise = WTFMove(promise)]
(RefPtr<MediaEngineEncodingConfiguration> engineConfiguration) mutable {

Ditto.

> Source/WebCore/platform/mediacapabilities/MediaEngineConfiguration.cpp:56
> +    double numerator = frameratePieces[0].toDouble(&ok);
> +    if (!ok)
> +	   return;
> +
> +    double denominator = frameratePieces[1].toDouble(&ok);
> +    if (!ok)
> +	   return;

You should also fail if numerator or denominator is zero.

> Source/WebCore/platform/mock/MediaEngineDecodingConfigurationMock.cpp:43
> +    RefPtr<MediaEngineVideoConfiguration> videoConfig =
videoConfiguration();

Nit: you can use "auto" here.

> Source/WebCore/platform/mock/MediaEngineDecodingConfigurationMock.cpp:51
> +    RefPtr<MediaEngineAudioConfiguration> audioConfig =
audioConfiguration();

Ditto.

> Source/WebCore/platform/mock/MediaEngineDecodingConfigurationMock.cpp:60
> +    RefPtr<MediaEngineVideoConfiguration> videoConfig =
videoConfiguration();

Ditto.

> Source/WebCore/platform/mock/MediaEngineDecodingConfigurationMock.cpp:66
> +    RefPtr<MediaEngineAudioConfiguration> audioConfig =
audioConfiguration();

Ditto.

> Source/WebCore/platform/mock/MediaEngineDecodingConfigurationMock.cpp:75
> +    RefPtr<MediaEngineVideoConfiguration> videoConfig =
videoConfiguration();

Ditto.

> Source/WebCore/platform/mock/MediaEngineDecodingConfigurationMock.cpp:81
> +    RefPtr<MediaEngineAudioConfiguration> audioConfig =
audioConfiguration();

Ditto.

> Source/WebCore/platform/mock/MediaEngineEncodingConfigurationMock.cpp:43
> +    RefPtr<MediaEngineVideoConfiguration> videoConfig =
videoConfiguration();

Ditto.

> Source/WebCore/platform/mock/MediaEngineEncodingConfigurationMock.cpp:51
> +    RefPtr<MediaEngineAudioConfiguration> audioConfig =
audioConfiguration();

Ditto.

> Source/WebCore/platform/mock/MediaEngineEncodingConfigurationMock.cpp:60
> +    RefPtr<MediaEngineVideoConfiguration> videoConfig =
videoConfiguration();

Ditto.

> Source/WebCore/platform/mock/MediaEngineEncodingConfigurationMock.cpp:62
> +    if (videoConfig) {
> +	   if (videoConfig->framerate() > 30)

Nit: these two lines can be collapsed.

> Source/WebCore/platform/mock/MediaEngineEncodingConfigurationMock.cpp:66
> +    RefPtr<MediaEngineAudioConfiguration> audioConfig =
audioConfiguration();

"auto".

> Source/WebCore/platform/mock/MediaEngineEncodingConfigurationMock.cpp:77
> +    RefPtr<MediaEngineVideoConfiguration> videoConfig =
videoConfiguration();
> +    if (videoConfig) {
> +	   if (videoConfig->contentType().containerType() != "video/mp4")

Ditto the above.

> Source/WebCore/platform/mock/MediaEngineEncodingConfigurationMock.cpp:81
> +    RefPtr<MediaEngineAudioConfiguration> audioConfig =
audioConfiguration();

"auto".