Bug 48357 – [GTK] Add the GtkScrollablePolicy property to the webview

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

[Alejandro G. Castro]

We have to add the GtkScrollablePolicy properties to the webView, it is now required for gtk3 gtkscrollable interfaces.

[Martin Robinson]

Is there more necessary than was added by this change? http://trac.webkit.org/changeset/70514

[Alejandro G. Castro]

(In reply to comment #1)
> Is there more necessary than was added by this change? http://trac.webkit.org/changeset/70514

Yeah, apparently they added this new property to control when to start the scrolling.

[Alejandro G. Castro]

Created attachment 71925 [details]
Proposed patch

[Martin Robinson]

Comment on attachment 71925 [details]
Proposed patch

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

Looks good, but I have a couple concerns.

> WebKit/gtk/webkit/webkitprivate.h:161
> +        guint hscroll_policy : 1;
> +        guint vscroll_policy : 1;
> +

I think I'd prefer these to be named horizontalScrollingPolicy and verticalScrollingPolicy. Why the use of bitfields here? I'm a little worried about such a rarely used feature causing issues with other compilers. Is it possible to make these of type GtkScrollablePolicy instead of guint?

[Alejandro G. Castro]

(In reply to comment #4)
> (From update of attachment 71925 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71925&action=review
> 
> Looks good, but I have a couple concerns.
> 
> > WebKit/gtk/webkit/webkitprivate.h:161
> > +        guint hscroll_policy : 1;
> > +        guint vscroll_policy : 1;
> > +
> 
> I think I'd prefer these to be named horizontalScrollingPolicy and verticalScrollingPolicy.

Changed.

> Why the use of bitfields here? I'm a little worried about such a rarely used feature causing issues with other compilers. Is it possible to make these of type GtkScrollablePolicy instead of guint?

It is the usual way in gtk, it is basically how it is defined in all the other gtk widgest in the library so I assume it is safe enough for us. But yeah, I guess it is just about making smaller structs. I'm ok using the enum because we are not using the bitwise any other place.

[Alejandro G. Castro]

Created attachment 72004 [details]
Proposed patch

[Martin Robinson]

Comment on attachment 72004 [details]
Proposed patch

Thanks!

[WebKit Review Bot]

Attachment 72004 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4852063

[Alejandro G. Castro]

(In reply to comment #8)
> Attachment 72004 [details] did not build on gtk:
> Build output: http://queues.webkit.org/results/4852063

I have to add guards to the .h for gtk2, I'll add them and upload.

[Alejandro G. Castro]

Landed http://trac.webkit.org/changeset/70861