Implement MediaStreamTrack Content Hints https://w3c.github.io/mst-content-hint/
<rdar://problem/44101773>
Created attachment 350083 [details] Patch
Comment on attachment 350083 [details] 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.
Created attachment 350291 [details] Patch
Created attachment 350292 [details] Patch
Comment on attachment 350292 [details] Patch This patch needs to be rebaselined on top of trunk for it to succeed on EWS.
Created attachment 350407 [details] Patch
Attachment 350407 [details] did not pass style-queue: ERROR: LayoutTests/imported/w3c/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 350459 [details] Patch
Created attachment 350460 [details] Patch
Created attachment 350484 [details] Patch
Attachment 350484 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.h:71: The parameter name "hintValue" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:105: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] ERROR: Source/WebCore/ChangeLog:9: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 3 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 350484 [details] Patch Attachment 350484 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9307459 New failing tests: imported/w3c/web-platform-tests/mst-content-hint/MediaStreamTrack-contentHint.html
Created attachment 350490 [details] Archive of layout-test-results from ews103 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 350484 [details] Patch Attachment 350484 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9307483 New failing tests: imported/w3c/web-platform-tests/mst-content-hint/MediaStreamTrack-contentHint.html
Created attachment 350492 [details] Archive of layout-test-results from ews104 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 350484 [details] 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-contentHint-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.
Created attachment 350708 [details] Patch
Comment on attachment 350708 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350708&action=review > Source/WebCore/ChangeLog:7 > + Reviewed by Youenn Fablet. prepare-ChangeLog should have added OOPS here. > Source/WebCore/ChangeLog:12 > + New added Tests: s/New added Tests:/Covered by tests:/ > Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:123 > + MediaStreamTrackPrivate::HintValue val; s/val/value. > Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.h:136 > + HintValue m_contentHint; What is the default value for m_contentHint? Do we have testing for that case?
Comment on attachment 350708 [details] Patch Attachment 350708 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9337639 New failing tests: imported/w3c/web-platform-tests/mst-content-hint/MediaStreamTrack-contentHint.html
Created attachment 350720 [details] Archive of layout-test-results from ews104 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 350727 [details] Patch
Created attachment 350761 [details] Patch
Comment on attachment 350761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350761&action=review > Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.h:70 > + HintValue contentHint() { return m_contentHint; } Would you be able to change it to a const getter as a follow-up patch?
Comment on attachment 350761 [details] Patch Rejecting attachment 350761 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 350761, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!. Full output: https://webkit-queues.webkit.org/results/9346960
Comment on attachment 350761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350761&action=review > Source/WebCore/ChangeLog:7 > + Reviewed by Youenn Fablet (OOPS!). You should remove the "(OOPS!)"
Comment on attachment 350761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350761&action=review >> Source/WebCore/ChangeLog:7 >> + Reviewed by Youenn Fablet (OOPS!). > > You should remove the "(OOPS!)" Reviewed by Youenn, OOPS indeed :D
Created attachment 350785 [details] Patch
Comment on attachment 350785 [details] Patch Clearing flags on attachment: 350785 Committed r236478: <https://trac.webkit.org/changeset/236478>
All reviewed patches have been landed. Closing bug.