Bug 76448 - [GTK] Add WebKitPrintOperation to WebKit2 GTK+ API
: [GTK] Add WebKitPrintOperation to WebKit2 GTK+ API
Status: RESOLVED FIXED
: WebKit
WebKit2
: 528+ (Nightly build)
: PC Linux
: P2 Normal
Assigned To:
:
: Gtk
: 76172
: 75544 76536
  Show dependency treegraph
 
Reported: 2012-01-17 06:24 PST by
Modified: 2012-02-16 09:11 PST (History)


Attachments
Patch (39.09 KB, patch)
2012-01-17 06:41 PST, Carlos Garcia Campos
gns: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch updated to apply on current git master (37.31 KB, patch)
2012-02-14 02:13 PST, Carlos Garcia Campos
pnormand: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Fix gtk-doc warnings (37.31 KB, patch)
2012-02-14 02:43 PST, Carlos Garcia Campos
mrobinson: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-01-17 06:24:29 PST
Initial printing API.
------- Comment #1 From 2012-01-17 06:41:49 PST -------
Created an attachment (id=122759) [details]
Patch

For now it just adds API to show the print dialog, used by the UI client to implement printFrame callback too, so that it can be tested with the MiniBrowser.
------- Comment #2 From 2012-01-17 06:45:25 PST -------
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
------- Comment #3 From 2012-01-17 10:59:09 PST -------
(From update of attachment 122759 [details])
Attachment 122759 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11268265
------- Comment #4 From 2012-01-17 23:40:53 PST -------
(In reply to comment #3)
> (From update of attachment 122759 [details] [details])
> Attachment 122759 [details] [details] did not pass gtk-ews (gtk):
> Output: http://queues.webkit.org/results/11268265

This is because this patch depends on patch attached to bug #76172.
------- Comment #5 From 2012-02-13 04:31:08 PST -------
(From update of attachment 122759 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=122759&action=review

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:251
> +    bool doPrint = webkitPrintOperationRunDialogUnix(printOperation, parent);

How about shouldPrint?

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:371
> +void webkit_print_operation_run_dialog(WebKitPrintOperation* printOperation, GtkWindow* parent)

Not sure about this. Blocking on this function is not very elegant I think (although it is indeed what GtkPrintOperation would do). Two options I'd like us to consider: a non-blocking _present_dialog() with the same parameters that shows the dialog but instead of running the main loop connects to signals and handles everything automatically, or a way to obtain the dialog and show it ourselves, with a _print() call that does the actual printing. Do you see a problem with any of those? Or do you think this use case is already handled by the API user showing a dialog themselves and then using webkit_print_operation_print() from bug 76536?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:573
> +     * Emitted when printing is requested on @web_view, usually by a javascript call,
> +     * before the print dialog is shown. This signal can be used to set the initial
> +     * print settings and page setup of @print_operation, by calling
> +     * webkit_print_operation_set_print_settings() and webkit_print_operation_set_page_setup().
> +     *
> +     * You can connect to this signal and return %TRUE to cancel the print operation.

So, when I set the default settings, do I return FALSE to not cancel the operation? I guess so from the code, but the doc is a bit confusing.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:106
> +    gboolean   (* script_prompt)  (WebKitWebView         *web_view,
> +                                   const gchar           *message,
> +                                   const gchar           *default_text,
> +                                   gchar                **text);
> +
> +    gboolean   (* print_requested) (WebKitWebView        *web_view,
> +                                    WebKitPrintOperation *print_operation);

They seem to be misaligned by one.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:213
> +

Extra line intended?
------- Comment #6 From 2012-02-13 05:32:56 PST -------
(In reply to comment #5)
> (From update of attachment 122759 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122759&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:251
> > +    bool doPrint = webkitPrintOperationRunDialogUnix(printOperation, parent);
> 
> How about shouldPrint?

Sounds good too.

> > Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:371
> > +void webkit_print_operation_run_dialog(WebKitPrintOperation* printOperation, GtkWindow* parent)
> 
> Not sure about this. Blocking on this function is not very elegant I think (although it is indeed what GtkPrintOperation would do).

I read somewhere that print is supposed to be a sync operation. All browsers I've tested show a modal print dialog (chromium, ephy, firefox, arora, ...). GtkPrintOperation can run the dialog asynchronouslym in unix. 

> Two options I'd like us to consider: a non-blocking _present_dialog() with the same parameters that shows the dialog but instead of running the main loop connects to signals and handles everything automatically,

This is possible in unix, but not in win32.

> or a way to obtain the dialog and show it ourselves, with a _print() call that does the actual printing.

Again, this can't be done in win32.

> Do you see a problem with any of those? Or do you think this use case is already handled by the API user showing a dialog themselves and then using webkit_print_operation_print() from bug 76536?

That's a possibility, although that api is thought to print without showing any print dialog at all, nothing prevents the uiser to implement their own printing dialog and call webkit_print_operation_print(). 

> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:573
> > +     * Emitted when printing is requested on @web_view, usually by a javascript call,
> > +     * before the print dialog is shown. This signal can be used to set the initial
> > +     * print settings and page setup of @print_operation, by calling
> > +     * webkit_print_operation_set_print_settings() and webkit_print_operation_set_page_setup().
> > +     *
> > +     * You can connect to this signal and return %TRUE to cancel the print operation.
> 
> So, when I set the default settings, do I return FALSE to not cancel the operation? I guess so from the code, but the doc is a bit confusing.

Yes, the next line in the docs says:

Returns: %TRUE to stop other handlers from being invoked for the event. %FALSE to propagate the event further.

So, you always have to return FALSE if you want the even to be propagated. I'll try to explain it better anyway. 

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:106
> > +    gboolean   (* script_prompt)  (WebKitWebView         *web_view,
> > +                                   const gchar           *message,
> > +                                   const gchar           *default_text,
> > +                                   gchar                **text);
> > +
> > +    gboolean   (* print_requested) (WebKitWebView        *web_view,
> > +                                    WebKitPrintOperation *print_operation);
> 
> They seem to be misaligned by one.

Right.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:213
> > +
> 
> Extra line intended?

Nope :-P
------- Comment #7 From 2012-02-14 02:13:50 PST -------
Created an attachment (id=126940) [details]
Patch updated to apply on current git master

Rebased and fixed review comments.
------- Comment #8 From 2012-02-14 02:16:57 PST -------
Attachment 126940 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets
Using index info to reconstruct a base tree...
<stdin>:1578: trailing whitespace.

<stdin>:1647: trailing whitespace.

<stdin>:1657: trailing whitespace.

<stdin>:1672: trailing whitespace.
        return 0;        
<stdin>:1674: trailing whitespace.

warning: squelched 7 whitespace errors
warning: 12 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 168765 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #9 From 2012-02-14 02:28:14 PST -------
(From update of attachment 126940 [details])
Attachment 126940 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11516185
------- Comment #10 From 2012-02-14 02:43:10 PST -------
Created an attachment (id=126944) [details]
Fix gtk-doc warnings

Sorry, I got a content conflict on every single file of the patch while rebasing and made a mistake resolving the file webkit2gtk-sections.txt
------- Comment #11 From 2012-02-14 02:45:20 PST -------
Attachment 126944 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets
Using index info to reconstruct a base tree...
<stdin>:1578: trailing whitespace.

<stdin>:1647: trailing whitespace.

<stdin>:1657: trailing whitespace.

<stdin>:1672: trailing whitespace.
        return 0;        
<stdin>:1674: trailing whitespace.

warning: squelched 7 whitespace errors
warning: 12 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 168766 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Tools/ChangeLog
CONFLICT (content): Merge conflict in Tools/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #12 From 2012-02-16 07:29:10 PST -------
(From update of attachment 126944 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=126944&action=review

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:86
> +    priv->webViewDestroyedId = g_signal_connect(priv->webView, "destroy", G_CALLBACK(webViewDestroyed), printOperation);

Do you think it would be slightly cleaner to use a weak reference here? Not a big deal either way.

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:248
> +        if (gtk_widget_is_toplevel(toplevel) && GTK_IS_WINDOW(toplevel))
> +            parent = GTK_WINDOW(toplevel);

Would it make sense to use the new helper here?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:665
> +     * You can connect to this signal and return %TRUE to cancel the print operation.

Might want to add "or implement your own print dialog"
------- Comment #13 From 2012-02-16 08:37:37 PST -------
(In reply to comment #12)
> (From update of attachment 126944 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126944&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:86
> > +    priv->webViewDestroyedId = g_signal_connect(priv->webView, "destroy", G_CALLBACK(webViewDestroyed), printOperation);
> 
> Do you think it would be slightly cleaner to use a weak reference here? Not a big deal either way.

Transient-for is implemented this way in GtkWindow.

> > Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:248
> > +        if (gtk_widget_is_toplevel(toplevel) && GTK_IS_WINDOW(toplevel))
> > +            parent = GTK_WINDOW(toplevel);
> 
> Would it make sense to use the new helper here?

Indeed

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:665
> > +     * You can connect to this signal and return %TRUE to cancel the print operation.
> 
> Might want to add "or implement your own print dialog"

Sure!
------- Comment #14 From 2012-02-16 09:11:26 PST -------
Committed r107947: <http://trac.webkit.org/changeset/107947>