May 2011 - Wine-Devel - list.winehq.org

Re: advapi: test and fix GetUserName() in case where buffer is too small

by Dan Kegel

I'm looking forward to Andrew's test-only patch... When I saw he had a test on testbot, I probably should have dropped mine.

May 31, 2011

Re: advapi: test and fix GetUserName() in case where buffer is too small

by Andrew Nguyen

On 05/31/2011 08:17 AM, Hans Leidekker wrote: > On Tue, 2011-05-31 at 06:32 -0500, Andrew Nguyen wrote: > >> - DWORD sizeW = *lpSize * 2; >> + DWORD sizeW = 0; >> + >> + GetUserNameW( NULL, &sizeW ); > > Can we avoid the extra call? I was trying to avoid making the assumption that a Unicode buffer with *lpSize Unicode characters would necessarily be appropriate for the second GetUserNameW call. Considering the failure path for the GetUserNameW call in the old version, I don't believe we know what the number of ANSI characters actually is unless we had WideCharToMultiByte figure it out. Thus, my intent was to never allow a failure due to insufficient buffer size so GetUserNameA knows the appropriate value for sizeA. That said, I suspect Windows does allocate the Unicode buffer based on the passed buffer size, so I think just going back to the old approach of unconditionally allocating *lpSize Unicode characters should be okay in practice. >> I didn't intend to scoop Dan's work, but I have a version of his fix, >> which is attached, with deeper tests which I wrote before Hans expressed >> his concerns. Should I just send in the tests with todo_wine and let Dan >> fix those, or should I send a revised version and allow a "winner" to be >> picked? > > I vote for getting some tests in first. > > I'll go ahead and extract the tests to a separate patch then.

May 31, 2011

Re: advapi: test and fix GetUserName() in case where buffer is too small

by Hans Leidekker

On Tue, 2011-05-31 at 06:32 -0500, Andrew Nguyen wrote: > - DWORD sizeW = *lpSize * 2; > + DWORD sizeW = 0; > + > + GetUserNameW( NULL, &sizeW ); Can we avoid the extra call? > I didn't intend to scoop Dan's work, but I have a version of his fix, > which is attached, with deeper tests which I wrote before Hans expressed > his concerns. Should I just send in the tests with todo_wine and let Dan > fix those, or should I send a revised version and allow a "winner" to be > picked? I vote for getting some tests in first.

May 31, 2011

Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9

by Michael Mc Donnell

