Re: [PATCH 5/8] dsound: Make capture behave like native in regards to COM aggregation.

[Michael Stefaniuc]

[e-mail] wrote:
> Hi,
>
> Michael Stefaniuc wrote:
> > static HRESULT WINAPI IDirectSoundCaptureImpl_CreateCaptureBuffer
> - if (pUnk) {
> -
>    WARN("invalid parameter: pUnk != NULL\n");
> -
>    /* *lplpDSCaptureBuffer = NULL; Not done by native dsound */
> -
>    return DSERR_NOAGGREGATION;
> - }
>
> What's your rationale for doing so?
> Bug compatibility with native is fine, however not setting NULL is
> problematic before you've reached 100% compatibility, have you?

Not in this case which deals with COM aggregation. dsound doesn't
supports that and it is documented.
> What I mean is that if there's a situation where Wine's CreateCaptureBuffer
> returns some DSERR such as NOTIMPL where native does not, then that's an
> unexpected situation for the app which it may not be well prepared to handle,
> as it is untestable on native.  Now not zeroing output pointers may make it
> even harder for the app to remain in sane state.
> Then blame the app or Wine for a crash?
>
> If OTOH you know that the Wine module *always* returns the exact same
> values as native, then I've no objection in not zeroing output pointers when
> native does neither. Yet you'll have to consider that native may do so in
> the future, witness a growing number of MSDN documents covering later
> APIs that specifically mention zeroing pointers in case of failure.
>
> IIRC, AJ removed from my mmdevapi tests one snippet that showed that native's GetService
> does not zero an output pointer even though MSDN says so -- Wine zeroes. Yet perhaps
> I got confused, messed up with patches and submitted the one that did not contain that test
> so maybe AJ never had a chance to see it. I'll have to check that.

Patch 8/8 has the tests for this. I used the standard COM aggregation
test for classes that do not support COM aggregation. That was designed
by Jacek in
http://source.winehq.org/git/wine.git/commitdiff/5b16e6e0fdb35e57a034fd5e6df...

And the
    ok(!unk, "unk = %p\n", unk);
failed on the Win7 box I used for testing. That test is in because it is
known that Windows has classes that do not follow their own COM
standard, especially when it comes to COM aggregation.

Now i would have had different options how to deal with that.
- Silently drop the test.
- Dropping the test and documenting why it deviates from the standard test.
- Adding the test and marking it todo_wine.
- Add the test and fix the code as the change is trivial.

I picked IMHO the cleanest solution as that matches native 100% and adds
a test for that behavior.

bye
    michael