Re: [PATCH 1/2] riched20: Stub for ITextFont interface and implement ITextRange::GetFont. (try 3)

[Nikolay Sivov]

> +typedef struct ITextFontImpl {
> - ITextFont ITextFont_iface;
> - LONG ref;
> -
> - ITextRangeImpl *txtRge;
> - ITextSelectionImpl *txtSel;
>
> +} ITextFontImpl;
> -

I think 'range' and 'selection' would be more readable, but what's more 
important you don't use 'txtSel' at all.
> +static HRESULT WINAPI ITextFont_fnQueryInterface(ITextFont *me, REFIID riid, void **ppvObj)

If we want some unification, it's usually called 'iface', not 'me'.
> -
>    *ppvObj = me;

That's not how it should look.
> +static IRichEditOleImpl *get_reOle(ITextFontImpl *txtFont)
> +{
> - if (txtFont->txtRge)
> -
>  return txtFont->txtRge->reOle;
> - else
> -
>  return txtFont->txtSel->reOle;
>
> +}

Please use a better naming, like 'get_reole_from_font()' or something 
like that.
> - FIXME("not implemented: %p\n", This);
> - return E_NOTIMPL;

This should give a proper trace line with arguments, and it should 
appear on every call.
> - ITextFontImpl *This = impl_from_ITextFont(me);
> - if (pValue)
> -
>    return E_INVALIDARG;
> - if (!get_reOle(This))
> -
>    return CO_E_RELEASED;
> -
> - FIXME("Stub\n");
> - *pValue = tomFalse;
> - return S_OK;

That's another example of tracing done wrong.
> static HRESULT WINAPI ITextRange_fnGetFont(ITextRange *me, ITextFont **pFont)
>   {
>       ITextRangeImpl *This = impl_from_ITextRange(me);
> - ITextFontImpl *txtFont = NULL;
> - HRESULT hres;
> -
> if (!This->reOle)
>     return CO_E_RELEASED;
>
> - FIXME("not implemented %p\n", This);
> - return E_NOTIMPL;
> - TRACE("%p\n", This);

And another.