[webkit-reviews] review denied: [Bug 189262] Implement MediaStreamTrack Content Hints : [Attachment 350484] 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 350484: Patch

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




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

This patch is going in the right direction.
Some comments below.
-expected.txt file needs to be fixed.

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

> Source/WebCore/ChangeLog:9
> +	   No new tests (OOPS!).

Please explain that the changes are covered by rebased tests.
It would also be good to explain below your changes, for instance that
contentHint is stored in MediaStreamTrackPrivate and add a reference to the
spec.

> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:103
> +    MediaStreamTrackPrivate::HintValue val = m_private->contentHint();

You can use auto here instead of MediaStreamTrackPrivate::HintValue.
And replace val by value.
Given it is only used in the switch statement, I would simply write switch
(m_private->contentHint())

> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:104
> +    switch (val) {

According WebKit style, the case statement should be indented like the switch
statement.
Please remove 4 spaces to all lines below.

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

s/&&/&/

> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:124
> +    MediaStreamTrackPrivate::HintValue val;

s/val/value/

> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:136
> +	   val = MediaStreamTrackPrivate::HintValue::Text;

What happens if hintValue is not any of these value?

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

bool value does not seem to be used anywhere.

> Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:111
> +    }

I would move these checks in MediaStreamTrack::setContentHint and here it would
just be a simple setter.

>
LayoutTests/imported/w3c/web-platform-tests/mst-content-hint/MediaStreamTrack-c
ontentHint-expected.txt:-4
> -FAIL Audio tracks ignore invalid/video contentHints assert_equals: Audio
tracks should ignore video-only contentHints. expected "speech" but got
"motion"

It seems bots do not agree with that expected.txt.