[GTK] Add an API to add a custom tab into the print dialog

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Tomas Popela]

Extend the WebKitPrintOperation API to be able to add a custom tab into the print dialog.

[Tomas Popela]

Created attachment 267000 [details]
Proposed patch based on GtkPrintOperation implementation

[WebKit Commit Bot]

Attachment 267000 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:214:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:216:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:276:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:277:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:278:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:279:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:280:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:281:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:297:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:298:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:299:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:300:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:301:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:302:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.h"
Total errors found: 14 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

Comment on attachment 267000 [details]
Proposed patch based on GtkPrintOperation implementation

View in context: https://bugs.webkit.org/attachment.cgi?id=267000&action=review

Sorry for the delay and thanks for working on this; I'm eager to add the API you need for Evolution. r- not because I'm opposed to the changes, but because I'm not sure this is the best API and I have many suggestions.

Please do propose this API on the mailing list for discussion. I'm not sure it's the best we can do:

 * Signals are OK, but not the easiest to work with.
 * Your API can only handle a single custom tab, whereas it would be good to allow any number of tabs.
 * I'm not thrilled that the tab label is a property, whereas the custom widget is returned via a signal; both are slightly clunky, and it's awkward that they're set separately. (It seems like the custom widget could be a property as well, rather than a signal?)
 * Perhaps the custom label property should be a GtkWidget, rather than a char* that we use to create a GtkLabel. I guess 90% of applications will want to use a plain label, but I don't see any real reason to restrict them from doing otherwise.

Consider this alternative API:

webkit_print_operation_add_custom_print_dialog_tab(const char* tabLabel, GtkContainer* tabContent, GCallback applyCallback)

Wouldn't that be simpler? That's just one function: we wouldn't need the two signals or the clunky tab label property; it's easy for the application to add multiple tabs, rather than artificially limiting applications to one custom tab; and it'd probably be a bit simpler to implement. Do you see any problems with this approach?

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:152
> +static GtkWidget* webkitPrintOperationCreateCustomWidget(WebKitPrintOperation *printOperation)

WebKitPrintOperation* printOperation

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:213
> +    g_object_class_install_property(gObjectClass,

You really do have to obey style checker for new properties, so indent four spaces:

g_object_class_install_property(
    gObjectClass,
    PROP_CUSTOM_TAB_LABEL,
    g_param_spec_string(
        "custom-tab-label",
        _("Custom tab label"),
        _("Label for the tab containing custom widgets."),
        nullptr,
        WEBKIT_PARAM_READWRITE));

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:269
> +    * Returns: (transfer none): A custom widget that gets embedded in

I'm really not sure if this is the right transfer annotation? The widget is going to be embedded into the widget tree, so it would be wrong for the caller to free it after this. But I think it's not typical to mark GtkWidget parameters with transfer full? It's important to get this right for bindings.

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:339
> +                customTabLabel = _("Application");

I think this might be impossible to translate without context, so it would surely need a translator comment. But I would rather force the application to specify a label. If the property is unset, you could g_critical and return, or g_return_val_if_fail, rather than fallback to g_get_application_name, since I suspect that will rarely ever be the desired label, and "Application" will never be the desired label. :)

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:343
> +

Nit of all nits, but I would not leave a blank line here. You're creating the label for no reason other than to add it to printDialog; those are closely-related enough to be on consecutive lines. IMO. :)

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:562
> + * @label: (allow-none): the label to use, or %NULL to use the default label

If you follow my suggestion and get rid of the default label, this would no longer be (allow-none).

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.h:71
>      void (*_webkit_reserved1) (void);

Careful not to break ABI here. Since you're adding two virtual signals, you have to remove two of the padding pointers.

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.h:105
> +                                          (WebKitPrintOperation *printOperation,

WebKitPrintOperation *print_operation, star moved, and with an underscore. Since it's a public header, welcome back to GNOME style. :)

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.h:106
> +                                           const gchar* label);

