Try to implement my first stub function - AbortPrinter() - (try 3)

[Loïc Maury]

Hello Andrew,
> 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.
Ok I will see that.
>
> 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.
Ok I will remove this TRACE and ERR.
>
> +    /* 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.
Ok
>
> +                retFromGetPrinter = GetPrinterW(hPrinter, 2, 
> (LPBYTE)pi2, needed, &needed);
> +                if(!retFromGetPrinter)
> +                    goto end;
>
> You can just do "if(!GetPrinterW(...))" and remove the local variable.
Yes the local variable is not necessary

Thank you for your help.

Loïc
>
> Thanks for contributing,
> Andrew
>