[webkit-reviews] review denied: [Bug 22898] [GTK] Patch to complete GtkPrint API : [Attachment 26119] Patch that completes and makes GtkPrint API public

[bugzilla-daemon at webkit.org]

Holger Freyther <zecke at selfish.org> has denied Colin Leroy <colin at colino.net>'s
request for review:
Bug 22898: [GTK] Patch to complete GtkPrint API
https://bugs.webkit.org/show_bug.cgi?id=22898

Attachment 26119: Patch that completes and makes GtkPrint API public
https://bugs.webkit.org/attachment.cgi?id=26119&action=review

------- Additional Comments from Holger Freyther <zecke at selfish.org>
Please see http://webkit.org/coding/contributing.html on Coding Style, how to
prepare a ChangeLog, etc... For the style issues and the lack of ChangeLog I
set this to r=-.




> -    webkit_web_frame_print(kit(frame));
> +    GError *error = NULL;
> +    webkit_web_frame_print(kit(frame), NULL, NULL, &error, NULL);
> +    if (error) {
> +	GtkWidget* topLevel =
gtk_widget_get_toplevel(GTK_WIDGET(webkit_web_frame_get_web_view(kit(frame))));

> +	if (!GTK_WIDGET_TOPLEVEL(topLevel))

indention is completely wrong. Four spaces. So the topLevel would already be at
level 8... So please use the right amount of spaces with a simple

   if (!error)
      return;

you just avoid deeply nested code, this is something that me and others aim
for.


Please try to avoid the usage of NULL in C++, it is not well defined.


>  void ChromeClient::exceededDatabaseQuota(Frame* frame, const String&)
> Index: WebKit/gtk/webkit/webkitwebframe.cpp
> ===================================================================
> --- WebKit/gtk/webkit/webkitwebframe.cpp	(revision 39335)
> +++ WebKit/gtk/webkit/webkitwebframe.cpp	(working copy)
> @@ -5,6 +5,7 @@
>   * Copyright (C) 2008 Christian Dywan <christian at imendio.com>
>   * Copyright (C) 2008 Collabora Ltd.
>   * Copyright (C) 2008 Nuanti Ltd.
> + * Copyright (C) 2008 Colin Leroy <colin at colino.net>.
>   *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Library General Public
> @@ -548,7 +549,7 @@
>      printContext->end();
>  }
>  
> -void webkit_web_frame_print(WebKitWebFrame* frame)
> +static void frame_print(WebKitWebFrame* frame, GtkPrintSettings *settings,
GtkPageSetup *page_setup, const gchar *filename, GError **error,
GtkPrintSettings **updated_settings)
>  {
>      GtkWidget* topLevel =
gtk_widget_get_toplevel(GTK_WIDGET(webkit_web_frame_get_web_view(frame)));
>      if (!GTK_WIDGET_TOPLEVEL(topLevel))
> @@ -558,33 +559,66 @@
>      ASSERT(coreFrame);
>  
>      PrintContext printContext(coreFrame);
> +    GtkPrintOperationAction action;
>  
>      GtkPrintOperation* op = gtk_print_operation_new();
> -    g_signal_connect(op, "begin-print", G_CALLBACK(begin_print),
&printContext);
> -    g_signal_connect(op, "draw-page", G_CALLBACK(draw_page), &printContext);

> -    g_signal_connect(op, "end-print", G_CALLBACK(end_print), &printContext);

> -    GError *error = NULL;
> -    gtk_print_operation_run(op, GTK_PRINT_OPERATION_ACTION_PRINT_DIALOG,
GTK_WINDOW(topLevel), &error);
> -    g_object_unref(op);
> +    g_signal_connect(G_OBJECT(op), "begin-print", G_CALLBACK(begin_print),
&printContext);
> +    g_signal_connect(G_OBJECT(op), "draw-page", G_CALLBACK(draw_page),
&printContext);
> +    g_signal_connect(G_OBJECT(op), "end-print", G_CALLBACK(end_print),
&printContext);


The G_OBJECT cast is not needed, g_signal_connect takes a gpointer and Xan
cleaned up some code to remove the bogus cast and we should not reintroduce .


>  
> -    if (error) {
> -	   GtkWidget* dialog = gtk_message_dialog_new(GTK_WINDOW(topLevel),
> -						     
GTK_DIALOG_DESTROY_WITH_PARENT,
> -						      GTK_MESSAGE_ERROR,
> -						      GTK_BUTTONS_CLOSE,
> -						      "%s", error->message);
> -	   g_error_free(error);
> +    if (filename) {
> +	   gtk_print_operation_set_export_filename(op, filename);
> +	action = GTK_PRINT_OPERATION_ACTION_EXPORT;

indention messed up again?


> +    if (result == GTK_PRINT_OPERATION_RESULT_APPLY && updated_settings) {
> +	   /* If the user printed, and the caller is interested in updated
settings, 
> +	    * give them back to him 
> +	    */

please see the CodingStyle, add a new line... 


Besides the style issues the patch looks fine and here is the catch. If we want
to put that into webkitwebframe.h we will have to discuss the API which maybe
adds another week or two delay, if you add the printing to private API I can
just r=+ the updated patch. Both things are fine with me, it is up to you.