Re: [PATCH] riched20: Don't create too many IRichEditOle interfaces for a RichEdit. (try 3)

[Huw Davies]

On Thu, Jun 26, 2014 at 02:52:16PM +0800, Jactry Zeng wrote:
> diff --git a/dlls/riched20/editor.c b/dlls/riched20/editor.c
> index 2af2582..55821bf 100644
> --- a/dlls/riched20/editor.c
> +++ b/dlls/riched20/editor.c
> @@ -4468,7 +4468,20 @@ LRESULT ME_HandleMessage(ME_TextEditor *editor, UINT msg, WPARAM wParam,
>    case EM_GETOLEINTERFACE:
>    {
>      LPVOID *ppvObj = (LPVOID*) lParam;
> -    return CreateIRichEditOle(editor, ppvObj);
> +    if (editor->reOle)
> +    {
> +        *ppvObj = editor->reOle;
> +        IRichEditOle_AddRef(*ppvObj);
> +        return 1;
> +    }
> +
> +    if (CreateIRichEditOle(editor, ppvObj))
> +    {
> +        editor->reOle = *ppvObj;
> +        IRichEditOle_AddRef(*ppvObj);
> +        return 1;
> +    }
> +    return 0;

I'm sure you can think about how to simplify this a bit.
The clue that it needs simplification is that you've
got two AddRef()s in there.

Hint: The code is trying to do two things:

1. If needed, assign an interface to reOle 
2. AddRef and return.
> --- a/dlls/riched20/richole.c
> +++ b/dlls/riched20/richole.c
> @@ -126,12 +126,13 @@ IRichEditOle_fnRelease(IRichEditOle *me)
>
> TRACE ("%p ref=%u\n", This, ref);
>
> -    if (!ref)
> +    if (ref == 1)
>      {
>          TRACE ("Destroying %p\n", This);
>          This->txtSel->reOle = NULL;
>          ITextSelection_Release(&This->txtSel->ITextSelection_iface);
>          IOleClientSite_Release(&This->clientSite->IOleClientSite_iface);
> +        This->editor->reOle = NULL;

No, this is wrong.  This function should remain unchanged.

The editor is in charge of the reOle reference, so it
needs to release it when the editor is destroyed -
ie in ME_DestroyEditor().

Huw.

[PATCH] riched20: Don't create too many IRichEditOle interfaces for a RichEdit. (try 3)

[Jactry Zeng]

Hi Huw,
2014-06-26 15:54 GMT+08:00 Huw Davies <huw(a)codeweavers.com>:
> I'm sure you can think about how to simplify this a bit.
> The clue that it needs simplification is that you've
> got two AddRef()s in there.
>
> Hint: The code is trying to do two things:
>
> 1. If needed, assign an interface to reOle
> 2. AddRef and return.
> > --- a/dlls/riched20/richole.c
> > +++ b/dlls/riched20/richole.c
> > @@ -126,12 +126,13 @@ IRichEditOle_fnRelease(IRichEditOle *me)
> >
> > TRACE ("%p ref=%u\n", This, ref);
> >
> > -    if (!ref)
> > +    if (ref == 1)
> >      {
> >          TRACE ("Destroying %p\n", This);
> >          This->txtSel->reOle = NULL;
> >          ITextSelection_Release(&This->txtSel->ITextSelection_iface);

IOleClientSite_Release(&This->clientSite->IOleClientSite_iface);
> > +        This->editor->reOle = NULL;
>
> No, this is wrong.  This function should remain unchanged.
>
> The editor is in charge of the reOle reference, so it
> needs to release it when the editor is destroyed -
> ie in ME_DestroyEditor().

I will try again.
Thanks for your review.  :)


-- 
Regards,
Jactry Zeng

[PATCH] riched20: Don't create too many IRichEditOle interfaces for a RichEdit. (try 3)

[Jactry Zeng]

Hi,

Another try was sent: http://source.winehq.org/patches/data/105376


2014-06-26 19:10 GMT+08:00 Jactry Zeng <jactry92(a)gmail.com>:
> Hi Huw,
> 2014-06-26 15:54 GMT+08:00 Huw Davies <huw(a)codeweavers.com>:
> > I'm sure you can think about how to simplify this a bit.
> > The clue that it needs simplification is that you've
> > got two AddRef()s in there.
> >
> > Hint: The code is trying to do two things:
> >
> > 1. If needed, assign an interface to reOle
> > 2. AddRef and return.
> > > --- a/dlls/riched20/richole.c
> > > +++ b/dlls/riched20/richole.c
> > > @@ -126,12 +126,13 @@ IRichEditOle_fnRelease(IRichEditOle *me)
> > >
> > > TRACE ("%p ref=%u\n", This, ref);
> > >
> > > -    if (!ref)
> > > +    if (ref == 1)
> > >      {
> > >          TRACE ("Destroying %p\n", This);
> > >          This->txtSel->reOle = NULL;
> > >          ITextSelection_Release(&This->txtSel->ITextSelection_iface);
>
> IOleClientSite_Release(&This->clientSite->IOleClientSite_iface);
> > > +        This->editor->reOle = NULL;
> >
> > No, this is wrong.  This function should remain unchanged.
> >
> > The editor is in charge of the reOle reference, so it
> > needs to release it when the editor is destroyed -
> > ie in ME_DestroyEditor().
>
> I will try again.
> Thanks for your review.  :)
>
> --
> Regards,
> Jactry Zeng

-- 
Regards,
Jactry Zeng