Bug 48202 – [GTK] Port to new GtkScrollable interface in GTK+ 3.x

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

[Xan Lopez]

SSIA.

[Xan Lopez]

Created attachment 71672 [details]
scrollable.diff

This seems to be enough to get things going...

[WebKit Review Bot]

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.

[Xan Lopez]

Created attachment 71673 [details]
scrollable.diff

Yikes.

[Xan Lopez]

Created attachment 71674 [details]
scrollable.diff

The code in get/set needs to be made conditional too.

[Martin Robinson]

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.

[Xan Lopez]

(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.

[Xan Lopez]

Created attachment 71842 [details]
scrollable.diff

Address review comments.

[Martin Robinson]

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.

[Xan Lopez]

Comment on attachment 71842 [details]
scrollable.diff

Landed as r70514.