[ManagedMSE] JS ManagedSourceBuffer object doesn't present any of its attributes #14092

jyavenard · 2023-05-19T18:52:14Z

`202d7e7`

[ManagedMSE] JS ManagedSourceBuffer object doesn't present any of its attributes
https://bugs.webkit.org/show_bug.cgi?id=257037
rdar://109569570

Reviewed by Chris Dumez.

Add missing custom toJS for MediaSource and SourceBuffer.

* LayoutTests/media/media-source/media-managedmse-bufferedchange-expected.txt:
* LayoutTests/media/media-source/media-managedmse-bufferedchange.html:
* Source/WebCore/Modules/mediasource/MediaSource.idl:
* Source/WebCore/Modules/mediasource/SourceBuffer.idl:
* Source/WebCore/Sources.txt:
* Source/WebCore/WebCore.xcodeproj/project.pbxproj:
* Source/WebCore/bindings/js/JSMediaSourceCustom.cpp: Added.
(WebCore::toJSNewlyCreated):
(WebCore::toJS):
* Source/WebCore/bindings/js/JSSourceBufferCustom.cpp: Added.
(WebCore::toJSNewlyCreated):
(WebCore::toJS):

Canonical link: https://commits.webkit.org/264284@main

f2252ea

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	~~🧪 gtk-wk2~~
	~~🧪 api-ios~~	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv	~~🧪 mac-AS-debug-wk2~~
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-05-19T18:52:33Z

EWS run on previous version of this PR (hash a7170c4)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
❌ 🧪 style	⏳ 🛠 ios	⏳ 🛠 mac	❌ 🛠 wpe	⏳ 🛠 wincairo
⏳ 🧪 bindings	⏳ 🛠 ios-sim	⏳ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2
~~🧪 webkitperl~~	⏳ 🧪 ios-wk2	⏳ 🧪 api-mac	❌ 🛠 gtk
⏳ 🧪 webkitpy	⏳ 🧪 ios-wk2-wpt	⏳ 🧪 mac-wk1	❌ 🧪 gtk-wk2
⏳ 🛠 🧪 jsc	⏳ 🧪 api-ios	⏳ 🧪 mac-wk2	❌ 🧪 api-gtk
	⏳ 🛠 tv	⏳ 🧪 mac-AS-debug-wk2
	⏳ 🛠 tv-sim	⏳ 🧪 mac-wk2-stress
	⏳ 🛠 watch
	⏳ 🛠 watch-sim

webkit-early-warning-system · 2023-05-19T18:58:17Z

EWS run on previous version of this PR (hash e9b9d5c)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
❌ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	❌ 🧪 mac-wk1	~~🧪 gtk-wk2~~
	⏳ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv	~~🧪 mac-AS-debug-wk2~~
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch
	✅ 🛠 watch-sim

cdumez · 2023-05-19T18:59:41Z

LayoutTests/media/media-source/media-managedmse-bufferedchange.html

@@ -37,6 +37,10 @@
            run('sourceBuffer.appendBuffer(loader.initSegment())');
            await waitFor(sourceBuffer, 'update');