On Tue, May 31, 2011 at 12:51 PM, Michael Mc Donnell <michael(a)mcdonnell.dk> wrote: > On Mon, May 30, 2011 at 10:09 PM, Matteo Bruni <matteo.mystral(a)gmail.com> wrote: >> 2011/5/30 Michael Mc Donnell <michael(a)mcdonnell.dk>: >>> Thanks for the comments Matteo! I have a few questions below. >>> >>> On Mon, May 30, 2011 at 2:45 PM, Matteo Bruni <matteo.mystral(a)gmail.com> wrote: >>>> 2011/5/29 Michael Mc Donnell <michael(a)mcdonnell.dk>: >>>>> On Sat, May 28, 2011 at 10:08 PM, Stefan Dösinger >>>>> <stefandoesinger(a)gmx.at> wrote: >>>>>> On Saturday 28 May 2011 19:55:55 you wrote: >>>>>> >>>>>>> It was implemented by setting the vertex declaration to null when the >>>>>>> declaration is invalid. That seems to match what happens on Windows, >>>>>>> because if a program has set an invalid declaration and then calls >>>>>>> GetDeclaration or DrawSubset it will fail with an access violation. >>>>>>> Should Wine also fail with an access violation or should it be >>>>>>> slightly more graceful and return E_FAIL instead (which my new patch >>>>>>> does)? >>>>>> Yes, I think returning E_FAIL is a good idea, but write a WARN if >>>>>> CreateVertexDeclaration fails or when you catch a NULL vdecl. A WARN will not >>>>>> be shown by default(unlike a FIXME), it is used when the app does something >>>>>> fishy but you think you handled it correctly. >>>>>> >>>>>>> + if (FAILED(hr)) >>>>>>> + new_vertex_declaration = NULL; >>>>>>> + >>>>>>> + /* Free old vertex declaration */ >>>>>>> + if (This->vertex_declaration) >>>>>>> + IDirect3DVertexDeclaration9_Release(This->vertex_declaration); >>>>>> In this situation you can just release the old decl before calling >>>>>> CreateVertexDeclaration and pass &This->vertex_declaration to CreateVdecl. >>>>>> That simplified the code a bit. Don't forget to set This->vertex_declaration to >>>>>> NULL in case of an error though >>>>> >>>>> Ok I've simplified the code. I've also inserted three WARNs. I've >>>>> skipped a WARN when freeing the mesh because that works fine on >>>>> Windows with an invalid declaration. >>>>> >>>>>>> + /* Set the position type to color instead of float3 (invalid >>>>>> declaration) */ >>>>>>> + hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration_wrong_type_color); >>>>>>> + todo_wine ok(hr == D3D_OK, "Test UpdateSematics wrong type color, " >>>>>>> + "got %#x expected D3D_OK\n", hr); >>>>>> Is this really invalid? I don't know it on top of my head, but in general >>>>>> types and usage are independent, especially with shaders. I don't see a check >>>>>> in d3d9 or wined3d for this case. >>>>> >>>>> You're right it isn't invalid. It doesn't crash, it just doesn't draw >>>>> anything. I've change that comment and re-ordered the tests. >>>>> >>>>>> Beyond that I think the patches look OK. I'd recommend to send them to wine- >>>>>> devel one more time and then next week to wine-patches. Henri is on Vacation >>>>>> this week, and he's usually more picky than I am. I also hope some of the >>>>>> people who regularly contribute to d3dx9 will have a look and comment. >>>>> >>>>> Ok, I've attached the newest version. I'll let the patches sit here >>>>> for a week, and start working on the other functions in the mean time. >>>>> >>>> >>>> The patches are OK with me, just some nitpicking: >>> >>> That sounds great :-) >>> >>>> + testContext = HeapAlloc(GetProcessHeap(), 0, sizeof(struct TestContext)); >>>> >>>> We usually use something like HeapAlloc(GetProcessHeap(), 0, >>>> sizeof(*testContext)); instead. >>> >>> Ok, I guess that's better in case the type of the variable changes at >>> some point? >>> >> >> Yes, that's a reason. The other one is simply consistency. > > Ok, I've changed that. > >>>> I think you can remove some obvious comments (like /* Create the test >>>> mesh */). Also, check your whitespaces. >>> >>> Is it too much or too little whitespace? Could you give me an example? >> >> + todo_wine ok(hr == D3D_OK, "Test UpdateSematics for bigger vertex size, " >> >> + "got %#x expected D3D_OK\n", hr); >> >> Here you put 4 spaces on the line continuation, which is neither the >> "align to be below the '(' + 1 space" used in most of the file, nor >> the 8 spaces used in wined3d. The d3dx9_36 sources aren't known for a >> nice and consistent coding style (I'm sure you noticed that), so >> that's mostly trying to not mix even more style variants... > > Ok I've changed it to the "align to be below the '(' + 1 space" convention. > >> There was also something wrong with spaces in a comment that caught my >> eye, I think. > > The only thing that stood out to me was that I had extra newlines in > my multi-line comments. I've removed those. > >> I told you I was nitpicking... :) > > That's ok, I prefer consistency too :-) Oops the patches I sent earlier had a tab character and some superfluous whitespace.

May 31, 2011

RFC: d3d9: Increment the reference count of the IDirect3D9 instance when creating a device.

by Andrew Nguyen

The Pathologic demo in bug 21552 expects that the reference count of a IDirect3D9 instance is incremented after a successful call to CreateDevice with the instance. Does my proposed fix for d3d8 and d3d9 manage the reference count of the device parent appropriately? In particular, I'm concerned about the slightly cumbersome acquisition of the pointer to the parent instance within the Release call for the Direct3D device interface. Is the overall location of the AddRef/Release calls correct, and should I be doing something differently when the device tries to decrement the parent reference count?

May 31, 2011

Re: advapi: test and fix GetUserName() in case where buffer is too small

by Andrew Nguyen

