On Fri, Sep 21, 2012 at 8:41 PM, Chris Teague chris.teague@gmail.com wrote:
I submitted bug 31753 for an application that I use, and started making an attempt to fix it. I started with the last few messages from the log:
fixme:win:LockWindowUpdate (0x501c4), partial stub! fixme:win:LockWindowUpdate ((nil)), partial stub! wine: Unhandled page fault on read access to 0x00000023 at address 0x78680087 (thread 0009), starting debugger...
Looking at LockWindowUpdate(), I discovered that it had no conformance tests. I wrote a few covering all the possible scenarios I could think of (only 4 of them), and ran them on wine as well as cross compiling them and running on XP. To my surprise, one of the tests fails on wine, but passes on WinXP! This was great news, so I dug a little further. Specifically the problem was attempting to do a "unlock" operation by passing NULL, when the the function was already unlocked. Windows treats this as an error and returns 0, while wine returns 1. So I fixed it, patch is attached (also at: https://dl.dropbox.com/u/477050/0001-Fixed-LockWindowUpdate.txt) I have two big questions:
- I discovered later that I don't think this is the root of my
original bug - and in fact doesn't seem to affect behavior of my program. Is it still worth submitting? I do believe it alleviates the need for the "fixme" in the LockWindowUpdate function.
The fixme is there because the function is far away from implemented, you can read more about it at the long standing bug 52 ( http://bugs.winehq.org/show_bug.cgi?id=52 ). IMO adding tests that prove something is wrong are always good.
- If it is worth submitting, could someone take a look at it and
point out all the dumb things I've done? I'm happy with the logic of the code, but I'm sure I've done something horribly wrong in terms of doing it the preferred way.
You should join all tests in the same function and use an existing file. You need to fix the whitespace issues, you didn't follow the surrounding code style (possibly your editor was configured differently from the file). You are also not freeing the created windows.
Thanks, Chris Teague
I'm not used to reviewing patches, I hope someone else can do a better review.
Best wishes, Bruno