On Fri, 6 Oct 2023 11:17:01 GMT, Renjith Kannath Pariyangad <d...@openjdk.org>
wrote:
>> Hi Reviewers,
>>
>> Observations :
>> 1. Without com initialize if we access Mixer for recording, library loaded
>> invalid GUID and clipped description in windows(ID not found in registry).
>> With com initialization library load proper GUID (same as registry).
>> 2. For Play back device always loading proper device GUID irrespective of
>> com Initialization.
>>
>> Test:
>> Since screen lock and unlock workflow required for reproducing this issue,
>> did coupe of iteration of manual testing post fix and confirmed its
>> resolving the problem.
>> To reconfirm nothing is broken, executed all audio related test cases on
>> test bench post fix and all are green.
>>
>> Please review the changes and let me know your comments if any.
>>
>> Regards,
>> Renjith.
>
> Renjith Kannath Pariyangad has updated the pull request incrementally with
> three additional commits since the last revision:
>
> - Whitespace error fixed
> - Whitespace error fixed
> - CoInitializeEx error check modified with better condition
Changes requested by aivanov (Reviewer).
src/java.desktop/windows/native/libjsound/PLATFORM_API_WinOS_DirectSound.cpp
line 188:
> 186:
> 187: HRESULT hr=::CoInitializeEx(NULL, COINIT_MULTITHREADED |
> COINIT_DISABLE_OLE1DDE);
> 188: if(hr != S_OK || hr != S_FALSE || hr != RPC_E_CHANGED_MODE) {
Suggestion:
HRESULT hr = ::CoInitializeEx(NULL, COINIT_MULTITHREADED |
COINIT_DISABLE_OLE1DDE);
if (FAILED(hr) && hr != RPC_E_CHANGED_MODE) {
You said Microsoft recommends using `FAILED` and `SUCCEEDED` macros, so we
should use them here too.
I think you got the condition wrong. The valid values to continue are
if (hr == S_OK || hr == S_FALSE || hr == RPC_E_CHANGED_MODE) {
// Good - do the stuff
}
Here we need to reverse the condition:
if (!(hr == S_OK || hr == S_FALSE || hr == RPC_E_CHANGED_MODE)) {
// Unlock and return
}
To negate the condition, you negate all the operators and `OR` becomes `AND`.
Thus, the condition becomes:
if (hr != S_OK && hr != S_FALSE && hr != RPC_E_CHANGED_MODE)) {
// Unlock and return
}
The first two conditions could be converted to `FAILED(hr)`, which gives us the
condition I suggested.
src/java.desktop/windows/native/libjsound/PLATFORM_API_WinOS_DirectSound.cpp
line 234:
> 232: }
> 233:
> 234: if(hr != RPC_E_CHANGED_MODE) {
Suggestion:
if (hr != RPC_E_CHANGED_MODE) {
Space between the `if` keyword and the opening parenthesis.
-------------
PR Review: https://git.openjdk.org/jdk/pull/14898#pullrequestreview-1662190325
PR Review Comment: https://git.openjdk.org/jdk/pull/14898#discussion_r1348907643
PR Review Comment: https://git.openjdk.org/jdk/pull/14898#discussion_r1348908246
