Re: Unicodified fontdlg (next try)

[Robert Shearman]

Jacek Caban wrote:
> Changelog:
>    Unicodified fontdlg
>
> @@ -216,52 +213,60 @@
>  */
> BOOL WINAPI ChooseFontA(LPCHOOSEFONTA lpChFont)
> {
> - LPCVOID template;
> - HRSRC hResInfo;
> - HINSTANCE hDlginst;
> - HGLOBAL hDlgTmpl;
> -
> - if ( (lpChFont->Flags&CF_ENABLETEMPLATEHANDLE)!=0 )
> - {
> -
>    template=(LPCVOID)lpChFont->hInstance;
> - } else
> - {
> -
>    if ( (lpChFont->Flags&CF_ENABLETEMPLATE)!=0 )
> -
>    {
> -
>        hDlginst=lpChFont->hInstance;
> -
>        if( !(hResInfo = FindResourceA(hDlginst, lpChFont->lpTemplateName,
> -
>                        (LPSTR)RT_DIALOG)))
> -
>        {
> -
>            COMDLG32_SetCommDlgExtendedError(CDERR_FINDRESFAILURE);
> -
>            return FALSE;
> -
>        }
> -
>    } else
> -
>    {
> -
>        hDlginst=COMDLG32_hInstance;
> -
>        if (!(hResInfo = FindResourceA(hDlginst, "CHOOSE_FONT", (LPSTR)RT_DIALOG)))
> -
>        {
> -
>            COMDLG32_SetCommDlgExtendedError(CDERR_FINDRESFAILURE);
> -
>            return FALSE;
> -
>        }
> -
>    }
> -
>    if (!(hDlgTmpl = LoadResource(hDlginst, hResInfo )) ||
> -
>            !(template = LockResource( hDlgTmpl )))
> -
>    {
> -
>        COMDLG32_SetCommDlgExtendedError(CDERR_LOADRESFAILURE);
> -
>        return FALSE;
> -
>    }
> - CHOOSEFONTW chFontw;
> - LOGFONTW logFontw;
> - LPLOGFONTA lpLogFonta;
> - int len;
> - BOOL ret;
> - LPSTR lpszStyle;
> - LPCSTR lpTemplateName;
> -
> - memcpy(&chFontw, lpChFont, sizeof(CHOOSEFONTW));
> - memcpy(&logFontw, lpChFont->lpLogFont, sizeof(LOGFONTA));
> - chFontw.lpLogFont = &logFontw;
> -
> - MultiByteToWideChar(CP_ACP, 0, lpChFont->lpLogFont->lfFaceName,
> -
>                    LF_FACESIZE, chFontw.lpLogFont->lfFaceName, LF_FACESIZE);
> -
> - if(lpChFont->lpszStyle)  {
> -
>    len = MultiByteToWideChar(CP_ACP, 0, lpChFont->lpszStyle, -1, NULL, 0);
> -
>    chFontw.lpszStyle = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, len*sizeof(WCHAR));

Initialising the whole string to zero just before it is about to be 
overwritten seems needlessly wasteful on CPU cycles and memory 
bandwidth. You seem to do this in a few places. Yes, MultiByteToWideChar 
can, in some cases, leave the string without a null terminator, but that 
only happens when there is insufficient buffer. In this case, the only 
way that can happen is if another thread modifies the buffer between the 
previous MultiByteToWideChar and the next one here. If you wanted to be 
really careful, you could always add "chFontw.lpszStyle[len-1] = '\0';" 
after to call to MultiByteToWideChar.
> -
>    MultiByteToWideChar(CP_ACP, 0, lpChFont->lpszStyle, -1, chFontw.lpszStyle, len);
> -
>    HeapFree(GetProcessHeap(), 0, lpChFont->lpszStyle);

You seem to be freeing a buffer that the user passed in. How do you know 
it was allocated with HeapAlloc?
> - }
> -
> - if(lpChFont->lpTemplateName)  {
> -
>    len = MultiByteToWideChar(CP_ACP, 0, lpChFont->lpTemplateName, -1, NULL, 0);
> -
>    chFontw.lpTemplateName = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, len*sizeof(WCHAR));
> -
>    MultiByteToWideChar(CP_ACP, 0, lpChFont->lpTemplateName,
> -
>                        -1, (LPWSTR)chFontw.lpTemplateName, len);
> - }
> -
> - ret = ChooseFontW(&chFontw);
> -
> - lpLogFonta = lpChFont->lpLogFont;
> - lpszStyle = lpChFont->lpszStyle;
> - lpTemplateName = lpChFont->lpTemplateName;
> - memcpy(lpChFont, &chFontw, sizeof(CHOOSEFONTA));
> - lpChFont->lpLogFont = lpLogFonta;
> - lpChFont->lpszStyle = lpszStyle;
> - lpChFont->lpTemplateName = lpTemplateName;
> - memcpy(lpChFont->lpLogFont, &logFontw, sizeof(LOGFONTA));
> - WideCharToMultiByte(CP_ACP, 0, logFontw.lfFaceName,
> -
>                    LF_FACESIZE, lpChFont->lpLogFont->lfFaceName, LF_FACESIZE, 0, 0);
> -
> - if(chFontw.lpszStyle)  {
> -
>    len = WideCharToMultiByte(CP_ACP, 0, chFontw.lpszStyle, -1, NULL, -1, 0, 0);;
> -
>    WideCharToMultiByte(CP_ACP, 0, chFontw.lpszStyle, -1, lpChFont->lpszStyle, len, 0, 0);
> -
>    HeapFree(GetProcessHeap(), 0, chFontw.lpszStyle);
> }
> -
>
> if (TRACE_ON(commdlg))
> -
>    _dump_cf_flags(lpChFont->Flags);
>
> -
>
> if (lpChFont->Flags & (CF_SELECTSCRIPT | CF_NOVERTFONTS ))
> -
>    FIXME(": unimplemented flag (ignored)\n");
> - if(chFontw.lpTemplateName)
> -
>    HeapFree(GetProcessHeap(), 0, (LPBYTE)chFontw.lpTemplateName);

