[PATCH] msxml3: put_output accepts DOMDocument destination

[Nikolay Sivov]

Hi, Jefferson. Please see comments below.
> @@ -344,10 +355,10 @@ static HRESULT write_output_buffer(mxwriter *writer, const WCHAR *data, int len)
>
> if (!avail)
>               {
> -                IStream_Write(writer->dest, buff->data, buff->written, &written);
> +                IStream_Write(writer->dest_stream, buff->data, buff->written, &written);
>                   buff->written = 0;
>                   if (src_len >= buff->allocated)
> -                    IStream_Write(writer->dest, data, src_len, &written);
> +                    IStream_Write(writer->dest_stream, data, src_len, &written);
>                   else if (src_len)
>                   {
>                       memcpy(buff->data, data, src_len);
> @@ -371,7 +382,7 @@ static HRESULT write_output_buffer(mxwriter *writer, const WCHAR *data, int len)
>                   /* drain what we go so far */
>                   if (buff->written)
>                   {
> -                    IStream_Write(writer->dest, buff->data, buff->written, &written);
> +                    IStream_Write(writer->dest_stream, buff->data, buff->written, &written);
>                       buff->written = 0;
>                       avail = buff->allocated;
>                   }
> I don't think this is a way to deal with that case. When writer output 
> is set to DOM document what will probably happen
> is a complete tree created and accumulated during SAX calls, and then 
> cloned back to said document on endDocument(). That means we'll simply
> have to skip over any writing call, both for existing stream or BSTR 
> path, and create nodes instead.
> +    case VT_DISPATCH:
> +    {
> +        IXMLDOMDocument *doc;
> +        hr = IUnknown_QueryInterface(V_DISPATCH(&dest), &IID_IXMLDOMDocument, (void**)&doc);
> +        if (hr == S_OK)
> +        {
> +            This->dest_type = MXWriterDestDocument;
> +            This->dest_document = doc;
> +            break;
> +        }
> +        FIXME("unhandled interface type for VT_DISPATCH destination\n");
> +        return E_NOTIMPL;
> +    }
> It's impossible to tell if this is correct without tests, e.g. 
> VT_UNKNOWN could also be passed with document pointer, it's valid in 
> VARIANT sense.

So please, if you're interested in working on this, first submit some 
tests and ideally a bug report, if you can tell which application is 
affected.

[Jefferson Carpenter]

Will write tests, thx.

On 4/14/2019 7:29 AM, Nikolay Sivov wrote:
> Hi, Jefferson. Please see comments below.
> > @@ -344,10 +355,10 @@ static HRESULT write_output_buffer(mxwriter 
> > *writer, const WCHAR *data, int len)
> >               if (!avail)
> >               {
> > -                IStream_Write(writer->dest, buff->data, 
> > buff->written, &written);
> > +                IStream_Write(writer->dest_stream, buff->data, 
> > buff->written, &written);
> >                   buff->written = 0;
> >                   if (src_len >= buff->allocated)
> > -                    IStream_Write(writer->dest, data, src_len, 
> > &written);
> > +                    IStream_Write(writer->dest_stream, data, src_len, 
> > &written);
> >                   else if (src_len)
> >                   {
> >                       memcpy(buff->data, data, src_len);
> > @@ -371,7 +382,7 @@ static HRESULT write_output_buffer(mxwriter 
> > *writer, const WCHAR *data, int len)
> >                   /* drain what we go so far */
> >                   if (buff->written)
> >                   {
> > -                    IStream_Write(writer->dest, buff->data, 
> > buff->written, &written);
> > +                    IStream_Write(writer->dest_stream, buff->data, 
> > buff->written, &written);
> >                       buff->written = 0;
> >                       avail = buff->allocated;
> >                   }
> > I don't think this is a way to deal with that case. When writer output 
> > is set to DOM document what will probably happen
> > is a complete tree created and accumulated during SAX calls, and then 
> > cloned back to said document on endDocument(). That means we'll simply
> > have to skip over any writing call, both for existing stream or BSTR 
> > path, and create nodes instead.
> > +    case VT_DISPATCH:
> > +    {
> > +        IXMLDOMDocument *doc;
> > +        hr = IUnknown_QueryInterface(V_DISPATCH(&dest), 
> > &IID_IXMLDOMDocument, (void**)&doc);
> > +        if (hr == S_OK)
> > +        {
> > +            This->dest_type = MXWriterDestDocument;
> > +            This->dest_document = doc;
> > +            break;
> > +        }
> > +        FIXME("unhandled interface type for VT_DISPATCH destination\n");
> > +        return E_NOTIMPL;
> > +    }
> > It's impossible to tell if this is correct without tests, e.g. 
> > VT_UNKNOWN could also be passed with document pointer, it's valid in 
> > VARIANT sense.
>
> So please, if you're interested in working on this, first submit some 
> tests and ideally a bug report, if you can tell which application is 
> affected.

[Jefferson Carpenter]

Is there a way for me to close a patch from my end, or do you have to do that?

On Sun, Apr 14, 2019 at 7:29 AM Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
> Hi, Jefferson. Please see comments below.
> > @@ -344,10 +355,10 @@ static HRESULT write_output_buffer(mxwriter *writer, const WCHAR *data, int len)
> >
> > if (!avail)
> >               {
> > -                IStream_Write(writer->dest, buff->data, buff->written, &written);
> > +                IStream_Write(writer->dest_stream, buff->data, buff->written, &written);
> >                   buff->written = 0;
> >                   if (src_len >= buff->allocated)
> > -                    IStream_Write(writer->dest, data, src_len, &written);
> > +                    IStream_Write(writer->dest_stream, data, src_len, &written);
> >                   else if (src_len)
> >                   {
> >                       memcpy(buff->data, data, src_len);
> > @@ -371,7 +382,7 @@ static HRESULT write_output_buffer(mxwriter *writer, const WCHAR *data, int len)
> >                   /* drain what we go so far */
> >                   if (buff->written)
> >                   {
> > -                    IStream_Write(writer->dest, buff->data, buff->written, &written);
> > +                    IStream_Write(writer->dest_stream, buff->data, buff->written, &written);
> >                       buff->written = 0;
> >                       avail = buff->allocated;
> >                   }
> > I don't think this is a way to deal with that case. When writer output
> > is set to DOM document what will probably happen
> > is a complete tree created and accumulated during SAX calls, and then
> > cloned back to said document on endDocument(). That means we'll simply
> > have to skip over any writing call, both for existing stream or BSTR
> > path, and create nodes instead.
> > +    case VT_DISPATCH:
> > +    {
> > +        IXMLDOMDocument *doc;
> > +        hr = IUnknown_QueryInterface(V_DISPATCH(&dest), &IID_IXMLDOMDocument, (void**)&doc);
> > +        if (hr == S_OK)
> > +        {
> > +            This->dest_type = MXWriterDestDocument;
> > +            This->dest_document = doc;
> > +            break;
> > +        }
> > +        FIXME("unhandled interface type for VT_DISPATCH destination\n");
> > +        return E_NOTIMPL;
> > +    }
> > It's impossible to tell if this is correct without tests, e.g.
> > VT_UNKNOWN could also be passed with document pointer, it's valid in
> > VARIANT sense.
>
> So please, if you're interested in working on this, first submit some
> tests and ideally a bug report, if you can tell which application is
> affected.

[Jefferson Carpenter]

On 4/14/2019 7:29 AM, Nikolay Sivov wrote:
> Hi, Jefferson. Please see comments below.
> > @@ -344,10 +355,10 @@ static HRESULT write_output_buffer(mxwriter 
> > *writer, const WCHAR *data, int len)
> >               if (!avail)
> >               {
> > -                IStream_Write(writer->dest, buff->data, 
> > buff->written, &written);
> > +                IStream_Write(writer->dest_stream, buff->data, 
> > buff->written, &written);
> >                   buff->written = 0;
> >                   if (src_len >= buff->allocated)
> > -                    IStream_Write(writer->dest, data, src_len, 
> > &written);
> > +                    IStream_Write(writer->dest_stream, data, src_len, 
> > &written);
> >                   else if (src_len)
> >                   {
> >                       memcpy(buff->data, data, src_len);
> > @@ -371,7 +382,7 @@ static HRESULT write_output_buffer(mxwriter 
> > *writer, const WCHAR *data, int len)
> >                   /* drain what we go so far */
> >                   if (buff->written)
> >                   {
> > -                    IStream_Write(writer->dest, buff->data, 
> > buff->written, &written);
> > +                    IStream_Write(writer->dest_stream, buff->data, 
> > buff->written, &written);
> >                       buff->written = 0;
> >                       avail = buff->allocated;
> >                   }
> > I don't think this is a way to deal with that case. When writer output 
> > is set to DOM document what will probably happen
> > is a complete tree created and accumulated during SAX calls, and then 
> > cloned back to said document on endDocument(). That means we'll simply
> > have to skip over any writing call, both for existing stream or BSTR 
> > path, and create nodes instead.

Indeed, I was thinking that instead of calling write_output_buffer at 
all when the destination is a DOMDocument, the DOMDocument's createNode 
etc. functions would be called instead.  Perhaps the mxwriter would need 
to hold a reference to an IXMLDOMNode for in-progress nodes whose 
attributes and child nodes are presently being assigned.

Note that presently even though write_output_buffer is called when the 
destination is a DOMDocument, it now contains the code

+    else {
+        FIXME("unsupported destination type for writing %d\n", 
writer->dest_type);
+    }

which would be called if the destination were set to a DOMDocument.

It's not clear without tests whether the entire result is cloned back on 
endDocument, or whether the DOMDocument is built progressively in 
response to events sent to the SAXContentHandler.
> > +    case VT_DISPATCH:
> > +    {
> > +        IXMLDOMDocument *doc;
> > +        hr = IUnknown_QueryInterface(V_DISPATCH(&dest), 
> > &IID_IXMLDOMDocument, (void**)&doc);
> > +        if (hr == S_OK)
> > +        {
> > +            This->dest_type = MXWriterDestDocument;
> > +            This->dest_document = doc;
> > +            break;
> > +        }
> > +        FIXME("unhandled interface type for VT_DISPATCH destination\n");
> > +        return E_NOTIMPL;
> > +    }
> > It's impossible to tell if this is correct without tests, e.g. 
> > VT_UNKNOWN could also be passed with document pointer, it's valid in 
> > VARIANT sense.

Will write tests ASAP.

[Jefferson Carpenter]

On Monday, April 22, 2019, Jefferson Carpenter <
jeffersoncarpenter2(a)gmail.com> wrote:
> Indeed, I was thinking that instead of calling write_output_buffer at all

when the destination is a DOMDocument, the DOMDocument's createNode etc.
functions would be called instead.  Perhaps the mxwriter would need to hold
a reference to an IXMLDOMNode for in-progress nodes whose attributes and
child nodes are presently being assigned.
> Note that presently even though write_output_buffer is called when the

destination is a DOMDocument, it now contains the code
> +    else {
> +        FIXME("unsupported destination type for writing %d\n",

writer->dest_type);
> +    }
>
> which would be called if the destination were set to a DOMDocument.
>
> It's not clear without tests whether the entire result is cloned back on
> endDocument, or whether the DOMDocument is built progressively in response
> to events sent to the SAXContentHandler.

But now I've finished updating my computer, so hopefully no one can hack me
and move copyrighted Windows binaries onto my system.