Bug 22898 – [GTK] need proper API for printing

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Colin Leroy]

Hello,

I'm looking at webkit-gtk to create an "html2pdf" based on the webkit engine. For this, I'd like to be able to batch-print frames to files without having to click on Print every time.

The attached patch does that. I hope including it could be possible!

[Colin Leroy]

Created attachment 26088 [details]
Patch to print frames directly to file

[Holger Freyther]

Comment on attachment 26088 [details]
Patch to print frames directly to file

> Index: WebKit/gtk/webkit/webkitwebframe.cpp
> ===================================================================
> --- WebKit/gtk/webkit/webkitwebframe.cpp	(revision 39335)
> +++ WebKit/gtk/webkit/webkitwebframe.cpp	(working copy)
> @@ -580,6 +580,45 @@
>      }
>  }
>  
> +
> +void webkit_web_frame_print_to_file(WebKitWebFrame* frame, GtkPrintSettings *settings, GtkPageSetup *setup, const gchar *filename)
> +{

> +    gtk_print_operation_set_export_filename(op, filename);
> +    if (page_setup)
> +        gtk_print_operation_set_default_page_setup(op, page_setup);

this does compile? I was not able to resolve page_setup, maybe the compiler is more clever than me. We will also need to consider making this official API...

[Colin Leroy]

Created attachment 26101 [details]
Fixed patch

Hum, indeed, I don't how I managed to send this broken patch ^^
This one should be better.

[Holger Freyther]

Okay the patch seems fine. My two random complains are:
  - We should consider making it public API
  - we should not run an error dialog but maybe use GError as an out param? E.g. if you automate form printing you do not want to be blocked by an error dialog...

what do you think?

[Colin Leroy]

(In reply to comment #4)
> Okay the patch seems fine. My two random complains are:
>   - We should consider making it public API

Would be nice :)

>   - we should not run an error dialog but maybe use GError as an out param?
> E.g. if you automate form printing you do not want to be blocked by an error
> dialog...

Good idea, yes.

I can update the patch if you want !

[Colin Leroy]

Created attachment 26119 [details]
Patch that completes and makes GtkPrint API public

Hello,

Here's a rather bigger patch that aims to complete the GtkPrint API and make it public.

What it does:
- WebKit/gtk/webkit/webkitwebframe.cpp: Factorizes code between print and print_to_file, as little of it changes in both cases; Removes the error dialog, leaving error handling to the caller; Adds the ability to specify page setup and print settings in the Print case too, which lets the calling app have a "Page setup" menu, and let it get back updated print settings in order to remember them. This adds a few parameters to the functions, but helps applications to have a full implementation; and add my name to the Copyright boiler-plate.
- WebKit/gtk/webkit/webkitwebframe.h: Make the API public; the only annoyance is that the API will be different whether compiled against GTK 2.10+ or not, as the parameters type didn't exist before, it's not possible anymore to have filler-functions printing a g_warning().
- WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp: Update code to use the new API, with the minimal changes needed - just error handling the same way as it was before, only at the caller level.

I tried to think of possible shortcomings to this API, because we wouldn't want to modify a public API, but I think it's complete. _print_to_file() shows no dialog, so there's no possibility to have updated settings after. GtkPageSetup can't be modified from inside a print dialog, so there are no modifications to get.

[Colin Leroy]

For completeness, here's the start of the app I'm doing, which can provide testing of the changes (both webkit_web_frame_print_to_file and webkit_web_frame_print() are used).

Its code is unclean and unfinished, but it demonstrates the thing :)

http://colino.net/tmp/webkit2pdf-0.1cvs1.tar.gz

[Holger Freyther]

Comment on attachment 26119 [details]
Patch that completes and makes GtkPrint API public

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 <[e-mail]>
>   * Copyright (C) 2008 Collabora Ltd.
>   * Copyright (C) 2008 Nuanti Ltd.
> + * Copyright (C) 2008 Colin Leroy <[e-mail]>.
>   *
>   * 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.

[Colin Leroy]

OK, I'll look at that; apart from coding style issues, is the patch fine?

[Colin Leroy]

(In reply to comment #9)
> OK, I'll look at that; apart from coding style issues, is the patch fine?

Forget that, I missed the end of the review :)

[Colin Leroy]

Created attachment 26145 [details]
updated patch that fixes CodingStyle

Hi,

Here's an updated patch that fixes the mentioned style issues. Sorry about indentations, my editor has a tendancy to replace 8 spaces with 1 tab and I usually prefer this coding style, so I let that be :) I doubled checked them this time.

