[webkit-reviews] review granted: [Bug 76172] [GTK] Add basic printing support to WebKit2 : [Attachment 122911] Updated patch

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 76172: [GTK] Add basic printing support to WebKit2
https://bugs.webkit.org/show_bug.cgi?id=76172

Attachment 122911: Updated patch
https://bugs.webkit.org/attachment.cgi?id=122911&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=122911&action=review


A bit involved, given the process boundaries, and idles always make me a little
nervous, but I think it's a great approach overall, keep rocking like this
Carlos =D

> Source/WebKit2/Shared/PrintInfo.h:54
>      explicit PrintInfo(NSPrintInfo *);

This is a bit weird, we would usually have a typedef PlatformPrintInfo for
these unifdef'ed cases, wouldn't we? Not a problem with the patch, though =)

> Source/WebKit2/Shared/gtk/ArgumentCodersGtk.cpp:255
> +    GKeyFile* keyFile = g_key_file_new();
> +    gtk_print_settings_to_key_file(printSettings, keyFile, "Print
Settings");
> +    encodeGKeyFile(encoder, keyFile);
> +    g_key_file_free(keyFile);

I think we should add a GKeyFile especialization to GOwnPtr.

> Source/WebKit2/WebProcess/WebPage/gtk/WebPrintOperationGtk.cpp:62
> +	   if (printerName) {
> +	       if (!strcmp(printerName, gtk_printer_get_name(printer)))
> +		   selectedPrinter = printer;

Nit: I think this would be more readable as if (printerName && !strcmp(...)),
then you can drop the braces.

> Source/WebKit2/WebProcess/WebPage/gtk/WebPrintOperationGtk.cpp:77
> +	   if (selectedPrinter) {
> +	       static int jobNumber = 0;
> +	       const char* applicationName = g_get_application_name();
> +	       GOwnPtr<char>jobName(g_strdup_printf("%s job #%d",
applicationName ? applicationName : "WebKit", ++jobNumber));
> +	       printOperation->m_printJob =
adoptGRef(gtk_print_job_new(jobName.get(), selectedPrinter,
> +								       
printOperation->printSettings(),
> +								       
printOperation->pageSetup()));
> +	       return TRUE;
> +	   }
> +
> +	   return FALSE;
> +    }

This looks a bit confusing, would be more readable using the early return
pattern:

 if (!selectedPrinter)
    return FALSE;
… rest of the code …
return TRUE;

> Source/WebKit2/WebProcess/WebPage/gtk/WebPrintOperationGtk.cpp:96
> +	   cairo_surface_t* surface =
gtk_print_job_get_surface(printOperation->m_printJob.get(), 0);
> +	   if (!surface) {
> +	       // FIXME: Printing error.
> +	       return;
> +	   }

Could this be moved to right after the m_printJob check, so we return as early
as possible? Wouldn't impact much, but still.... =)

> 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.

> 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()?

> Source/WebKit2/WebProcess/WebPage/gtk/WebPrintOperationGtk.h:54
> +    unsigned int printPages() const { return m_printPages; }

See bellow.

> Source/WebKit2/WebProcess/WebPage/gtk/WebPrintOperationGtk.h:89
> +    unsigned int m_printPagesIdleId;
> +    unsigned int m_printPages;
> +    GtkPageRange* m_pageRanges;
> +    size_t m_pageRangesCount;
> +    bool m_doRotate;

'printPages' and 'doRotate' confused me a bit, given their imperative names.
How about m_needsRotation and m_pagesToPrint (with the appropriate changes to
the accessors)?