const gchar *label

[Michael Catanzaro]

(In reply to comment #3) 
> You really do have to obey style checker for new properties, so indent four
> spaces:

And for the signals as well

> > Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.h:105
> > +                                          (WebKitPrintOperation *printOperation,
> 
> WebKitPrintOperation *print_operation, star moved, and with an underscore.
> Since it's a public header, welcome back to GNOME style. :)

I got a bit confused here; you do have the * in the right place on this line.

[Tomas Popela]

Created attachment 298345 [details]
Patch

[Michael Catanzaro]

Comment on attachment 298345 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=298345&action=review

This API is much nicer than your last patch. First r=me. (You need a second review for new API.)

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:88
> +    Vector<std::pair<GtkWidget*, GtkWidget*>> customTabs;

I think you should use Vector<std::pair<GRefPtr<GtkWidget>, GRefPtr<GtkWidget>>>. Partly because then you don't have to manually ref the widgets and can get rid of the loop in the destructor, mostly because it's good to use GRefPtr whenever possible in new code.

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:465
> + * @tab_content: a #GtKWidget to put in the custom tab
> + * @tab_label: a #GtKWidget to use as a tab label

Nit: I wonder if we should reverse the order of these parameters, since a GtkNotebook is effectively a map from tab labels to tab content, not vice-versa.

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:483
> +    g_object_ref(tabContent);
> +    g_object_ref(tabLabel);
> +
> +    printOperation->priv->customTabs.append(std::make_pair(tabContent, tabLabel));

Then you wouldn't need to ref the objects explicitly here, since creating a GRefPtr without using adoptGRef will ref the widget automatically.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestPrinting.cpp:379
> +        g_object_ref(event->key.window);

Where you you unref it?

[Tomas Popela]

(In reply to comment #6)
> > Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:465
> > + * @tab_content: a #GtKWidget to put in the custom tab
> > + * @tab_label: a #GtKWidget to use as a tab label
> 
> Nit: I wonder if we should reverse the order of these parameters, since a
> GtkNotebook is effectively a map from tab labels to tab content, not
> vice-versa.

I was thinking about doing the same as I ended up several times with exchanged content and tab. But I was trying to stick with the GtkPrintUnixDialog. Maybe we need another opinion (from the second reviewer)? :)
 
> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestPrinting.cpp:379
> > +        g_object_ref(event->key.window);
> 
> Where you you unref it?

Nowhere. I actually took the code from Tools/TestWebKitAPI/Tests/WebKit2/gtk
/InputMethodFilter.cpp, but we are doing the same in Evolution (not unrefing it). But I think you are right and we should unref it.

[Tomas Popela]

(In reply to comment #7)
> > > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestPrinting.cpp:379
> > > +        g_object_ref(event->key.window);
> > 
> > Where you you unref it?
> 
> Nowhere. I actually took the code from Tools/TestWebKitAPI/Tests/WebKit2/gtk
> /InputMethodFilter.cpp, but we are doing the same in Evolution (not unrefing
> it). But I think you are right and we should unref it.

It's unreferenced by gtk+ - https://git.gnome.org/browse/gtk+/tree/gdk/gdkevents.c?h=gtk-3-22#n843

[Tomas Popela]

Created attachment 298570 [details]
Patch

Update the code so the widgets are not destroyed together with the dialog.

[Carlos Garcia Campos]

(In reply to comment #3)
> Comment on attachment 267000 [details]
> Proposed patch based on GtkPrintOperation implementation
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267000&action=review
> 
> Sorry for the delay and thanks for working on this; I'm eager to add the API
> you need for Evolution. r- not because I'm opposed to the changes, but
> because I'm not sure this is the best API and I have many suggestions.

I'm sorry because I'm late too, and I agree more with the initial patch than the current one :-(

> Please do propose this API on the mailing list for discussion. I'm not sure
> it's the best we can do:
> 
>  * Signals are OK, but not the easiest to work with.

Signals are consistent with the GTK+ API, and even with the WebKit API, whenever a webview needs to create another view it sends the create signal. Whenever the print dialog needs to create a custom tab it can emit create signal, adn we don't need to mess up with the widgets references. I don't think we want to reuse custom widgets from multiple calls to print. The print dialog is created everytime and destroyed, the custom widget is part of it, so it should have the exactly same life cycle. There are things about the GTK+ API that I don't like either and we can improve that, but setting the custom tab like if it was a property of the print operation is not a good idea IMO.

>  * Your API can only handle a single custom tab, whereas it would be good to
> allow any number of tabs.

That's a limitation of GTK+.

>  * I'm not thrilled that the tab label is a property, whereas the custom
> widget is returned via a signal; both are slightly clunky, and it's awkward
> that they're set separately. (It seems like the custom widget could be a
> property as well, rather than a signal?)

I agree, this is one of the things I don't like either. I think it was done that way because the create signal returns the custom widget itself.

>  * Perhaps the custom label property should be a GtkWidget, rather than a
> char* that we use to create a GtkLabel. I guess 90% of applications will
> want to use a plain label, but I don't see any real reason to restrict them
> from doing otherwise.

I think here we should also be consistent with GTK+, our dialog should look like the print dialog of any other GTK+ application.

> Consider this alternative API:
> 
> webkit_print_operation_add_custom_print_dialog_tab(const char* tabLabel,
> GtkContainer* tabContent, GCallback applyCallback)
> 
> Wouldn't that be simpler? That's just one function: we wouldn't need the two
> signals or the clunky tab label property; it's easy for the application to
> add multiple tabs, rather than artificially limiting applications to one
> custom tab; and it'd probably be a bit simpler to implement. Do you see any
> problems with this approach?

It's not enough. The GtkPrintSettings API is extendable, so if you have custom settings you use the same GtkPrintSettings objet to handle them. The custom widget needs to read their settings form there, and when dialog is applied, update the settings again to make sure they are used when printing. That's why GTK+ has update-custom-widget (emitted when the selected printer changes to pass the GtkPageSetup and GtkSettings), and custom-widget-apply (emitted before the dialog is destroyed to allow the custom widget to update the settings or whatever before the actual print starts). We need both signals.

> > Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:152
> > +static GtkWidget* webkitPrintOperationCreateCustomWidget(WebKitPrintOperation *printOperation)
> 
> WebKitPrintOperation* printOperation
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:213
> > +    g_object_class_install_property(gObjectClass,
> 
> You really do have to obey style checker for new properties, so indent four
> spaces:
> 
> g_object_class_install_property(
>     gObjectClass,
>     PROP_CUSTOM_TAB_LABEL,
>     g_param_spec_string(
>         "custom-tab-label",
>         _("Custom tab label"),
>         _("Label for the tab containing custom widgets."),
>         nullptr,
>         WEBKIT_PARAM_READWRITE));
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:269
> > +    * Returns: (transfer none): A custom widget that gets embedded in
> 
> I'm really not sure if this is the right transfer annotation? The widget is
> going to be embedded into the widget tree, so it would be wrong for the
> caller to free it after this. But I think it's not typical to mark GtkWidget
> parameters with transfer full? It's important to get this right for bindings.

I guess this is copied from GTK+, the doc is probably wrong, because the dialog becomes the owner of the custom widget in GTK+ so it's indeed a transfer full.

> > Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:339
> > +                customTabLabel = _("Application");
> 
> I think this might be impossible to translate without context, so it would
> surely need a translator comment. But I would rather force the application
> to specify a label. If the property is unset, you could g_critical and
> return, or g_return_val_if_fail, rather than fallback to
> g_get_application_name, since I suspect that will rarely ever be the desired
> label, and "Application" will never be the desired label. :)

Again this is what GTK+ does.

> > Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:343
> > +
> 
> Nit of all nits, but I would not leave a blank line here. You're creating
> the label for no reason other than to add it to printDialog; those are
> closely-related enough to be on consecutive lines. IMO. :)
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:562
> > + * @label: (allow-none): the label to use, or %NULL to use the default label
> 
> If you follow my suggestion and get rid of the default label, this would no
> longer be (allow-none).
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.h:71
> >      void (*_webkit_reserved1) (void);
> 
> Careful not to break ABI here. Since you're adding two virtual signals, you
> have to remove two of the padding pointers.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.h:105
> > +                                          (WebKitPrintOperation *printOperation,
> 
> WebKitPrintOperation *print_operation, star moved, and with an underscore.
> Since it's a public header, welcome back to GNOME style. :)
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.h:106
> > +                                           const gchar* label);
> 
> const gchar *label

So, my proposal is to add an API similar to the GTK+ one for consistency, but also improving the things we don't like. For example, I think we could use a new object to create the custom tab, something like WebKitPrintCustomWidget. I would avoid using Tab, because I'm not sure the custom widget is also a tab in windows print dialog, for example, just in case some day we have windows support. Then WebKitPrintOperation has a create signal "create-custom-widget", emitted when constructing the print dialog, like gtk does. It expects a WebKitPrintCustomWidget object, so the user can create it with something like:

WebKitPrintCustomWidget *webkit_print_custom_widget_new(GtkWidget *widget, const char *label);

We can discuss if allow NULL label or not, I think we should for consistency with GTK+.

The object is transferred, so the user doesn't need to care about the lifeftime. Then instead of adding update and apply signals to the print operation we can add them to the WebKitPrintCustomWidget object. So, it's similar to the WebKitWebView::create signal, you create your object, connect to the update and apply signals and returns it in the callback.

What do you think?

[Tomas Popela]

Created attachment 299240 [details]
Patch

I reworked the patch in a way that Carlos suggested. I made the label mandatory as I really don't like the logic about determining its value when it's not defined. This is definitely not a patch for review, but I would like to get some feedback. I also left the MiniBrowser changes there to show how the API will be used (I will create the unit test once I will be sure, that we don't change the API ;))

[Carlos Garcia Campos]

Comment on attachment 299240 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299240&action=review

LGTM

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:25
> +#include "WebKitPrintCustomWidgetPrivate.h"
> +#include "WebKitPrivate.h"

WebKitPrintCustomWidgetPrivate.h should include WebKitPrivate.h already.

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:45
> +    CUSTOM_WIDGET_APPLY,
> +    UPDATE_CUSTOM_WIDGET,

This is something I don't like in GTK+ api, either use CUSTOM_WIDGET_FOO or FOO_CUSTOM_WIDGET, but the same in both cases. Actually, since the object is called WebKitPrintCustomWidget, the custom-widegt prefix/suffix in the signal name is redundant. We can simply use update and apply.

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:58
> +    CString customWidgetLabel;

The widget is called customWidget, so we can simply use label here, we already know it's the label of the custom widget. Instead of label, I would call it title, though.

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:59
> +    GRefPtr<GtkWidget> customWidget;

I'm not sure we want to use smart pointers here, we should just pass the ownership to the dialog. Same here about the name, use just widget.

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:168
> +            webkit_marshal_VOID__OBJECT_OBJECT_OBJECT,

Can't we use a generic marshal here?

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:173
> +WebKitPrintCustomWidget* webkit_print_custom_widget_new(GtkWidget *widget, const char *label)

We should document that we take the ownership of the widget, like when passed to a container, the user doesn't need to unref/destroy it. And that it will be destroyed when the print dialog is destroyed even if this object is still alive. The apply signal is used to notify the user that the widget will be destroyed.

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:176
> +    g_return_val_if_fail(GTK_IS_WIDGET(widget), NULL);
> +    g_return_val_if_fail(label, NULL);

nullptr.

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:269
> +    WebKitPrintCustomWidget* printCustomWidget = nullptr;
> +    g_signal_emit(printOperation, signals[CREATE_CUSTOM_WIDGET], 0, &printCustomWidget);

I think we should adopt the ref and document this is transfer full.

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:274
> +        g_object_get(printCustomWidget, "custom-widget", &customWidget, "custom-widget-label", &customWidgetLabel, nullptr);

Do not use g_object_get here, either add public getters or private.

[Tomas Popela]

(In reply to comment #12)
> > Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:58
> > +    CString customWidgetLabel;
> 
> The widget is called customWidget, so we can simply use label here, we
> already know it's the label of the custom widget. Instead of label, I would
> call it title, though.

OK, while doing so I will also change the properties names from CUSTOM_WIDGET to just WIDGET and CUSTOM_WIDGET_LABEL to TITLE.

> > Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:168
> > +            webkit_marshal_VOID__OBJECT_OBJECT_OBJECT,
> 
> Can't we use a generic marshal here?

We could, but I don't like that the accumulator callback will be duplicated in WebKitWebView.cpp, WebKitPrintOperation.cpp and WebKitPrintCustomWidget.cpp. Maybe we could have only a private one in WebKitWebView.cpp, so we could remove this duplication. OK?

> > Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:274
> > +        g_object_get(printCustomWidget, "custom-widget", &customWidget, "custom-widget-label", &customWidgetLabel, nullptr);
> 
> Do not use g_object_get here, either add public getters or private.

OK I will add public getters there.

[Tomas Popela]

Created attachment 299598 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 299598 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299598&action=review

API looks good to me, the unit test is quite difficult to follow, I think we are doing too much in the test class . . .

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:37
> + * A #WebKitPrintCustomWidget allows to embed a custom widget in the print

Do not use # here since it creates a link to here.

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:39
> + * signal, creating a new #WebKitPrintCustomWidget object with

Ditto. Also, I would omit object.

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:62
> +    GtkWidget* widget;

I think we should use a smart pointer here. We are supposed to transfer the ownership to the dialog, but if the dialog is never shown we would be leaking the widget. So, I think we should take a ref and unref it after apply.

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:111
> +     */

Since: 2.16

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:118
> +            _("widget that will be added to the print dialog."),

Widget

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:126
> +     */

Since: 2.16

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:132
> +            _("Label"),

Title

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:133
> +            _("Label of the widget that will be added to the print dialog."),

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:140
> +     * @widget: the custom #GtkWidget that's embedded in the dialog

Why do we pass the widget? We have a getter, the user can simpler do get_widget in the callback, no?

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:145
> +     */

Since: 2.16

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:146
> +    signals[APPLY] =

It doesn't really matter but it's weird that apply is here before update.

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:160
> +     * @widget: the custom #GtkWidget that's embedded in the dialog

Why do we pass the widget? We have a getter, the user can simpler do get_widget in the callback, no?

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:167
> +     */

Since: 2.16

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:186
> + * ownership is taken and it is destroyed together with the dialog even this

even if?

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:191
> + */

Since: 2.16

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:199
> +    WebKitPrintCustomWidget* printCustomWidget = WEBKIT_PRINT_CUSTOM_WIDGET(g_object_new(WEBKIT_TYPE_PRINT_CUSTOM_WIDGET, "widget", widget, "title", title, nullptr));
> +
> +    return printCustomWidget;

No need for the local variable:

return WEBKIT_PRINT_CUSTOM_WIDGET(g_object_new(WEBKIT_TYPE_PRINT_CUSTOM_WIDGET, "widget", widget, "title", title, nullptr));

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:207
> + * Return the value of #WebKitPrintCustomWidget:widget property for the given
> + * @print_custom_widget object.

We should document that it could return %NULL if called after apply, and that is never null when called from update or apply callbacks.

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:210
> + */

Since: 2.16

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:226
> + */

Since: 2.16

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:236
> +    g_signal_emit(printCustomWidget, signals[APPLY], 0, printCustomWidget->priv->widget);

I don't think we need to pass the widget. After apply we should null our pointer or use a weak pointer to make it null when the dialog destroys it.

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:241
> +    g_signal_emit(printCustomWidget, signals[UPDATE], 0, printCustomWidget->priv->widget, pageSetup, printSettings);

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.h:53
> +    void    (*apply)               (WebKitPrintCustomWidget *print_custom_widget,

(* apply)

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.h:54
> +                                    GtkWidget *widget);

This should be lined up

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.h:55
> +    void    (*update)              (WebKitPrintCustomWidget *print_custom_widget,

(* update)

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.h:58
> +                                    GtkWidget *widget,
> +                                    GtkPageSetup *page_setup,
> +                                    GtkPrintSettings *print_settings);

And all these lined up

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.h:63
> +    void    (*_webkit_reserved0)   (void);
> +    void    (*_webkit_reserved1)   (void);
> +    void    (*_webkit_reserved2)   (void);
> +    void    (*_webkit_reserved3)   (void);

Remove extra spaces between ) (

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.h:69
> +WebKitPrintCustomWidget *

WEBKIT_API

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.h:70
> +webkit_print_custom_widget_new                  (GtkWidget *widget, const char *title);

Use new line per parameter and line up the names

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.h:72
> +GtkWidget *

WEBKIT_API

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.h:75
> +const gchar *

WEBKIT_API

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:82
> +    GRefPtr<WebKitPrintCustomWidget> customWidget;

Why do we need to keep this here? The dialog is always run modal, so we could make this local to webkitPrintOperationRunDialog

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:249
> +static void notifySelectedPrinterCallback(GtkPrintUnixDialog* dialog, GParamSpec *, WebKitPrintCustomWidget *printCustomWidget)

GParamSpec*, WebKitPrintCustomWidget*

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:278
> +    WebKitPrintCustomWidget* printCustomWidget = nullptr;

GRefPtr<WebKitPrintCustomWidget> and then we don't need to keep it in the private struct.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestPrinting.cpp:376
> +        test->sendKeyEvent(GDK_KEY_Right, GDK_KEY_PRESS);
> +        test->sendKeyEvent(GDK_KEY_Right, GDK_KEY_RELEASE);
> +
> +        test->sendKeyEvent(GDK_KEY_Right, GDK_KEY_PRESS);
> +        test->sendKeyEvent(GDK_KEY_Right, GDK_KEY_RELEASE);

What are we doing here? what should we expect after this?

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestPrinting.cpp:394
> +        g_timeout_add_seconds(2, (GSourceFunc)jumpToCustomWidget, test);

So, on realize we jump to first printer, which would emit update. Why don't we just to custom widget on update then instead of waiting for 2 seconds?

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestPrinting.cpp:406
> +        : m_webViewClosed(false)
> +        , m_updateCalled(false)
> +        , m_createCalled(false)
> +        , m_applyCalled(false)
> +        , m_widgetRealized(false)
> +        , m_testWindow(gtk_window_new(GTK_WINDOW_TOPLEVEL))
> +        , m_printFinished(false)

Do the initializations in member declarations.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestPrinting.cpp:504
> +    test->loadHtml("<html><body onLoad=\"w = window.open();w.print();w.close();\"></body></html>", 0);

I think this will be easier to test using webkit_print_operation_run_dialog instead of creating the dialog from js.

[Build Bot]

Comment on attachment 299598 [details]
Patch

Attachment 299598 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2941201

New failing tests:
media/modern-media-controls/tracks-support/tracks-support-show-panel-fullscreen.html

[Build Bot]

Created attachment 299602 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Tomas Popela]

Created attachment 299809 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 299809 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299809&action=review

I'm still not happy with the unit test, but I don't want to block this. Thanks

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:24
> +#include "WebKitWebViewPrivate.h"

Why do you need WebKitWebViewPrivate.h here?

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:91
> +        printCustomWidget->priv->widget = adoptGRef(GTK_WIDGET(g_object_ref(g_value_get_object(value))));

adopting the ref that we manually take is the same as simply assigning the pointer returned by g_value_get_object. It's even better because GRefPtr will wink the reference if it's floating.

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:212
> + * @print_custom_widget object. The returned value will never be valid if called

will never be valid? I guess will always be valid, or will never be invalid, no?

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:214
> + * callbacks, but it could be %NULL if called after the

I would say it will be %NULL, we should ensure that

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:221
> +GtkWidget* webkit_print_custom_widget_get_widget(WebKitPrintCustomWidget *printCustomWidget)

WebKitPrintCustomWidget *printCustomWidget -> WebKitPrintCustomWidget* printCustomWidget

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:235
> + * Returns: Title of the #GtkWidget associated with @print_custom_widget.

I find confusing mentioning the GtkWidget here, I would just say this is the title of the @print_custom_widget.

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:239
> +const gchar* webkit_print_custom_widget_get_title(WebKitPrintCustomWidget *printCustomWidget)

WebKitPrintCustomWidget *printCustomWidget -> WebKitPrintCustomWidget* printCustomWidget

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:246
> +void webkitPrintCustomWidgetEmitCustomWidgetApplySignal(WebKitPrintCustomWidget *printCustomWidget)

WebKitPrintCustomWidget *printCustomWidget -> WebKitPrintCustomWidget* printCustomWidget

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.cpp:249
> +    printCustomWidget->priv->widget.clear();

printCustomWidget->priv->widget = nullptr;

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintCustomWidget.h:77
> +WEBKIT_API GType
> +webkit_print_custom_widget_get_type             (void);
> +
> +WEBKIT_API WebKitPrintCustomWidget *
> +webkit_print_custom_widget_new                  (GtkWidget  *widget,
> +                                                 const char *title);
> +
> +WEBKIT_API GtkWidget *
> +webkit_print_custom_widget_get_widget           (WebKitPrintCustomWidget *print_custom_widget);
> +
> +WEBKIT_API const gchar *
> +webkit_print_custom_widget_get_title            (WebKitPrintCustomWidget *print_custom_widget);

You should remove all the extra spaces between function name and parameters and line them up to the longest one. Also parameters of all functions are usually lined up.

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:27
> +#include "WebKitWebViewPrivate.h"

And here?

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:232
> +     * Returns: (transfer full): A #WebKitPrintCustomWidget that will be embedded
> +     * in the dialog.

This could be one line.

> Tools/ChangeLog:19
> +        Add a new create-custom-widget signal to the WebKitPrintOperation. The
> +        signal is emitted before the dialog is displayed and it gives an
> +        opportunity to embed a custom widget in the dialog. You can do so by
> +        creating a new WebKitPrintCustomWidget and returning it from the
> +        create-custom-widget signal handler. The WebKitPrintCustomWidget is
> +        emitting two signals:
> +          - update - emitted when the currently selected printer is changed,
> +                     to be able to actualize the custom widget based on the
> +                     current printer
> +          - apply - emitted when the dialog is closed, just before the
> +                    printing will start, to be able e.g. to change content
> +                    based on the custom widget state.

No need to duplicate all this in the Tools ChangeLog, simply say that you are adding a new test for the new api.

> Tools/ChangeLog:25
> +        Add a new WebKitPrintOperation/custom-widget test in TestPrinting
> +        that is testing a newly added API.

So, move this above.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestPrinting.cpp:363
> +        g_file_delete(m_outputFile.get(), 0, 0);

0 -> nullptr

[Tomas Popela]

Committed r211481: <http://trac.webkit.org/changeset/211481>