[webkit-reviews] review granted: [Bug 192632] [MediaStream] Calculate width or height when constraints contain only the other : [Attachment 357174] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 192632: [MediaStream] Calculate width or height when constraints contain
only the other
https://bugs.webkit.org/show_bug.cgi?id=192632

Attachment 357174: Patch

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




--- Comment #8 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 357174
  --> https://bugs.webkit.org/attachment.cgi?id=357174
Patch

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

> Source/WebKit/ChangeLog:17
> +2018-12-12  Eric Carlson  <eric.carlson at apple.com>

Double entry

> Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:890
> +    m_intrinsicSize = size;

Should we check whether m_size != this->size() before calling
notifySettingsDidChangeObservers?

> Source/WebCore/platform/mediastream/RealtimeMediaSource.h:-123
> -    const IntSize& size() const { return m_size; }

Why stopping inlining?

> Source/WebCore/platform/mediastream/RealtimeMediaSource.h:124
> +    const IntSize intrinsicSize() const;

Could be inlined as well.

> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:395
> +	   if (!size.isEmpty() && size !=
expandedIntSize(sample.presentationSize())) {

We probably cannot have size being empty anymore since we should have an
intrinsic size.
Maybe we can remove the check, or add an ASSERT() if there is a risk for
converting to an empty size.

> Source/WebCore/platform/mediastream/mac/DisplayCaptureSourceCocoa.cpp:152
> +	   return intrinsicSize();

Simplify to if (frameSize.isEmpty())

> Source/WebCore/platform/mock/MockRealtimeVideoSource.h:73
> +    void setSizeAndFrameRateWithPreset(IntSize, double, RefPtr<VideoPreset>)
final;

RefPtr<VideoPreset>&& would be better.

> Source/WebKit/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp:104
> +#if HAVE(IOSURFACE)

W could remove "#if HAVE(IOSURFACE)" here and make RemoteVideoSample::create
return null if HAVE(IOSURFACE) is not defined

> Source/WebKit/WebProcess/cocoa/UserMediaCaptureManager.cpp:139
> +	   if (!videoSampleSize.width() && !videoSampleSize.height())

if (videoSampleSize.isZero())

> Source/WebKit/WebProcess/cocoa/UserMediaCaptureManager.cpp:141
> +	   else if (!videoSampleSize.width() || !videoSampleSize.height()) {

else if (!videoSampleSize.width())
    videoSampleSize.setWidth
else if (!videoSampleSize.height())
    videoSampleSize.setHeight