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!
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...
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?
(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 !
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.
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
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 <christian@imendio.com>
> * Copyright (C) 2008 Collabora Ltd.
> * Copyright (C) 2008 Nuanti Ltd.
> + * Copyright (C) 2008 Colin Leroy <colin@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.
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!
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.
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.
(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?
(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.
(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.
(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.
(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 :)
(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 =).
(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 :)
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?
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.
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.
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.
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.
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?
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.
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?
(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.
=)
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(-)
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.
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?
(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 =).
2008-12-17 01:37 PST, Colin Leroy
2008-12-17 13:41 PST, Colin Leroy
2008-12-18 01:02 PST, Colin Leroy
2008-12-19 09:56 PST, Colin Leroy
2008-12-19 10:12 PST, Colin Leroy
2009-02-11 02:42 PST, Colin Leroy
2009-03-23 12:17 PDT, Gustavo Noronha (kov)
2009-03-23 12:24 PDT, Gustavo Noronha (kov)
2009-03-30 01:37 PDT, Colin Leroy
2009-03-30 01:39 PDT, Colin Leroy
2009-03-31 09:33 PDT, Gustavo Noronha (kov)
2009-03-31 09:33 PDT, Gustavo Noronha (kov)
2009-04-03 07:03 PDT, Gustavo Noronha (kov)
2009-04-03 07:03 PDT, Gustavo Noronha (kov)
2009-04-04 12:13 PDT, Gustavo Noronha (kov)
2009-04-06 14:39 PDT, Gustavo Noronha (kov)
2009-04-06 14:39 PDT, Gustavo Noronha (kov)
2009-04-12 16:04 PDT, Gustavo Noronha (kov)