Bug 48202

Summary: [GTK] Port to new GtkScrollable interface in GTK+ 3.x
Product: WebKit Reporter: Xan Lopez <xan.lopez>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
scrollable.diff
none
scrollable.diff
none
scrollable.diff
mrobinson: review-
scrollable.diff none

Description Xan Lopez 2010-10-24 01:15:35 PDT
SSIA.
Comment 1 Xan Lopez 2010-10-24 01:18:59 PDT
Created attachment 71672 [details]
scrollable.diff

This seems to be enough to get things going...
Comment 2 WebKit Review Bot 2010-10-24 01:21:36 PDT
Attachment 71672 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/gtk/webkit/webkitwebview.cpp:414:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/gtk/webkit/webkitwebview.cpp:417:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/gtk/webkit/webkitwebview.cpp:418:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/gtk/webkit/webkitwebview.cpp:419:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/gtk/webkit/webkitwebview.cpp:421:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Xan Lopez 2010-10-24 01:27:08 PDT
Created attachment 71673 [details]
scrollable.diff

Yikes.
Comment 4 Xan Lopez 2010-10-24 01:29:21 PDT
Created attachment 71674 [details]
scrollable.diff

The code in get/set needs to be made conditional too.
Comment 5 Martin Robinson 2010-10-24 22:17:27 PDT
Comment on attachment 71674 [details]
scrollable.diff

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

Looks good. I have a couple nits.

> WebKit/gtk/webkit/webkitwebview.cpp:202
> +#else
> +    PROP_VIEW_MODE,

I believe it's fine to have the extra comma on PROP_VIEW_MODE for the GTK+ 2.0 version. That would simplify the enum and #ifdef.

> WebKit/gtk/webkit/webkitwebview.cpp:214
> +                        G_IMPLEMENT_INTERFACE(GTK_TYPE_SCROLLABLE, NULL))

Should this NULL be a 0?

> WebKit/gtk/webkit/webkitwebview.cpp:400
> +static void webkit_web_view_set_hadjustment(WebKitWebView* webView, GtkAdjustment* hadj)

IMO if these setters aren't public they should be named in WebKit style. I can see naming them this way for consistency though. I definitely think 'hadj' should be 'horizontalAdjustment' though. These comments apply for webkit_web_view_set_vadjustment as well.
Comment 6 Xan Lopez 2010-10-25 23:27:50 PDT
(In reply to comment #5)
> (From update of attachment 71674 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71674&action=review
> 
> Looks good. I have a couple nits.
> 
> > WebKit/gtk/webkit/webkitwebview.cpp:202
> > +#else
> > +    PROP_VIEW_MODE,
> 
> I believe it's fine to have the extra comma on PROP_VIEW_MODE for the GTK+ 2.0 version. That would simplify the enum and #ifdef.

I believe the trailing comma was forbidden in the old standards of C and C++. For sure it's been traditionally not allowed in GNOME code for this reason. Not sure where we stand on this in WebKit, though.

> 
> > WebKit/gtk/webkit/webkitwebview.cpp:214
> > +                        G_IMPLEMENT_INTERFACE(GTK_TYPE_SCROLLABLE, NULL))
> 
> Should this NULL be a 0?

Yeah, I used NULL because I wasn't sure... reading the macro I think it's OK to use 0, so I'll change it.

> 
> > WebKit/gtk/webkit/webkitwebview.cpp:400
> > +static void webkit_web_view_set_hadjustment(WebKitWebView* webView, GtkAdjustment* hadj)
> 
> IMO if these setters aren't public they should be named in WebKit style. I can see naming them this way for consistency though. I definitely think 'hadj' should be 'horizontalAdjustment' though. These comments apply for webkit_web_view_set_vadjustment as well.

I used hadjustment/vadjustment because that's how the property is named (in the parent class, so it's not up to us to change the name). If the methods remain private we can do whatever we want though, so I'll change it.
Comment 7 Xan Lopez 2010-10-26 00:05:49 PDT
Created attachment 71842 [details]
scrollable.diff

Address review comments.
Comment 8 Martin Robinson 2010-10-26 01:24:09 PDT
Comment on attachment 71842 [details]
scrollable.diff

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

Okay. Patch looks great to me, but perhaps you might consider my suggestions before landing. Thanks!

> WebKit/gtk/webkit/webkitwebview.cpp:203
> +#ifndef GTK_API_VERSION_2
> +    , PROP_HADJUSTMENT
> +    , PROP_VADJUSTMENT
> +#endif

I guess I prefer the original method to this one. :/

> WebKit/gtk/webkit/webkitwebview.cpp:398
> +static void setHorizontalAdjustment(WebKitWebView* webView, GtkAdjustment* hadj)

My preference is to rename hadj/vadj to horizontalAdjustment/verticalAdjustment before landing.
Comment 9 Xan Lopez 2010-10-26 01:40:36 PDT
Comment on attachment 71842 [details]
scrollable.diff

Landed as r70514.