Bug 76448 – [GTK] Add WebKitPrintOperation to WebKit2 GTK+ API

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

Initial printing API.

[Carlos Garcia Campos]

Created attachment 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.

[WebKit Review Bot]

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

[Gustavo Noronha (kov)]

Comment on attachment 122759 [details]
Patch

Attachment 122759 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11268265

[Carlos Garcia Campos]

(In reply to comment #3)
> (From update of attachment 122759 [details])
> Attachment 122759 [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.

[Gustavo Noronha (kov)]

Comment on attachment 122759 [details]
Patch

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?

[Carlos Garcia Campos]

(In reply to comment #5)
> (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?

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

[Carlos Garcia Campos]

Created attachment 126940 [details]
Patch updated to apply on current git master

Rebased and fixed review comments.

[WebKit Review Bot]

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.

[Philippe Normand]

Comment on attachment 126940 [details]
Patch updated to apply on current git master

Attachment 126940 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11516185

[Carlos Garcia Campos]

Created attachment 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

[WebKit Review Bot]

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.

[Martin Robinson]

Comment on attachment 126944 [details]
Fix gtk-doc warnings

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"

[Carlos Garcia Campos]

(In reply to comment #12)
> (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.

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!

[Carlos Garcia Campos]

Committed r107947: <http://trac.webkit.org/changeset/107947>