+            sourceBuffer.onbufferedchange = () => {


Testing isn't great. I'd add something like:

shouldBe("sourceBuffer.__proto__", "ManagedSourceBuffer.prototype"); shouldBeTrue("sourceBuffer.hasOwnProperty('onbufferedchange')");

Or whatever the equivalent is in media tests.

A test that doesn't FAIL when the property is missing is not a good test.

it would have failed if the property was missing.
That's how I found the bug to start.

If the sourceBuffer object is actually a SourceBuffer rather than a ManagedSourceBuffer then the onbufferedchange property doesn't exist to start, and so setting it does nothing.

It wouldn't have failed, it would have given different output which could have easily been mistakenly rebaselined. It is bad practice. Please make it so that the test output contains a FAIL line if the property is missing.

cdumez · 2023-05-19T19:04:47Z

Source/WebCore/bindings/js/JSMediaSourceCustom.cpp

+
+#include "config.h"
+
+#if ENABLE(MANAGED_MEDIA_SOURCE)


This doesn't seem right.

oh right...

Source/WebCore/bindings/js/JSMediaSourceCustom.cpp

Source/WebCore/bindings/js/JSSourceBufferCustom.cpp

webkit-early-warning-system · 2023-05-19T22:17:54Z

EWS run on previous version of this PR (hash f0b9438)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
❌ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	⏳ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	⏳ 🧪 wpe-wk2
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	⏳ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	~~🧪 mac-wk1~~	⏳ 🧪 gtk-wk2
⏳ 🛠 🧪 jsc	⏳ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv	⏳ 🧪 mac-AS-debug-wk2
	⏳ 🛠 tv-sim	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch
	✅ 🛠 watch-sim

cdumez

LGTM % nits.

cdumez · 2023-05-19T22:21:58Z

Source/WebCore/bindings/js/JSSourceBufferCustom.cpp

+#if ENABLE(MANAGED_MEDIA_SOURCE)
+#include "JSManagedSourceBuffer.h"
+#endif
+#include "JSSourceBuffer.h"


First come the non guarded includes, then the guarded ones with a blank line in between, so it should look like:

#include "JSSourceBuffer.h" #if ENABLE(MANAGED_MEDIA_SOURCE) #include "JSManagedSourceBuffer.h" #endif

cdumez · 2023-05-19T22:22:39Z

LayoutTests/media/media-source/media-managedmse-bufferedchange.html

@@ -35,6 +35,9 @@
            waitFor(video, 'error').then(failTest);

            run('sourceBuffer = source.addSourceBuffer(loader.type())');
+            testExpected('sourceBuffer.__proto__', ManagedSourceBuffer.prototype);
+            testExpected('sourceBuffer.onbufferedchange !== undefined', true);


My comment was to add these checks in addition to your new event listener. Checking that the event listener gets called seemed useful too.

indeed ; but right now there's another bug that makes it behave differently if the GPU process is turned on. I will fix it soon

Oh, right. Fine.

webkit-early-warning-system · 2023-05-19T22:30:55Z

EWS run on current version of this PR (hash f2252ea)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	~~🧪 gtk-wk2~~
	~~🧪 api-ios~~	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv	~~🧪 mac-AS-debug-wk2~~
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

… attributes https://bugs.webkit.org/show_bug.cgi?id=257037 rdar://109569570 Reviewed by Chris Dumez. Add missing custom toJS for MediaSource and SourceBuffer. * LayoutTests/media/media-source/media-managedmse-bufferedchange-expected.txt: * LayoutTests/media/media-source/media-managedmse-bufferedchange.html: * Source/WebCore/Modules/mediasource/MediaSource.idl: * Source/WebCore/Modules/mediasource/SourceBuffer.idl: * Source/WebCore/Sources.txt: * Source/WebCore/WebCore.xcodeproj/project.pbxproj: * Source/WebCore/bindings/js/JSMediaSourceCustom.cpp: Added. (WebCore::toJSNewlyCreated): (WebCore::toJS): * Source/WebCore/bindings/js/JSSourceBufferCustom.cpp: Added. (WebCore::toJSNewlyCreated): (WebCore::toJS): Canonical link: https://commits.webkit.org/264284@main

webkit-commit-queue · 2023-05-20T03:36:04Z

Committed 264284@main (202d7e7): https://commits.webkit.org/264284@main

Reviewed commits have been landed. Closing PR #14092 and removing active labels.

jyavenard requested a review from cdumez as a code owner May 19, 2023 18:52

jyavenard self-assigned this May 19, 2023

jyavenard added the Media Bugs related to the HTML 5 Media elements. label May 19, 2023

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 19, 2023

jyavenard removed the merging-blocked Applied to prevent a change from being merged label May 19, 2023

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 19, 2023

jyavenard force-pushed the bz/257037 branch from a7170c4 to e9b9d5c Compare May 19, 2023 18:57

jyavenard removed the merging-blocked Applied to prevent a change from being merged label May 19, 2023

cdumez requested changes May 19, 2023

View reviewed changes

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 19, 2023

jyavenard removed the merging-blocked Applied to prevent a change from being merged label May 19, 2023

jyavenard force-pushed the bz/257037 branch from e9b9d5c to f0b9438 Compare May 19, 2023 22:17

cdumez approved these changes May 19, 2023

View reviewed changes

jyavenard force-pushed the bz/257037 branch from f0b9438 to f2252ea Compare May 19, 2023 22:28

jyavenard added the merge-queue Applied to send a pull request to merge-queue label May 20, 2023

webkit-commit-queue force-pushed the bz/257037 branch from f2252ea to 202d7e7 Compare May 20, 2023 03:36

webkit-commit-queue merged commit 202d7e7 into WebKit:main May 20, 2023

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label May 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ManagedMSE] JS ManagedSourceBuffer object doesn't present any of its attributes #14092

[ManagedMSE] JS ManagedSourceBuffer object doesn't present any of its attributes #14092

jyavenard commented May 19, 2023 •

edited by webkit-early-warning-system

webkit-early-warning-system commented May 19, 2023 •

edited

webkit-early-warning-system commented May 19, 2023 •

edited

cdumez May 19, 2023

jyavenard May 19, 2023

cdumez May 19, 2023

cdumez May 19, 2023

jyavenard May 19, 2023

webkit-early-warning-system commented May 19, 2023 •

edited

cdumez left a comment

cdumez May 19, 2023

cdumez May 19, 2023

jyavenard May 19, 2023

cdumez May 19, 2023

webkit-early-warning-system commented May 19, 2023 •

edited

webkit-commit-queue commented May 20, 2023

[ManagedMSE] JS ManagedSourceBuffer object doesn't present any of its attributes #14092

[ManagedMSE] JS ManagedSourceBuffer object doesn't present any of its attributes #14092

Conversation

jyavenard commented May 19, 2023 • edited by webkit-early-warning-system

webkit-early-warning-system commented May 19, 2023 • edited

webkit-early-warning-system commented May 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented May 19, 2023 • edited

cdumez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented May 19, 2023 • edited

webkit-commit-queue commented May 20, 2023

jyavenard commented May 19, 2023 •

edited by webkit-early-warning-system

webkit-early-warning-system commented May 19, 2023 •

edited

webkit-early-warning-system commented May 19, 2023 •

edited

webkit-early-warning-system commented May 19, 2023 •

edited

webkit-early-warning-system commented May 19, 2023 •

edited