On Fri, 4 Apr 2025 20:42:03 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> tests/system/src/test/java/test/util/ScreenshotCapture.java line 60:
>>
>>> 58: * @throws IOException when an I/O error occurs
>>> 59: */
>>> 60: public static byte[] takeScreenshot() throws IOException {
>>
>> I have no idea how to interpret this byte array. What is the use case for
>> this method?
>
> To save to a file, or send to a stream, for example. This is a general
> purpose test utility. The javadoc says "in PNG format".
OK, I see that now. You might say that it's a PNG in the `@return` statement
(like you do for the other methods). Maybe also add ", returning it as a byte
array" at the end of the first sentence?
>> tests/system/src/test/java/test/util/ScreenshotCapture.java line 107:
>>
>>> 105: * @return the screenshot in Base-64 encoded PNG, or an error
>>> message
>>> 106: */
>>> 107: public static String takeScreenshotBase64(String prefix, String
>>> postfix) {
>>
>> Passing in the `data:image/png;base64,` as a prefix is odd. This method
>> takes a screen shot and turns it into a base64-encoded PNG. Don't make the
>> caller pass in something that has to match this internal behavior of this
>> method. I would recommend removing the prefix and postfix entirely, and
>> having this method return a simple base64-encoded PNG with a
>> `data:image/png;base64,` data URL. The caller can add any other String
>> before or after this.
>
> I wanted to avoid creating multiple multi-megabyte strings.
>
> The method to use is writeScreenshot(), which should be simple enough to use,
> it's the method mentioned at the class level javadoc.
>
> Related: I was debating whether we should have two specific methods
>
> writeScreenshotStdout();
> writeSceeenshotStderr();
>
> instead of passing `System:out` or `System::err`. What do you think?
My suggestion doesn't change anything about how many or what size strings are
allocated in the common case. All it does is moves which method tacks on the
needed "data:image/png;base64," piece from the caller to the method that
actually creates the PNG.
As for two methods vs one, I like the flexibility as you have it, although
really only the System.err version is likely to be used (with gradle as the
test runner, writing it to stdout isn't generally as useful).
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1746#discussion_r2029424186
PR Review Comment: https://git.openjdk.org/jfx/pull/1746#discussion_r2029429884