On 05/31/2011 02:17 AM, Hans Leidekker wrote: > On Mon, 2011-05-30 at 21:13 -0700, Dan Kegel wrote: > >> @@ -91,7 +91,7 @@ GetUserNameW( LPWSTR lpszName, LPDWORD lpSize ) >> >> if (len > *lpSize) >> { >> - SetLastError(ERROR_MORE_DATA); >> + SetLastError(ERROR_INSUFFICIENT_BUFFER); > > GetUserNameA also needs to be fixed. It sets ERROR_MORE_DATA when > WideCharToMultiByte fails, which can simply be removed because that > function will set ERROR_INSUFFICIENT_BUFFER itself if the buffer is too > small. > > And there are at least two cases, in dlls/crypt32/protectdata.c and > dlls/msi/package.c, where we depend on the wrong error. > > > > I didn't intend to scoop Dan's work, but I have a version of his fix, which is attached, with deeper tests which I wrote before Hans expressed his concerns. Should I just send in the tests with todo_wine and let Dan fix those, or should I send a revised version and allow a "winner" to be picked?

May 31, 2011

Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9

by Michael Mc Donnell

On Mon, May 30, 2011 at 10:09 PM, Matteo Bruni <matteo.mystral(a)gmail.com> wrote: > 2011/5/30 Michael Mc Donnell <michael(a)mcdonnell.dk>: >> Thanks for the comments Matteo! I have a few questions below. >> >> On Mon, May 30, 2011 at 2:45 PM, Matteo Bruni <matteo.mystral(a)gmail.com> wrote: >>> 2011/5/29 Michael Mc Donnell <michael(a)mcdonnell.dk>: >>>> On Sat, May 28, 2011 at 10:08 PM, Stefan Dösinger >>>> <stefandoesinger(a)gmx.at> wrote: >>>>> On Saturday 28 May 2011 19:55:55 you wrote: >>>>> >>>>>> It was implemented by setting the vertex declaration to null when the >>>>>> declaration is invalid. That seems to match what happens on Windows, >>>>>> because if a program has set an invalid declaration and then calls >>>>>> GetDeclaration or DrawSubset it will fail with an access violation. >>>>>> Should Wine also fail with an access violation or should it be >>>>>> slightly more graceful and return E_FAIL instead (which my new patch >>>>>> does)? >>>>> Yes, I think returning E_FAIL is a good idea, but write a WARN if >>>>> CreateVertexDeclaration fails or when you catch a NULL vdecl. A WARN will not >>>>> be shown by default(unlike a FIXME), it is used when the app does something >>>>> fishy but you think you handled it correctly. >>>>> >>>>>> + if (FAILED(hr)) >>>>>> + new_vertex_declaration = NULL; >>>>>> + >>>>>> + /* Free old vertex declaration */ >>>>>> + if (This->vertex_declaration) >>>>>> + IDirect3DVertexDeclaration9_Release(This->vertex_declaration); >>>>> In this situation you can just release the old decl before calling >>>>> CreateVertexDeclaration and pass &This->vertex_declaration to CreateVdecl. >>>>> That simplified the code a bit. Don't forget to set This->vertex_declaration to >>>>> NULL in case of an error though >>>> >>>> Ok I've simplified the code. I've also inserted three WARNs. I've >>>> skipped a WARN when freeing the mesh because that works fine on >>>> Windows with an invalid declaration. >>>> >>>>>> + /* Set the position type to color instead of float3 (invalid >>>>> declaration) */ >>>>>> + hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration_wrong_type_color); >>>>>> + todo_wine ok(hr == D3D_OK, "Test UpdateSematics wrong type color, " >>>>>> + "got %#x expected D3D_OK\n", hr); >>>>> Is this really invalid? I don't know it on top of my head, but in general >>>>> types and usage are independent, especially with shaders. I don't see a check >>>>> in d3d9 or wined3d for this case. >>>> >>>> You're right it isn't invalid. It doesn't crash, it just doesn't draw >>>> anything. I've change that comment and re-ordered the tests. >>>> >>>>> Beyond that I think the patches look OK. I'd recommend to send them to wine- >>>>> devel one more time and then next week to wine-patches. Henri is on Vacation >>>>> this week, and he's usually more picky than I am. I also hope some of the >>>>> people who regularly contribute to d3dx9 will have a look and comment. >>>> >>>> Ok, I've attached the newest version. I'll let the patches sit here >>>> for a week, and start working on the other functions in the mean time. >>>> >>> >>> The patches are OK with me, just some nitpicking: >> >> That sounds great :-) >> >>> + testContext = HeapAlloc(GetProcessHeap(), 0, sizeof(struct TestContext)); >>> >>> We usually use something like HeapAlloc(GetProcessHeap(), 0, >>> sizeof(*testContext)); instead. >> >> Ok, I guess that's better in case the type of the variable changes at >> some point? >> > > Yes, that's a reason. The other one is simply consistency. Ok, I've changed that. >>> I think you can remove some obvious comments (like /* Create the test >>> mesh */). Also, check your whitespaces. >> >> Is it too much or too little whitespace? Could you give me an example? > > + todo_wine ok(hr == D3D_OK, "Test UpdateSematics for bigger vertex size, " > > + "got %#x expected D3D_OK\n", hr); > > Here you put 4 spaces on the line continuation, which is neither the > "align to be below the '(' + 1 space" used in most of the file, nor > the 8 spaces used in wined3d. The d3dx9_36 sources aren't known for a > nice and consistent coding style (I'm sure you noticed that), so > that's mostly trying to not mix even more style variants... Ok I've changed it to the "align to be below the '(' + 1 space" convention. > There was also something wrong with spaces in a comment that caught my > eye, I think. The only thing that stood out to me was that I had extra newlines in my multi-line comments. I've removed those. > I told you I was nitpicking... :) That's ok, I prefer consistency too :-)

