Re: gdi32: Make GetICMProfile behave more like native. Rewrite ansi version as a wrapper and move color management functions to their own file.

[Detlef Riekenberg]

On Mo, 2008-01-21 at 16:33 +0100, Hans Leidekker wrote:

- gdi32: Make GetICMProfile behave more like native. Rewrite ansi
version as a wrapper and move color management functions to their own
file.

The large subject is an indicator, that the Patch can be splitted.
IMHO, moving the stubs to a different file can be a seperate patch.
> +BOOL WINAPI GetICMProfileA(HDC hdc, LPDWORD size, LPSTR filename)
> - if (!hdc || !size || !filename) return FALSE;

This is not the same as documented in MSDN, and
> +BOOL WINAPI GetICMProfileW(HDC hdc, LPDWORD size, LPWSTR filename)
> -
> - if (!hdc || !size) return FALSE;

ANSI and UNICODE are different.
Are you really sure?
(Please insert a big warning in the comment then)

Returning FALSE without a SetLastError() looks not correct, compared to
SetLastError(ERROR_INSUFFICIENT_BUFFER) below.

Sorry, i'm unable to test your code now. I try to do it tomorrow.

gdi32: Make GetICMProfile behave more like native. Rewriteansi version as a wrapper and move color management functions to theirown file.

[Dmitry Timoshkov]

"Detlef Riekenberg"  wrote:
> The large subject is an indicator, that the Patch can be splitted.
> IMHO, moving the stubs to a different file can be a seperate patch.

If the APIs get rewritten at the same time IMO it's ok to move them.

gdi32: Make GetICMProfile behave more like native. Rewrite ansi version as a wrapper and move color management functions to their own file.

[Hans Leidekker]

On Tuesday 22 January 2008 01:04:31 am Detlef Riekenberg wrote:
> - gdi32: Make GetICMProfile behave more like native. Rewrite ansi
>
> version as a wrapper and move color management functions to their own
> file.
>
> The large subject is an indicator, that the Patch can be splitted.
> IMHO, moving the stubs to a different file can be a seperate patch.

My reasoning was that moving the code and rewriting the ansi version
are neutral operations from a functional perspective, and the tests show
that (actually, the previous implementation crashes on my test, but you
get the point).

I agree that the changelog entry is long, I could have left the second
sentence out since it's more of a patch reading hint.
> > +BOOL WINAPI GetICMProfileA(HDC hdc, LPDWORD size, LPSTR filename)
> > - if (!hdc || !size || !filename) return FALSE;
>
> This is not the same as documented in MSDN, and
> > +BOOL WINAPI GetICMProfileW(HDC hdc, LPDWORD size, LPWSTR filename)
> > -
> > - if (!hdc || !size) return FALSE;
>
> ANSI and UNICODE are different.
> Are you really sure?
> (Please insert a big warning in the comment then)

Yes, I'm sure, it's what the tests brought up and they serve very well
as documentation here IMO.
> Returning FALSE without a SetLastError() looks not correct, compared to
> SetLastError(ERROR_INSUFFICIENT_BUFFER) below.

We return FALSE without a SetLastError() in many places. I think the
consensus is that we do it only when applications depend on it. Setting 
ERROR_INSUFFICIENT_BUFFER is part of the idiom for querying the buffer
size, clearly something that applications can be expected to depend on.

-Hans