About NULL usage in c++, I left them as is because I've noticed lots of NULLs in other cpp files, and I'm not sure 0x0 would be nicer... Feel free to tell me what you'd prefer if you really prefer I don't add NULLs :)

I've left the API in webframe.h, I don't mind an extra delay with this patch, and OTOH i'd like to have a defined API - there's another place where I could use it, a future Claws Mail plugin.

Thanks!

[Colin Leroy]

Created attachment 26147 [details]
updated patch that fixes CodingStyle & Changelog

Oops, and add a Changelog entry.

[Colin Leroy]

Hi,

I'm pinging back after the holidays... Has someone been able to review my last patch?

Thanks! :)

[Marco Barisione]

You cannot have a different API based on the version of GTK used to compile WebKit and that GTK version could be different from the one used to compile your app.

[Colin Leroy]

Created attachment 27556 [details]
Updated patch to have similar API with older GTK

This patch is the same as the previous one, adding the same API when built against GTK+2.8 - just replacing undefined types with void in this case.

[Colin Leroy]

(In reply to comment #14)
> You cannot have a different API based on the version of GTK used to compile
> WebKit and that GTK version could be different from the one used to compile
> your app.

My latest patch should fix that?

[Gustavo Noronha (kov)]

(In reply to comment #16)
> (In reply to comment #14)
> > You cannot have a different API based on the version of GTK used to compile
> > WebKit and that GTK version could be different from the one used to compile
> > your app.
> 
> My latest patch should fix that?
> 

I wonder if it is useful at all to support GTK+ versions < 2.10 since we are now requiring such a recent libsoup, for instance. Also, I would prefer having a signal being emitted that lets the application hand in an already prepared GtkPrintOperation, so that the application has good control over the print process, and we would only need a single API method.

[Colin Leroy]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #14)
> > > You cannot have a different API based on the version of GTK used to compile
> > > WebKit and that GTK version could be different from the one used to compile
> > > your app.
> > 
> > My latest patch should fix that?
> > 
> 
> I wonder if it is useful at all to support GTK+ versions < 2.10 since we are
> now requiring such a recent libsoup, for instance.

I've been specifically asked for GTK < 2.10 support (or rather API compatilibity). It doesn't add much to the patch.

> Also, I would prefer having
> a signal being emitted that lets the application hand in an already prepared
> GtkPrintOperation, so that the application has good control over the print
> process, and we would only need a single API method.

The current patch already provides good control in my opinion, and even if it needs two API methods, this way seems more straight-forward for application developers (at least to me) than having to setup a signal callback which will probably require a specific structure to pass GtkPrintSettings and GtkPageSetup back and forth.

Printing to file is already possible from within the print dialog, so application developers who only need to print on user's request should be fine with the existing implementation anyway, or with this implemenation using only one API method (webkit_web_frame_print).

The goal of this patch is to be able to print to file without user interaction at all.

[Gustavo Noronha (kov)]

(In reply to comment #18)
> > I wonder if it is useful at all to support GTK+ versions < 2.10 since we are
> > now requiring such a recent libsoup, for instance.
> 
> I've been specifically asked for GTK < 2.10 support (or rather API
> compatilibity). It doesn't add much to the patch.

By who? With what rationale? It's just a small amount of code, indeed, but I am not convinced we need it at all.

> The current patch already provides good control in my opinion, and even if it
> needs two API methods, this way seems more straight-forward for application
> developers (at least to me) than having to setup a signal callback which will
> probably require a specific structure to pass GtkPrintSettings and GtkPageSetup
> back and forth.

I think passing a GtkPrintOperation, really, much in the same way the create-web-view does with WebKitWebView. I am worried that we limit the amount of control the application has regarding the printing process - for instance, displaying progress.

> Printing to file is already possible from within the print dialog, so
> application developers who only need to print on user's request should be fine
> with the existing implementation anyway, or with this implemenation using only
> one API method (webkit_web_frame_print).
> 
> The goal of this patch is to be able to print to file without user interaction
> at all.

Right. The way I understand this, though, ChromeClient::print may be called by javascript code requesting printing, for instance. The current implementation will popup the print dialog if this happens, which may not be what the application intends - it may just as well want to print to a file automatically, with an automatically-generated name, or send the job with a pre-defined configuration to a pre-defined printer.

Coupled with my worries about the amount of control the application is able to have regarding the printing process as a whole, I wonder if adding a print-requested signal or something like that, which expects the client to give it an already setup GtkPrintOperation is not the way to go.

[Colin Leroy]

(In reply to comment #19)

> > I've been specifically asked for GTK < 2.10 support (or rather API
> > compatilibity). It doesn't add much to the patch.
> 
> By who? With what rationale? It's just a small amount of code, indeed, but I am
> not convinced we need it at all.

See comment #14...
 
> I think passing a GtkPrintOperation, really, much in the same way the
> create-web-view does with WebKitWebView. I am worried that we limit the amount
> of control the application has regarding the printing process - for instance,
> displaying progress.
> 
> > [...]
> 
> Right. The way I understand this, though, ChromeClient::print may be called by
> javascript code requesting printing, for instance. The current implementation
> will popup the print dialog if this happens, which may not be what the
> application intends - it may just as well want to print to a file
> automatically, with an automatically-generated name, or send the job with a
> pre-defined configuration to a pre-defined printer.
> 
> Coupled with my worries about the amount of control the application is able to
> have regarding the printing process as a whole, I wonder if adding a
> print-requested signal or something like that, which expects the client to give
> it an already setup GtkPrintOperation is not the way to go.

It may be good to have an "official" view on this from Webkit's team. 

Anyway, I lack time to go through complete rewrites of this patch. If the Webkit developers prefer your way of doing, I'll let you write the patch :)

[Gustavo Noronha (kov)]

(In reply to comment #20)
> (In reply to comment #19)
> > > I've been specifically asked for GTK < 2.10 support (or rather API
> > > compatilibity). It doesn't add much to the patch.
> > 
> > By who? With what rationale? It's just a small amount of code, indeed, but I am
> > not convinced we need it at all.
> 
> See comment #14...

OK, what I read there is that we should not be exporting different public APIs depending on GTK+ version. This makes perfect sense, but it doesn't mean we _need_ to support older GTK+ versions. I believe we can focus on gtk > 2.10 from now on, if nobody objects to it for some reason.

> It may be good to have an "official" view on this from Webkit's team. 
> 
> Anyway, I lack time to go through complete rewrites of this patch. If the
> Webkit developers prefer your way of doing, I'll let you write the patch :)

Yeah, I think we need to have more API users comment on this, but I'll be glad to write a patch later this month, if need be =). 

[Colin Leroy]

(In reply to comment #21)

> > See comment #14...
> 
> OK, what I read there is that we should not be exporting different public APIs
> depending on GTK+ version. This makes perfect sense, but it doesn't mean we
> _need_ to support older GTK+ versions. I believe we can focus on gtk > 2.10
> from now on, if nobody objects to it for some reason.

Yes, that's what the patch does (and stock Webkit too): just a g_warning when trying to print on GTK < 2.10.

> Yeah, I think we need to have more API users comment on this, but I'll be glad
> to write a patch later this month, if need be =). 
 
OK, great :) The size of Webkit's source code makes it painful to maintain my patch locally, and handle it cleanly against my package manager, so I'd love to have some way to do what I need upstream - even if it means rewriting 10-50 lines of my own code :)

[Gustavo Noronha (kov)]

*** Bug 16493 has been marked as a duplicate of this bug. ***

[Gustavo Noronha (kov)]

The bug I marked as duplicate above has a good API suggestion (as well as some discussion regarding goals) IMO. I'll try to implement a patch and post it here. I don't think we will be able to free WebKitFrame from GTK+, though, opinions on that?

[Gustavo Noronha (kov)]

Created attachment 28859 [details]
proposed printing API

Notice that with this patch I am removing support for GTK+ < 2.10. I believe we should start requiring GTK+ >= 2.10 from now on. I will prepare a patch that does that change, too.

[Gustavo Noronha (kov)]

Created attachment 28860 [details]
proposed API

Fixed some small documentation issues.

[Colin Leroy]

Looks good from my point of view... 

[Jérémy Lal]

looks good to me too. In fact i'm using it right now, and it works quite well.
I wonder when it will be available upstream ?

[Christian Dywan]

Could you post a patch or source file using the proposed API? I don't really have a good idea of printing and real use cases are the best way to find out what an interface can do.

[Colin Leroy]

Created attachment 29054 [details]
API usage example

This file shows various uses of the print API implemented in attachment #28860 [details].

(It doesn't compile as is, but gives a good idea of what can be done).

From my point of view, it's fine and indeed, better than my patch.

[Colin Leroy]

Created attachment 29055 [details]
(Fixed) API usage example

Same example with a bug fixed in the callback ;)

[Gustavo Noronha (kov)]

Created attachment 29121 [details]
proposed printing API


        Reviewed by NOBODY (OOPS!).

        https://bugs.webkit.org/show_bug.cgi?id=22898
        [GTK] need proper API for printing

        Implement proper printing API, using the GTK+ printing API.

        * WebCoreSupport/ChromeClientGtk.cpp:
        (WebKit::ChromeClient::print):
        * webkit/webkitprivate.h:
        * webkit/webkitwebframe.cpp:
        * webkit/webkitwebframe.h:
        * webkit/webkitwebview.cpp:
---
 WebKit/gtk/ChangeLog                          |   16 +++++
 WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp |    9 +++-
 WebKit/gtk/webkit/webkitprivate.h             |    3 -
 WebKit/gtk/webkit/webkitwebframe.cpp          |   84 ++++++++++++++++--------
 WebKit/gtk/webkit/webkitwebframe.h            |   11 +++
 WebKit/gtk/webkit/webkitwebview.cpp           |   29 +++++++++
 6 files changed, 120 insertions(+), 32 deletions(-)

[Gustavo Noronha (kov)]

Created attachment 29122 [details]
unit tests for printing


        Reviewed by NOBODY (OOPS!).

        https://bugs.webkit.org/show_bug.cgi?id=22898
        [GTK] need proper API for printing

        Added simple printing unit tests.

        * tests/testwebframe.c:
        (print_requested_cb):
        (print_timeout):
        (test_webkit_web_frame_printing):
        (main):
---
 WebKit/gtk/ChangeLog            |   15 +++++++
 WebKit/gtk/tests/testwebframe.c |   81 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 96 insertions(+), 0 deletions(-)

[Gustavo Noronha (kov)]

Comment on attachment 29121 [details]
proposed printing API

This patch has one important change compared to the original one I posted: it makes webkit_web_frame_print take only a WebKitWebFrame, to better acomodate clients which are already messing with that symbol, and adds a webkit_web_frame_print_full to provide the more flexible functionality.

[Holger Freyther]

Comment on attachment 29122 [details]
unit tests for printing


> +
> +    g_assert_cmpint(GPOINTER_TO_INT(g_object_get_data(G_OBJECT(webView), "signal-handled")), ==, TRUE);

great!!!

> +
> +    // Does printing directly to a file?
> +    GError *error = NULL;
> +    gchar* temporaryFilename = NULL;
> +    gint fd = g_file_open_tmp ("webkit-testwebframe-XXXXXX", &temporaryFilename, &error);
> +    close(fd);
> +
> +    // We delete the file, so that we can easily figure out that the
> +    // file got printed;
> +    if (error || (g_unlink(temporaryFilename) == -1)) {
> +        g_critical("Failed to open a temporary file for writing: %s.", error->message);
> +        g_error_free(error);
> +        goto cleanup;
> +    }


> +    gtk_print_operation_set_export_filename(operation, temporaryFilename);
> +    result = webkit_web_frame_print_full (webFrame, operation, action, error);


This looks like a race. Will GtkPrint just overwrite the export file?

[Holger Freyther]

Comment on attachment 29121 [details]
proposed printing API


> -        g_signal_connect(dialog, "response", G_CALLBACK(gtk_widget_destroy), NULL);
> +        g_signal_connect(dialog, "response", G_CALLBACK(gtk_widget_destroy), 0);

unrelated but okay.



> +    GtkWidget* topLevel = gtk_widget_get_toplevel(GTK_WIDGET(webkit_web_frame_get_web_view(frame)));
> +    if (!GTK_WIDGET_TOPLEVEL(topLevel))
> +        topLevel = NULL;

not consistent with the one above :)


> +
> +    Frame* coreFrame = core(frame);
> +    if (!coreFrame)
> +        return GTK_PRINT_OPERATION_RESULT_CANCEL

Why this and not GTK_PRINT_OPERATION_RESULT_ERROR?



> --- a/WebKit/gtk/webkit/webkitwebframe.h
> +++ b/WebKit/gtk/webkit/webkitwebframe.h
> @@ -22,6 +22,8 @@
>  #define WEBKIT_WEB_FRAME_H
>  
>  #include <glib-object.h>
> +#include <gtk/gtk.h>

oh interesting, the first Gtk+ symbol here :)


