RFR: 8313181: Enabling modern media controls on webkit 616.1 does not load button images on HTML5 video Element

[Kevin Rushforth]

On Sun, 6 Aug 2023 11:46:32 GMT, Jay Bhaskar <jbhaskar at 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