Re: [PATCH] uxtheme: fixed the todo blocks in the SetWindowTheme tests. [try 2]

[James Hawkins]

On Tue, Oct 28, 2008 at 2:29 AM, Reece Dunn <msclrhd(a)googlemail.com> wrote:
> Hi,
>
> This fixes the todo blocks in the SetWindowTheme tests.
>
> Changes:
>  v2 -- initialise the hr variable so that it is initialised in the
> case where hwnd is a valid window handle.

Please fix your changelog entry to describe the *fix*, not the end
result (getting rid of todo_wine's).  Also,

+    HRESULT hr = S_OK;
     TRACE("(%p,%s,%s)\n", hwnd, debugstr_w(pszSubAppName),
           debugstr_w(pszSubIdList));
-    hr = UXTHEME_SetWindowProperty(hwnd, atSubAppName, pszSubAppName);
+    if(!IsWindow(hwnd))
+        hr = E_HANDLE;
+    if(SUCCEEDED(hr))
+        hr = UXTHEME_SetWindowProperty(hwnd, atSubAppName, pszSubAppName);
     if(SUCCEEDED(hr))
         hr = UXTHEME_SetWindowProperty(hwnd, atSubIdList, pszSubIdList);
     if(SUCCEEDED(hr))

This code is screaming for a goto.

-- 
James Hawkins

[PATCH] uxtheme: fixed the todo blocks in the SetWindowTheme tests. [try 2]

[Rob Shearman]

2008/10/28 James Hawkins <truiken(a)gmail.com>:
> +    HRESULT hr = S_OK;
>     TRACE("(%p,%s,%s)\n", hwnd, debugstr_w(pszSubAppName),
>           debugstr_w(pszSubIdList));
> -    hr = UXTHEME_SetWindowProperty(hwnd, atSubAppName, pszSubAppName);
> +    if(!IsWindow(hwnd))
> +        hr = E_HANDLE;
> +    if(SUCCEEDED(hr))
> +        hr = UXTHEME_SetWindowProperty(hwnd, atSubAppName, pszSubAppName);
>     if(SUCCEEDED(hr))
>         hr = UXTHEME_SetWindowProperty(hwnd, atSubIdList, pszSubIdList);
>     if(SUCCEEDED(hr))
>
> This code is screaming for a goto.

I disagree.

-- 
Rob Shearman

[PATCH] uxtheme: fixed the todo blocks in the SetWindowTheme tests. [try 2]

[James Hawkins]

On Tue, Oct 28, 2008 at 10:17 AM, Rob Shearman <robertshearman(a)gmail.com> wrote:
> 2008/10/28 James Hawkins <truiken(a)gmail.com>:
> > +    HRESULT hr = S_OK;
> >     TRACE("(%p,%s,%s)\n", hwnd, debugstr_w(pszSubAppName),
> >           debugstr_w(pszSubIdList));
> > -    hr = UXTHEME_SetWindowProperty(hwnd, atSubAppName, pszSubAppName);
> > +    if(!IsWindow(hwnd))
> > +        hr = E_HANDLE;
> > +    if(SUCCEEDED(hr))
> > +        hr = UXTHEME_SetWindowProperty(hwnd, atSubAppName, pszSubAppName);
> >     if(SUCCEEDED(hr))
> >         hr = UXTHEME_SetWindowProperty(hwnd, atSubIdList, pszSubIdList);
> >     if(SUCCEEDED(hr))
> >
> > This code is screaming for a goto.
>
> I disagree.

Yes, it was late and my mind stopped after the next thought.  There's
nothing to clean up, so simply returning the failed result is optimal.

-- 
James Hawkins

[PATCH] uxtheme: fixed the todo blocks in the SetWindowTheme tests. [try 2]

[Reece Dunn]

2008/10/28 James Hawkins <truiken(a)gmail.com>:
> On Tue, Oct 28, 2008 at 10:17 AM, Rob Shearman <robertshearman(a)gmail.com> wrote:
> > 2008/10/28 James Hawkins <truiken(a)gmail.com>:
> > > This code is screaming for a goto.
> >
> > I disagree.
>
> Yes, it was late and my mind stopped after the next thought.  There's
> nothing to clean up, so simply returning the failed result is optimal.

Ok, so I will use this version of the code but will hunt down where
the hwnd is being used, as per Alexandre's comments. I'll send a
revised patch later on today or tomorrow.

Thanks,
- Reece