[webkit-reviews] review granted: [Bug 77197] [GTK] Handle printing errors in WebKit2 : [Attachment 127987] 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 77197: [GTK] Handle printing errors in WebKit2
https://bugs.webkit.org/show_bug.cgi?id=77197

Attachment 127987: Patch
https://bugs.webkit.org/attachment.cgi?id=127987&action=review

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


OK, looks good to me!

> Source/WebKit2/UIProcess/API/gtk/WebKitError.h:112
> + * @WEBKIT_PRINT_ERROR_INVALID_PAGE_RANGE: There are no pages to print

Nit: Invalid page ranges could include pages that exist, so perhaps saying
there are no pages is not the most conceptually correct here. Go for Invalid
page range?

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:207
> +	* finishes after an error and #WebKitPrintOperation::finished signal is
emitted
> +	* after this one.

Suggestion: replace the last sentence with just The
#WebKitPrintOperation::finished signal is emitted after this one.

> Source/WebKit2/UIProcess/API/gtk/tests/TestPrinting.cpp:102
> +	   if (strcmp(gtk_printer_get_name(printer), "Print to File"))

Hrm. Something just popped on my mind. Does gtk_printer_get_name() return the
localized string? I guess all of us run our systems in English, but be good to
make this robust.