[Webkit-unassigned] [Bug 48202] [GTK] Port to new GtkScrollable interface in GTK+ 3.x

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=48202


Xan Lopez <xan.lopez at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |xan.lopez at gmail.com




--- Comment #6 from Xan Lopez <xan.lopez at gmail.com>  2010-10-25 23:27:50 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.