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

[Jay Bhaskar]

On Tue, 8 Aug 2023 11:41:11 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

>> Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   clean up according to review comments
>
> 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).

done

> 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`

The python would be used to generate base64 code of images and will generate ModernMediaControlResource.cpp at build time later

> 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

done

> 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 ... ?

it is follow up

> 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.

done

> 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.

done

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1201#discussion_r1287333145
PR Review Comment: https://git.openjdk.org/jfx/pull/1201#discussion_r1287334646
PR Review Comment: https://git.openjdk.org/jfx/pull/1201#discussion_r1287335208
PR Review Comment: https://git.openjdk.org/jfx/pull/1201#discussion_r1287337043
PR Review Comment: https://git.openjdk.org/jfx/pull/1201#discussion_r1287335012
PR Review Comment: https://git.openjdk.org/jfx/pull/1201#discussion_r1287335407