[Webkit-unassigned] [Bug 22898] [GTK] Patch to complete GtkPrint API

[bugzilla-daemon at webkit.org]

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


zecke at selfish.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #26119|review?                     |review-
               Flag|                            |




------- Comment #8 from zecke at selfish.org  2008-12-19 09:11 PDT -------
(From update of attachment 26119)
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.


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