On Sun, 6 Aug 2023 11:46:32 GMT, Jay Bhaskar <jbhas...@openjdk.org> wrote:
> Issue : Enabling modern media controls on webkit 616.1 does not load button > images on HTML5 Video Element > Solution: Add resources and correct MediaControl Stylesheet I tested this on Windows and confirm that the media controls are visible and active now. I left a few inline comments on the test and the utility script. modules/javafx.web/src/main/native/Source/WebCore/platform/java/ModernMediaControlResource.cpp line 26: > 24: */ > 25: > 26: Minor: extra blank line (only one is needed). modules/javafx.web/src/main/native/Source/WebCore/platform/java/generate_icon_resource.py line 1: > 1: #!/usr/bin/env python3 Do we need this tool in our repo? If so, I recommend moving it somewhere under `src/main/native/Tools` modules/javafx.web/src/main/native/Source/WebCore/platform/java/generate_icon_resource.py line 30: > 28: ''' > 29: ''' > 30: Tool to generate base64 code There is a trailing space on this line. We don't currently enforce whitespace rules for `.py` files, but might in the future. tests/manual/web/HTML5VideoControlTest.java line 39: > 37: webView.getEngine().loadContent( > 38: "<video width=\"320\" height=\"240\" controls>\n" + > 39: " <source src=\"your_video_url.mp4\" type=\"video/mp4\">\n" + We need a real URL here. You might consider using this, which we use for our other samples that need video: https://download.oracle.com/otndocs/products/javafx/oow2010-2.mp4 tests/manual/web/HTML5VideoControlTest.java line 48: > 46: > 47: HBox content = new HBox(); > 48: content.getChildren().addAll(createInstructionsBox(), webView); > // Swap the order here Is the comment meant to suggest a follow-up action or ... ? tests/manual/web/HTML5VideoControlTest.java line 64: > 62: > 63: instructionsBox.getChildren().addAll( > 64: new javafx.scene.control.Label("Instructions:"), Minor: I recommend importing `javafx.scene.control.Label` so you don't need to specify the fully qualified name multiple times. tests/manual/web/HTML5VideoControlTest.java line 68: > 66: new javafx.scene.control.Label("2. Use the video controls to > pause,"), > 67: new javafx.scene.control.Label("2. controls should be > visible,"), > 68: new javafx.scene.control.Label("4. it works!") You have two step 2 lines, and step 4 isn't precise enough. I would recommend replacing both line 2s and line 4 with something like: 2. The media controls should be visible once the video starts playing. 3. Use the media controls to play/pause/seek the video. ------------- PR Review: https://git.openjdk.org/jfx/pull/1201#pullrequestreview-1567105007 PR Review Comment: https://git.openjdk.org/jfx/pull/1201#discussion_r1286996186 PR Review Comment: https://git.openjdk.org/jfx/pull/1201#discussion_r1287005352 PR Review Comment: https://git.openjdk.org/jfx/pull/1201#discussion_r1287175444 PR Review Comment: https://git.openjdk.org/jfx/pull/1201#discussion_r1287163516 PR Review Comment: https://git.openjdk.org/jfx/pull/1201#discussion_r1287006552 PR Review Comment: https://git.openjdk.org/jfx/pull/1201#discussion_r1287007778 PR Review Comment: https://git.openjdk.org/jfx/pull/1201#discussion_r1287172628
