Bug 76172 – [GTK] Add basic printing support to WebKit2

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

We need to be able to pass GtkSettings and GtkPageSetup from UI to Web process and implement basic printing features in the Web process:
 - Page ranges
 - Paper size and margins
 - Orientation

[Carlos Garcia Campos]

Created attachment 122233 [details]
Patch

It only implements printing for unix platforms, but it already includes an empty class for win32 to make it easier for someone to implement it.

[Carlos Garcia Campos]

Btw, error and print progress reporting hasn't been implemented yet, because there aren't IPC messages for that.

[Carlos Garcia Campos]

I've realized that method used by mac and win drawPagesToPDF has a callback that is emitted on the UI process for every page drawn. That would allow us to implement print progress and the UI process would know when the printing has finished to call endPrinting(). I'll update the patch next week to follow the same approach.

[Carlos Garcia Campos]

Created attachment 122755 [details]
Updated patch

It adds a new message drawPagesForPrinting using a callback (a void callback for now) to notify the ui process when the print operation has finished.

[Carlos Garcia Campos]

Created attachment 122911 [details]
Updated patch

Patch updated again, this time to allow sending empty print settings and page setup from ui to web process.

[Gustavo Noronha (kov)]

Comment on attachment 122911 [details]
Updated patch

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

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

Committed r106102: <http://trac.webkit.org/changeset/106102>

[Nikolas Zimmermann]

(In reply to comment #8)
> Committed r106102: <http://trac.webkit.org/changeset/106102>

This broke the gtk bots, not sure why. http://build.webkit.org/builders/GTK%20Linux%2032-bit%20Release/builds/21217

[Carlos Garcia Campos]

(In reply to comment #9)
> (In reply to comment #8)
> > Committed r106102: <http://trac.webkit.org/changeset/106102>
> 
> This broke the gtk bots, not sure why. http://build.webkit.org/builders/GTK%20Linux%2032-bit%20Release/builds/21217

This is not actually related to this particular commit, it sometimes happens when building and it's related to parallel builds.

[Carlos Garcia Campos]

Closing again.

[Gustavo Noronha (kov)]

(In reply to comment #7)
> > > +        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.

I understand that, but the if (printername && !strcmp()) I suggested would work the same the current code does, or am I missing something? =)
 
> > > 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.

Ack.

[Gustavo Noronha (kov)]

(In reply to comment #10)
> > This broke the gtk bots, not sure why. http://build.webkit.org/builders/GTK%20Linux%2032-bit%20Release/builds/21217
> 
> This is not actually related to this particular commit, it sometimes happens when building and it's related to parallel builds.

This problem has now been fixed: http://trac.webkit.org/changeset/106132

[Carlos Garcia Campos]

(In reply to comment #12)
> (In reply to comment #7)
> > > > +        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.
> 
> I understand that, but the if (printername && !strcmp()) I suggested would work the same the current code does, or am I missing something? =)

Yes, but in that case I would need to check the printername is not NULL in both if branches

if (printername && !strcmp())
...
else if (!printername && printer_is_default)

So, I thought it was more clear that in case we have a printerName we are not interested in whether the printer is the default one or not.

[Martin Robinson]

Comment on attachment 122911 [details]
Updated patch

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.

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

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

[Carlos Garcia Campos]

(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