Yeah, this and the test case look pretty good.

[Gustavo Noronha (kov)]

yo!

(In reply to comment #35)
> (From update of attachment 29122 [details] [review])
> > +
> > +    // Does printing directly to a file?
> > +    GError *error = NULL;
> > +    gchar* temporaryFilename = NULL;
> > +    gint fd = g_file_open_tmp ("webkit-testwebframe-XXXXXX", &temporaryFilename, &error);
> > +    close(fd);
> > +
> > +    // We delete the file, so that we can easily figure out that the
> > +    // file got printed;
> > +    if (error || (g_unlink(temporaryFilename) == -1)) {
> > +        g_critical("Failed to open a temporary file for writing: %s.", error->message);
> > +        g_error_free(error);
> > +        goto cleanup;
> > +    }
> 
> 
> > +    gtk_print_operation_set_export_filename(operation, temporaryFilename);
> > +    result = webkit_web_frame_print_full (webFrame, operation, action, error);
> 
> 
> This looks like a race. Will GtkPrint just overwrite the export file?
> 

Yes. That doesn't seem to be documented, but from what I could understand from gtk+'s source code, and from my tests, the file is simply overwritten. Why do you think there is a race here?

[Gustavo Noronha (kov)]

(In reply to comment #36)
> (From update of attachment 29121 [details] [review])
> 
> > -        g_signal_connect(dialog, "response", G_CALLBACK(gtk_widget_destroy), NULL);
> > +        g_signal_connect(dialog, "response", G_CALLBACK(gtk_widget_destroy), 0);
> 
> unrelated but okay.

This was actually a mistake. We have agreed to use NULL on API files, so the second one is correct!

> > +    Frame* coreFrame = core(frame);
> > +    if (!coreFrame)
> > +        return GTK_PRINT_OPERATION_RESULT_CANCEL
> 
> Why this and not GTK_PRINT_OPERATION_RESULT_ERROR?

My thinking was that the operation has not been attempted, so no error happened. It was cancelled because the frame it was supposed to print does not exist, but on second thought, given that we are in the context of WebKitWebFrame, and that CANCEL also means 'print settings should not be stored', I think ERROR is a better option indeed.

> >  #include <glib-object.h>
> > +#include <gtk/gtk.h>
> 
> oh interesting, the first Gtk+ symbol here :)

I guess we could move all this to WebKitWebView, and we would have stuff such as:

GtkPrintOperationResult webkit_web_view_print_frame(WebKitWebView* view, WebKitWebFrame* frame, GError* error);
GtkPrintOperationResult webkit_web_view_print_frame_full(WebKitWebView* view, WebKitWebFrame* frame, GtkPrintOperation* operation, GtkPrintOperationAction action, GError* error);

> Yeah, this and the test case look pretty good.
 
=)

