On Mon, 21 Oct 2024 04:16:54 GMT, Abhishek Kumar <abhis...@openjdk.org> wrote:
>> Naveen Narayanan has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8342098: Updated review comments
>
> test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 70:
>
>> 68: Graphics g = realImage.createGraphics();
>> 69: g.setColor(Color.yellow);
>> 70: g.fillRect(0, 0, 200, 100);
>
> You may define `rect_width` and `rect_height` variable with 200 and 100
> respectively.
> These numbers are used more than once and you are trying to avoid magic
> numbers for better practice.
These are `IMAGE_WIDTH` and `IMAGE_HEIGHT` correspondingly, right?
> test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 78:
>
>> 76:
>> 77: canvas = new ImageCanvas();
>> 78: canvas.setBackground(Color.YELLOW);
>
> Use either `Color.yellow` or `Color.YELLOW` for consistency.
The latter is better.
> `robot.waitForIdle()` should be followed by `robot.delay(delay)`.
Yes.
> you can get rid of delay variable, just used once.
I don't mind a constant to control the delay.
> test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 102:
>
>> 100: String errorMessage = "FAIL : Captured Image is different
>> from "
>> 101: + "the actual image";
>> 102: System.err.println("Test failed");
>
> I think you should save the images if image comparison fails, they can be
> used for diagnostic purpose.
> No need to define a `errorMessage` variable, can throw the error message
> directly.
>
> `System.err.println("Test failed");` look redundant to me.
Absolutely right! Save the images — it'll make your life easier when you need
to analyse the test failure.
> test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 125:
>
>> 123: capturedPixel = capturedImg.getRGB(i, j);
>> 124: realPixel = realImg.getRGB(i, j);
>> 125: if (capturedPixel != realPixel) {
>
> To optimize further, `capturedImg.getRGB(i, j)` and `realImg.getRGB(i, j)`
> can be directly evaluated inside if condition.
Yet, you'll be unable to print values unless you use `getRGB` again.
> test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 131:
>
>> 129: }
>> 130: }
>> 131: return result;
>
> Suggestion:
>
> return true;
In fact, image comparison can be borrowed from the `Util` class (in Swing part):
https://github.com/openjdk/jdk/blob/1f3574855e79221739d8800235583b7c47ebae97/test/jdk/javax/swing/regtesthelpers/Util.java#L59-L63
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808827599
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808830534
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808835292
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808841275
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808846337
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808857931
