Re: riched20: ITextDocument stubs for ITextServices (retry)

[Caibin Chen]

Anybody reviewing this patch? It's almost all stubs, so although it's
long, it shouldn't be hard to review

2013/7/15 Caibin Chen <tigersoldi(a)gmail.com>:
> (Retry with real name in mail. The name in the patch is already my real name)
>
> This patch implements a standalone stub of ITextDocument. The stub is
> currently used in txtsrv.c and will replace the stub in richole.c
>
> ---
>  dlls/riched20/Makefile.in    |   1 +
>  dlls/riched20/tests/txtsrv.c |  16 ++-
>  dlls/riched20/txtdoc.c       | 282 +++++++++++++++++++++++++++++++++++++++++++
>  dlls/riched20/txtdoc.h       |  46 +++++++
>  dlls/riched20/txtsrv.c       |   6 +
>  5 files changed, 350 insertions(+), 1 deletion(-)
>  create mode 100644 dlls/riched20/txtdoc.c
>  create mode 100644 dlls/riched20/txtdoc.h

riched20: ITextDocument stubs for ITextServices (retry)

[Caibin Chen]

Hi,

It has been 2 weeks since I sent this patch[1]. It's still new status
in the patches list. I'm working on following patches based on it. I
appreciate your reviews and will work on the patch to get it
submitted.

Regards,
Caibin

[1] http://source.winehq.org/patches/data/97331

2013/7/18 Caibin Chen <tigersoldi(a)gmail.com>:
> Anybody reviewing this patch? It's almost all stubs, so although it's
> long, it shouldn't be hard to review
>
> 2013/7/15 Caibin Chen <tigersoldi(a)gmail.com>:
> > (Retry with real name in mail. The name in the patch is already my real name)
> >
> > This patch implements a standalone stub of ITextDocument. The stub is
> > currently used in txtsrv.c and will replace the stub in richole.c
> >
> > ---
> >  dlls/riched20/Makefile.in    |   1 +
> >  dlls/riched20/tests/txtsrv.c |  16 ++-
> >  dlls/riched20/txtdoc.c       | 282 +++++++++++++++++++++++++++++++++++++++++++
> >  dlls/riched20/txtdoc.h       |  46 +++++++
> >  dlls/riched20/txtsrv.c       |   6 +
> >  5 files changed, 350 insertions(+), 1 deletion(-)
> >  create mode 100644 dlls/riched20/txtdoc.c
> >  create mode 100644 dlls/riched20/txtdoc.h

riched20: ITextDocument stubs for ITextServices (retry)

[Jacek Caban]

Hi Caibin,

On 07/29/13 01:52, Caibin Chen wrote:
> Hi,
>
> It has been 2 weeks since I sent this patch[1]. It's still new status
> in the patches list. I'm working on following patches based on it. I
> appreciate your reviews and will work on the patch to get it
> submitted.

You sent the patch during code freeze, when it couldn't be committed, so
it probably didn't get enough attention. Now is the right time to resubmit.

+    /* COM aggregation on ITextDocument */
+    hr = IUnknown_QueryInterface(unk_obj.inner_unk, &IID_ITextDocument, (void**)&textdoc);
+    ok(hr == S_OK, "QueryInterface for IID_ITextDocument failed: %08x\n", hr);
+    refcount = ITextDocument_AddRef(textdoc);
+    ok(refcount == unk_obj.ref, "CreateTextServices just pretends to support COM aggregation\n");
+    refcount = ITextDocument_Release(textdoc);
+    ok(refcount == unk_obj.ref, "CreateTextServices just pretends to support COM aggregation\n");
+    refcount = ITextDocument_Release(textdoc);


It doesn't really make sense to test COM aggregation on every interface
implemented by an object. The valuable part of the test is that
ITextDocument should be supported, but there is probably a better place
to put this test.

+
+struct tagReTxtDoc {
+  IUnknown *outerObj;
+  ME_TextEditor *editor;
+  ITextDocument iTextDocumentIface;
+};

Do you really need a separated structure for that? Wouldn't adding ITextDocument implementation to ITextServicesImpl be enough?


Thanks,
Jacek

riched20: ITextDocument stubs for ITextServices (retry)

[Caibin Chen]

Hi Jacek,

Thanks for your review. I really appreciate it :)

2013/7/29 Jacek Caban <jacek(a)codeweavers.com>:
> You sent the patch during code freeze, when it couldn't be committed, so
> it probably didn't get enough attention. Now is the right time to resubmit.
> Does that means I should resend the patch to the mail list?
> It doesn't really make sense to test COM aggregation on every interface
> implemented by an object. The valuable part of the test is that
> ITextDocument should be supported, but there is probably a better place
> to put this test.
> Sure I can remove the aggregation tests.
> +struct tagReTxtDoc {
> +  IUnknown *outerObj;
> +  ME_TextEditor *editor;
> +  ITextDocument iTextDocumentIface;
> +};
>
> Do you really need a separated structure for that? Wouldn't adding ITextDocument implementation to ITextServicesImpl be enough?
> This structure will be used in both ITexstServicesImpl and
> IRichEditOleImpl(richole.c). For now IRichEditOleImpl has its own
> implementation(stub) of ITextDocument and ITextSelection. A stub of
> ITextSelection is needed before removing IRichEditOleImpl's own
> implementation and use the separate one.
> There are already several patches that are ready for review once this
> patch get submitted.

Thanks
Caibin

riched20: ITextDocument stubs for ITextServices (retry)

[Jacek Caban]

On 07/29/13 17:55, Caibin Chen wrote:
> > +struct tagReTxtDoc {
> > +  IUnknown *outerObj;
> > +  ME_TextEditor *editor;
> > +  ITextDocument iTextDocumentIface;
> > +};
> >
> > Do you really need a separated structure for that? Wouldn't adding ITextDocument implementation to ITextServicesImpl be enough?
> > This structure will be used in both ITexstServicesImpl and
> > IRichEditOleImpl(richole.c). For now IRichEditOleImpl has its own
> > implementation(stub) of ITextDocument and ITextSelection. A stub of
> > ITextSelection is needed before removing IRichEditOleImpl's own
> > implementation and use the separate one.
> > There are already several patches that are ready for review once this
> > patch get submitted.

I see, assuming that substantial part of the implementation may really
be shared between those objects, that makes sense. Bellow is review of
the remaining part of the patch:

+struct tagReTxtDoc {
+  IUnknown *outerObj;
+  ME_TextEditor *editor;
+  ITextDocument iTextDocumentIface;
+};


Please follow http://wiki.winehq.org/COMGuideline about naming
convention (*_iface names).

+ITextDocument *ReTxtDoc_get_ITextDocument(ReTxtDoc *document)
+{
+  TRACE("(%p)\n", document);
+  return &document->iTextDocumentIface;
+}


Full declaration of ReTxtDoc in a header (so you don't need such helper) would be cleaner, IMO.


new file mode 100644
index 0000000..02579b2
--- /dev/null
+++ b/dlls/riched20/txtdoc.h


How about using an existing header?



Thanks,
Jacek