May 31, 2011

Re: [1/8] d3d8: Use D3DPOOL <-> WINED3DPOOL enum conversion functions instead of implicit conversions (clang)

by Alexandre Julliard

Stefan Dösinger <stefandoesinger(a)gmx.at> writes: > Am 31.05.2011 um 12:12 schrieb Alexandre Julliard: >> Most enums are identical, or just have an extra value or two. I don't >> see why you can't pass either type to wined3d since they have the same >> name. > Do you mean the status quo? Or do you mean using the D3DPOOL type in > wined3d by including the d3d9 headers(this works in wined3d.dll > itself)? I mean using D3DPOOL in wined3d, not by including the d3d9 headers, but by defining it when necessary, i.e. if neither d3d8types.h nor d3d9types.h have been included already. This way d3d8 uses the d3d8 definition, d3d9 uses the d3d9 definition, and ddraw/d3d10/wined3d internals use the wined3d definition which is probably a superset of the others. -- Alexandre Julliard julliard(a)winehq.org

May 31, 2011

Re: [1/8] d3d8: Use D3DPOOL <-> WINED3DPOOL enum conversion functions instead of implicit conversions (clang)

by Stefan Dösinger

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Am 31.05.2011 um 12:12 schrieb Alexandre Julliard: > Most enums are identical, or just have an extra value or two. I don't > see why you can't pass either type to wined3d since they have the same > name. Do you mean the status quo? Or do you mean using the D3DPOOL type in wined3d by including the d3d9 headers(this works in wined3d.dll itself)? The problem here's ddraw(and d3d10/11), the functionally equivalent types work differently there, they are a set of flags in DDSURFACEDESC2. Yet we can't include d3d8types.h or d3d9types.h in ddraw because there are a handful of types that cause name conflicts. We used to do that a long time ago, but there are only a handful of types that exist in all 3 d3d versions and can be reused. In d3d10 the fixed function pipeline is gone, and so are the types related to it, but they are still needed for the wined3d interface definition. -----BEGIN PGP SIGNATURE----- Version: GnuPG/MacGPG2 v2.0.17 (Darwin) iQIcBAEBAgAGBQJN5MOyAAoJEN0/YqbEcdMwmFkP/AtNv+3BKIuaY08uDmrpcSI0 qvudinT7Yw6bA0B7TKRIbmgfGVFHc+u+GGfl6bk035LXDL0DBmeo2eYRsS3V3pSq 8lDd/T2g1MDCCFjUx9qoQJeHNGb9qN5gU+ztQeFLynsxvdvCmHNT+ztPejLUSLfe mDIKFZns1/Y6YtyqEbpmCTLjy/WadTeLCj0PfhNERCIujyyDxxdudcqiM2uTCmwe 01wSjISTXcwl3vUdevYOZ2phHHOkPrAQ5mhz0Y1vyTqqg/1sySuKfK4EyZvAiguC 9CSH4nDoiU8OriXFvfCQxHwKJ3dxVfP+EOh3BOXbaB/Nc90+5zKzQYPJzzLEp7uT bX0WkKJHr8R/thlQ/q3zYU2QvTjSVfk+eqdjnKZw5jziWdeM66SdBLeahsubyqOv poIepool5HqPt2oqFvzTyrvJzX730isSu1cy2vU8DmxLGh9H8zrExbPm3Mj16jcO lwapsIxHOagwhVvO1w8TzXGS5KuuRC6TQMaujEVJihdidf0F478YuSw2dpQynOra jaxHwjU+Hy9Tf0HWLr9VefV/mTvUDLe3e07c1QxcU6ImEwQx8QHgViN2nbgCshco WfJefQTTFHg73cN4F9gkXjfIM1Yl3SNcz387spL2GXb3OB3fvAZeinzs9rlUi3b/ sUBcqL50lIuwUgUKOzrg =6HyE -----END PGP SIGNATURE-----

May 31, 2011

Re: [PATCH 1/6] mshtml: Added support for accessing document elements by index

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=11331 Your paranoid android. === W2KPROSP4 (32 bit) === No test summary line found === WXPPROSP3 (32 bit) === No test summary line found === W2K3R2SESP2 (32 bit) === No test summary line found === WVISTAADM (32 bit) === No test summary line found === W2K8SE (32 bit) === No test summary line found === W7PRO (32 bit) === No test summary line found === W7PROX64 (32 bit) === No test summary line found === W7PROX64 (64 bit) === No test summary line found

May 31, 2011