Bug 48357 - [GTK] Add the GtkScrollablePolicy property to the webview
Summary: [GTK] Add the GtkScrollablePolicy property to the webview
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-26 11:56 PDT by Alejandro G. Castro
Modified: 2010-10-29 04:26 PDT (History)
4 users (show)

See Also:

Attachments
Proposed patch (4.50 KB, patch)
2010-10-26 12:16 PDT, Alejandro G. Castro 		mrobinson: review-
Details | Formatted Diff | Diff
Proposed patch (4.52 KB, patch)
2010-10-27 04:40 PDT, Alejandro G. Castro 		mrobinson: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Alejandro G. Castro 2010-10-26 11:56:13 PDT 
We have to add the GtkScrollablePolicy properties to the webView, it is now required for gtk3 gtkscrollable interfaces.
Comment 1 Martin Robinson 2010-10-26 11:59:01 PDT 
Is there more necessary than was added by this change? http://trac.webkit.org/changeset/70514
Comment 2 Alejandro G. Castro 2010-10-26 12:14:43 PDT 
(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.
Comment 3 Alejandro G. Castro 2010-10-26 12:16:39 PDT 
Created attachment 71925 [details]
Proposed patch
Comment 4 Martin Robinson 2010-10-26 12:22:03 PDT 
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?
Comment 5 Alejandro G. Castro 2010-10-27 04:03:06 PDT 
(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.
Comment 6 Alejandro G. Castro 2010-10-27 04:40:58 PDT 
Created attachment 72004 [details]
Proposed patch
Comment 7 Martin Robinson 2010-10-27 09:52:10 PDT 
Comment on attachment 72004 [details]
Proposed patch

Thanks!
Comment 8 WebKit Review Bot 2010-10-28 01:55:37 PDT 
Attachment 72004 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4852063
Comment 9 Alejandro G. Castro 2010-10-29 03:26:14 PDT 
(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.
Comment 10 Alejandro G. Castro 2010-10-29 04:26:59 PDT 
Landed http://trac.webkit.org/changeset/70861