[Gustavo Noronha (kov)]

Created attachment 29226 [details]
proposed printing API


        Reviewed by NOBODY (OOPS!).

        https://bugs.webkit.org/show_bug.cgi?id=22898
        [GTK] need proper API for printing

        Implement proper printing API, using the GTK+ printing API.

        * WebCoreSupport/ChromeClientGtk.cpp:
        (WebKit::ChromeClient::print):
        * webkit/webkitprivate.h:
        * webkit/webkitwebframe.cpp:
        * webkit/webkitwebframe.h:
        * webkit/webkitwebview.cpp:
---
 WebKit/gtk/ChangeLog                          |   16 +++++
 WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp |    9 +++-
 WebKit/gtk/webkit/webkitprivate.h             |    3 -
 WebKit/gtk/webkit/webkitwebframe.cpp          |   77 +++++++++++++++++++------
 WebKit/gtk/webkit/webkitwebframe.h            |   11 ++++
 WebKit/gtk/webkit/webkitwebview.cpp           |   29 +++++++++
 6 files changed, 122 insertions(+), 23 deletions(-)

[Gustavo Noronha (kov)]

Created attachment 29227 [details]
unit tests for printing


        Reviewed by NOBODY (OOPS!).

        https://bugs.webkit.org/show_bug.cgi?id=22898
        [GTK] need proper API for printing

        Added simple printing unit tests.

        * tests/testwebframe.c:
        (print_requested_cb):
        (print_timeout):
        (test_webkit_web_frame_printing):
        (main):
