[Webkit-unassigned] [Bug 76172] [GTK] Add basic printing support to WebKit2

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=76172





--- Comment #16 from Carlos Garcia Campos <cgarcia at igalia.com>  2012-02-01 00:39:05 PST ---
(In reply to comment #15)
> (From update of attachment 122911 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122911&action=review
> 
> >>> Source/WebKit2/WebProcess/WebPage/gtk/WebPrintOperationGtk.cpp:62
> >>> +                selectedPrinter = printer;
> >> 
> >> Nit: I think this would be more readable as if (printerName && !strcmp(...)), then you can drop the braces.
> > 
> > The thing is that we don't want to use the default printer if a printer name has been chosen by the user, so I would need to check again in the else, that printerName is NULL. So the printerName check is not just to avoid a crash in strcmp, but to check whether a printer has been specified, to use the default one otherwise.
> 
> I agree with Gustavo that it's best to handle the exceptional case up front and avoid the nesting.
> 
> if ((printerName && g_strcmp0(printerName, gtk_printer_get_name(printer)) ||
>     (!printerName && !gtk_printer_is_default(printer))
>     return FALSE;
> 
> You end up checking printerName twice, but this check is so cheap it's practically free. It's a lot clearer too. Then you can just use printer and get rid of selectedPrinter.

Ok, fair enough

> >>> Source/WebKit2/WebProcess/WebPage/gtk/WebPrintOperationGtk.cpp:257
> >>> +    size_t totalToPrint;
> >> 
> >> This looks like it's the same as pages.size(), what do you think of doing away with it? Btw, I think we should use the m_* style for these as well.
> > 
> > Indeed, that's because I used and array of integers first, and then I realized it would be better to use a Vector. We use the m_ prefix for members of classes but not for structs.
> 
> Structs that have methods should be classes, so these variables should be prefixed with m_.

The coding style document doesn't say anything, and WebCore is full of examples of structs with methods and without m_ prefix.

> >>> Source/WebKit2/WebProcess/WebPage/gtk/WebPrintOperationGtk.cpp:262
> >>> +    bool isDone : 1;
> >> 
> >> How about turning this into a method that does return m_totalPrinted == pages.size()?
> > 
> > That would work now, but not when some manual capabilities are implemented like multiple pages per sheet.
> 
> Please do not use bitfields for booleans. The amount of memory this saves is negligible and we don't do this typically.

It's harmless anyway, and again WebCore is full of examples


$ grep "bool m_.* : 1" `find ./WebCore -name "*.h"` | wc -l
423

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.