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 :
> (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 :
> 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 :
> > (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 :
> 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