SSIA.
Created attachment 71672 [details] scrollable.diff This seems to be enough to get things going...
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.
Created attachment 71673 [details] scrollable.diff Yikes.
Created attachment 71674 [details] scrollable.diff The code in get/set needs to be made conditional too.
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.
(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.
Created attachment 71842 [details] scrollable.diff Address review comments.
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 on attachment 71842 [details] scrollable.diff Landed as r70514.