You shouldn't need a cast here, unless lpTemplateName is a const.
> - return DialogBoxIndirectParamA(COMDLG32_hInstance, template,
> -
>        lpChFont->hwndOwner, FormatCharDlgProcA, (LPARAM)lpChFont );
> - return ret;
>
> }
> -
>
> #define TEXT_EXTRAS 4
> #define TEXT_COLORS 16

...
> @@ -1072,34 +1083,34 @@
> }
>
> /***********************************************************************
> -
> -
>       FormatCharDlgProcA   [internal]
> -
> -
>       FormatCharDlgProc   [internal]
>
> */
> -INT_PTR CALLBACK FormatCharDlgProcA(HWND hDlg, UINT uMsg, WPARAM wParam,
> +INT_PTR CALLBACK FormatCharDlgProc(HWND hDlg, UINT uMsg, WPARAM wParam,
>         LPARAM lParam)
> {
> - LPCHOOSEFONTA lpcf;
> -
>
> LPCHOOSEFONTW lpcf;
> INT_PTR res = FALSE;
>
> if (uMsg!=WM_INITDIALOG)
> {
> -
>    lpcf=(LPCHOOSEFONTA)GetPropA(hDlg, WINE_FONTDATA);
> -
>    if (!lpcf && uMsg != WM_MEASUREITEM)
> -
>    lpcf=(LPCHOOSEFONTW)GetPropW(hDlg, strWineFontData);
> -
>    if (!lpcf)
>       return FALSE;
>   if (CFn_HookCallChk32(lpcf))
> -
>        res=CallWindowProcA((WNDPROC)lpcf->lpfnHook, hDlg, uMsg, wParam, lParam);
> -
>        res=CallWindowProcW((WNDPROC)lpcf->lpfnHook, hDlg, uMsg, wParam, lParam);

This isn't something wrong with your patch, but is in the original code 
too. We shouldn't need to cast here, because if lpfnHook isn't using the 
right calling conventions or doesn't have the same number of parameters 
then we shouldn't be using CallWindowProc to start with.
>     if (res)
>         return res;
> }
> else
> {
> -
>    lpcf=(LPCHOOSEFONTA)lParam;
> -
>    lpcf=(LPCHOOSEFONTW)lParam;
>   if (!CFn_WMInitDialog(hDlg, wParam, lParam, lpcf))
>   {
>       TRACE("CFn_WMInitDialog returned FALSE\n");
>       return FALSE;
>   }
>   if (CFn_HookCallChk32(lpcf))
> -
>        return CallWindowProcA((WNDPROC)lpcf->lpfnHook,hDlg,WM_INITDIALOG,wParam,lParam);
> -
>        return CallWindowProcW((WNDPROC)lpcf->lpfnHook,hDlg,WM_INITDIALOG,wParam,lParam);
> }
>   switch (uMsg)
>   {

Rob