---
 WebKit/gtk/ChangeLog            |   15 +++++++
 WebKit/gtk/tests/testwebframe.c |   81 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 96 insertions(+), 0 deletions(-)

[Gustavo Noronha (kov)]

Created attachment 29256 [details]
When a download is requested by an ongoing request, use the already

provided response to set the suggested filename for the WebKitDownload
object, if available.
---
 WebKit/gtk/ChangeLog                               |   20 +++++++++++++++++
 WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp |    9 ++++++-
 WebKit/gtk/webkit/webkitdownload.cpp               |   23 +++++++++++++------
 WebKit/gtk/webkit/webkitprivate.h                  |    5 +++-
 WebKit/gtk/webkit/webkitwebview.cpp                |    7 +++++-
 5 files changed, 53 insertions(+), 11 deletions(-)

[Gustavo Noronha (kov)]

Comment on attachment 29256 [details]
When a download is requested by an ongoing request, use the already

Ugh... =) bad usage of shell history when using git-send-bugzilla.

[Gustavo Noronha (kov)]

Created attachment 29288 [details]
proposed printing API


        Reviewed by NOBODY (OOPS!).

        https://bugs.webkit.org/show_bug.cgi?id=22898
        [GTK] need proper API for printing

        Implement proper printing API, using the GTK+ printing API.

        * WebCoreSupport/ChromeClientGtk.cpp:
        (WebKit::ChromeClient::print):
        * webkit/webkitprivate.h:
        * webkit/webkitwebframe.cpp:
        * webkit/webkitwebframe.h:
        * webkit/webkitwebview.cpp:
