On Thu, 15 Feb 2024 09:56:51 GMT, Alexey Ivanov <aiva...@openjdk.org> wrote:
>> Christoph Langer has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add comments
>
> src/java.desktop/windows/native/libawt/windows/Devices.cpp line 143:
>
>> 141: } else {
>> 142: J2dTraceLn1(J2D_TRACE_INFO, "Devices::CollectMonitors:
>> GetMonitorInfo failed for monitor with handle %p", hMon);
>> 143: }
>
> I also have a concern for branching… just for logging. In release builds, the
> body of the `else`-blocks becomes empty and compiler should optimize it out,
> so it may not affect release builds at all.
Modified the coding.
> src/java.desktop/windows/native/libawt/windows/awt_Win32GraphicsDevice.cpp
> line 191:
>
>> 189: return;
>> 190: }
>> 191: VERIFY(::GetDIBits(hBMDC, hBM, 0, 1, NULL, gpBitmapInfo,
>> DIB_RGB_COLORS));
>
> I think the return value of `::GetDIBits` should also be analysed explicitly.
>
> I would prefer backing out if any of the functions above including
> `::GetDIBits` returns an error.
>
> Yet I'm not sure how critical it is to run the code below. [As I
> mentioned](https://github.com/openjdk/jdk/pull/17197#issuecomment-1875562325)
> in PR #17197, there's a workaround for a case where `::GetDIBits` returns 0.
> @MBaesken implemented a fallback to run the code below if even if all the
> above calls fail. It may be safe to ignore … in headless environment.
Hm, I added some code for verification of ::GetDIBits result. If it is 0, I
skip the rest of the coding. I don't want to touch more here since I don't
understand what it is doing.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17614#discussion_r1492140796
PR Review Comment: https://git.openjdk.org/jdk/pull/17614#discussion_r1492140340
