[Webkit-unassigned] [Bug 76172] [GTK] Add basic printing support to WebKit2
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Jan 26 11:07:59 PST 2012
https://bugs.webkit.org/show_bug.cgi?id=76172
Gustavo Noronha (kov) <gns at gnome.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #122911|review? |review+
Flag| |
--- Comment #6 from Gustavo Noronha (kov) <gns at gnome.org> 2012-01-26 11:07:58 PST ---
(From update of attachment 122911)
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)?
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list