[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 #7 from Carlos Garcia Campos <cgarcia at igalia.com>  2012-01-27 01:42:49 PST ---
(In reply to comment #6)
> (From update of attachment 122911 [details])
> 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

Thanks! The idle is to make sure we don't block the web process main loop during the whole printing, but only for every page that is rendered.

> > 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 =)

In this case the PlatformPrintInfo is a combination of GtkPrintSettings and GtkPageSetup

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

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

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

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.

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

Right. 

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

Yes.

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

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

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

Sounds good to me :-)

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