[webkit-reviews] review denied: [Bug 189262] Implement MediaStreamTrack Content Hints : [Attachment 350083] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has denied Wendy <yuhan_wu at apple.com>'s
request for review:
Bug 189262: Implement MediaStreamTrack Content Hints
https://bugs.webkit.org/show_bug.cgi?id=189262

Attachment 350083: Patch

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




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

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

> Source/WebCore/ChangeLog:30
> +	   (WebCore::MediaStreamTrackPrivate::contentHint const):

Change log has double entries. Also it is marked "No new tests" but I see that
you added some in this patch.

> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:100
> +bool MediaStreamTrack::setContentHint(const String& hintValue) const

Should probably be a String&& and use WTFMove().

> Source/WebCore/Modules/mediastream/MediaStreamTrack.h:73
> +    WEBCORE_EXPORT bool setContentHint(const String&) const;

Should not be const, no need for WEBCORE_EXPORT either.

> Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:99
> +bool MediaStreamTrackPrivate::setContentHint(const String& hintValue)

String&&

> Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:103
> +	       m_contentHint = hintValue;

m_contentHint = WTFMove(hintValue)
But, since contentHint is a limited set of values, we could use an enum maybe?

> LayoutTests/imported/w3c/ChangeLog:9
> +	   *
web-platform-tests/mediacapture-streams/MediaStreamTrack-contentHint.https.html
: Added.

Are these new tests or tests imported from upstream WPT?

> LayoutTests/imported/w3c/ChangeLog:52
> +	   https://bugs.webkit.org/show_bug.cgi?id=189262

This is probably the reason for the patch not applying.