Hi Victor,

you are perfectly right here, the situation is complicated. In my commit msg I provided a link, which specifies the following behaviour:

hInstApp HINSTANCE
[out] If SEE_MASK_NOCLOSEPROCESS is set and the ShellExecuteEx call succeeds, it sets this member to a value greater than 32. If the function fails, it is set to an SE_ERR_XXX error value that indicates the cause of the failure.
That made me think: what happens to this field if the ShellExecuteEx call succeeds and the flag SEE_MASK_NOCLOSEPROCESS is not set? Since this check was thought to forward the actual result of the execution i switched to a simpler and (in my eyes) more reliable code. Plus the original code did not use the flag noted in the specification.
The link you provided on the other hand makes it look like ShellExecuteEx always sets this field indendent of any flags passed to the function.

To find out the truth one would have to write test cases. It might well be that the implementation we use is partially incorrect. Especially when looking at testman (it works again :-O ) @ shell32_winetest shlexec which still has some failures.

Greg


2010/4/19 victor martinez <vicmarcal@hotmail.com>
Hi, i have been giving a look to this patch and i have some doubts(just learning :) )
The code is:
@@ -1298,8 +1298,8 @@ static HRESULT WINAPI ICPanel_IContextMenu2_InvokeCommand(
        sei.hwnd = lpcmi->hwnd;
        sei.nShow = SW_SHOWNORMAL;
        sei.lpVerb = L"open";
-       ShellExecuteExW(&sei);
-       if (sei.hInstApp <= (HINSTANCE)32)
+
+       if (ShellExecuteExW(&sei) == FALSE)
           return E_FAIL;


MSDN says that if ShellExecuteExW fails it sets hInstApp to a value lower than 32 and,also, returns FALSE.
(MSDN: http://msdn.microsoft.com/en-us/library/bb762154(VS.85).aspx )
So both lines of codes(buggy and patched) are,at first sight,doing the same.Just using a different way.
The Commit says:
- Simplify checks for success of ShellExecuteEx, field hInst may be an unreliable indicator according to
http://msdn.microsoft.com/en-us/library/bb759784%28v=VS.85%29.aspx

Why is it unreliable? :) I am not saying it is reliable, just that i dont find the reason there...my skills are limited.. :(
I have just found: "Although hInstApp is declared as an HINSTANCE for compatibility with 16-bit Windows applications, it is not a true HINSTANCE. It can be cast only to an int and compared to either 32 or the following SE_ERR_XXX error codes ".

So my question is: Is the Bug in the cast to (HINSTANCE) instead to (INT)?I mean,do this solve the bug too?:
-       if (sei.hInstApp <= (HINSTANCE)32)
+       if ((INT)sei.hInstApp <= 32)

As you see i am just learning, so thanks in advance.
And yes, i prefer the new way that patch is using.Using the returned the value seems much more logic than using a "collateral damage". :)
Btw, thanks Gregor for your hunting-fixing week ;)



Tus datos personales, más seguros con Internet Explorer 8. ¡Descárgatelo gratis!

_______________________________________________
Ros-dev mailing list
Ros-dev@reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev