On Tue, 8 Aug 2023 11:41:11 GMT, Kevin Rushforth <k...@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
