Hi Loïc,
On 04/08/2011 07:54 AM, Loïc Maury wrote:
I try always to implement my first stub function - AbortPrinter.
It's looking good. Before you submit this, you should add some tests. Take a look in <dlls/winspool.drv/tests/>. Demonstrate that AbortPrinter() on Windows behaves the same way as your function does. Make sure to test the value of GetLastError() on failure. You can run your tests through the testbot https://testbot.winehq.org/ before submitting. Look at other tests for some guidance.
Below are a couple of tiny suggestions.
+ ERR("The handle for the printer is invalid.\n"); + TRACE("Document inside for job id : %d\n", printer->doc->job_id); + ERR("No document.\n"); + TRACE("(hf : %p, job id : %d)\n", printer->doc->hf, printer->doc->job_id); + TRACE("portname : %s\n", debugstr_w(portname)); + TRACE("Remove job from the list.\n"); + TRACE("Remove document for the printer : %s.\n", debugstr_w(printer->printername));
You're outputting a lot of debug information. The ERRs aren't really errors, but just invalid values from the application which are handled appropriately. No problem here, so no need to ERR.
The other TRACE statements might be useful for debugging this particular function, but can clutter the output and make future maintenance more difficult. I'd remove them.
+ /* For each jobs, see if we have a job document in the double linked list */ + /* Get portname. */ + /* Cups Port */ + /* The document for the printer is not useful anymore. */
These comments are entirely redundant. The code is perfectly clear, and the comments will just rot if this code is updated later. Don't bother with comments like these unless you're doing something very unclear.
+ retFromGetPrinter = GetPrinterW(hPrinter, 2, (LPBYTE)pi2, needed, &needed); + if(!retFromGetPrinter) + goto end;
You can just do "if(!GetPrinterW(...))" and remove the local variable.
Thanks for contributing, Andrew