---
 WebKit/gtk/ChangeLog                          |   16 ++++++
 WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp |    9 +++-
 WebKit/gtk/webkit/webkitprivate.h             |    3 -
 WebKit/gtk/webkit/webkitwebframe.cpp          |   65 ++++++++++++++++++++-----
 WebKit/gtk/webkit/webkitwebframe.h            |   11 ++++
 WebKit/gtk/webkit/webkitwebview.cpp           |   29 +++++++++++
 6 files changed, 116 insertions(+), 17 deletions(-)

[Gustavo Noronha (kov)]

Created attachment 29289 [details]
unit tests for printing


        Reviewed by NOBODY (OOPS!).

        https://bugs.webkit.org/show_bug.cgi?id=22898
        [GTK] need proper API for printing

        Added simple printing unit tests.

        * tests/testwebframe.c:
        (print_requested_cb):
        (print_timeout):
        (test_webkit_web_frame_printing):
        (main):
---
 WebKit/gtk/ChangeLog            |   15 +++++++
 WebKit/gtk/tests/testwebframe.c |   86 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 101 insertions(+), 0 deletions(-)

[Gustavo Noronha (kov)]

Comment on attachment 29289 [details]
unit tests for printing

fixing a crasher spotted by chpe

[Holger Freyther]

Comment on attachment 29288 [details]
proposed printing API


> +     * Emitted when printing is requested by the frame, usually
> +     * because of a javascript call. While handling this signal you
> +     * should call webkit_web_frame_print() to do the actual printing.

s/While/When/ ???



> +    webkit_web_view_signals[PRINT_REQUESTED] = g_signal_new("print-requested",
> +            G_TYPE_FROM_CLASS(webViewClass),
> +            (GSignalFlags)(G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION),
> +            0,
> +            NULL,
> +            NULL,
> +            webkit_marshal_BOOLEAN__OBJECT,
> +            G_TYPE_BOOLEAN, 1,
> +            WEBKIT_TYPE_WEB_FRAME);

Should we use g_signal_accumulator_true_handled to stop the signal emission?

[Gustavo Noronha (kov)]

(In reply to comment #46)
> > +     * Emitted when printing is requested by the frame, usually
> > +     * because of a javascript call. While handling this signal you
> > +     * should call webkit_web_frame_print() to do the actual printing.
> 
> s/While/When/ ???

Yep =) 

> > +    webkit_web_view_signals[PRINT_REQUESTED] = g_signal_new("print-requested",
> > +            G_TYPE_FROM_CLASS(webViewClass),
> > +            (GSignalFlags)(G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION),
> > +            0,
> > +            NULL,
> > +            NULL,
> > +            webkit_marshal_BOOLEAN__OBJECT,
> > +            G_TYPE_BOOLEAN, 1,
> > +            WEBKIT_TYPE_WEB_FRAME);
> 
> Should we use g_signal_accumulator_true_handled to stop the signal emission?

Yes, indeed. Adding it =).

[Gustavo Noronha (kov)]

Created attachment 29424 [details]
printing API

[Holger Freyther]

Comment on attachment 29424 [details]
printing API

go ahead.

[Gustavo Noronha (kov)]

Landed as r42435 and r42436.

[Colin Leroy]

(In reply to comment #50)
> Landed as r42435 and r42436.

Great, thanks to everybody involved in that :)