See the patch.
Created attachment 379996 [details] Patch The upload and style check scripts don't seem to work right now, so created a patch with `git format-patch` and without the check. It's mostly ready, though I want to replace the current shadow gradient in css with something less complicated first.
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Created attachment 379997 [details] Patch
Created attachment 380000 [details] Patch Ok, now with the changed gradient.
Created attachment 380001 [details] Screenshot
Created attachment 380003 [details] Screenshot Err, actual screenshot
Comment on attachment 380000 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380000&action=review Thanks for the patch, I have a few comments but looks good. > Source/WebKit/PlatformGTK.cmake:520 > + " <file alias=\"css/gtk.css\">gtk.css</file>\n" I would make it explicit that this css has nothing to do with UA css, it's the theme css. Maybe: <file alias=\"theme/gtk-theme.css\">gtk-theme.css</file> What do you think? > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1448 > + gtk_widget_class_set_css_name(widgetClass, "webkitwebview"); This is only available since GTK 3.20 we need to add versions checks here. > Source/WebKit/UIProcess/ViewGestureController.h:169 > + GtkStyleContext* createStyleContext(const char*); Why is this public? > Source/WebKit/UIProcess/ViewGestureController.h:421 > + GRefPtr<GtkCssProvider> m_cssProvider; You need to include GRefPtr.h. It probably doesn't fail due to unified builds. We should also remove the gtk include from the header and forward delcare the gtk types > Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:280 > +GtkStyleContext* ViewGestureController::createStyleContext(const char* name) This could return a GRefPtr<GtkStyleContext> instead. > Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:286 > + // FIXME: is there a GRefPtr-like helper here? GtkWidgetPath is not a GObject > + GtkWidgetPath* path = gtk_widget_path_copy(gtk_widget_get_path(widget)); You can use GRefPtr, but include WebCore/GRefPtrGtk.h > Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:288 > + int pos = gtk_widget_path_append_type(path, GTK_TYPE_WIDGET); pos -> position > Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:302 > +static RefPtr<cairo_pattern_t> createElementPattern(GRefPtr<GtkStyleContext> context, double w, double h) I think it's better to pass the raw pointer here. Use width and height instead of w and h > Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:304 > + RefPtr<cairo_surface_t> surface = adoptRef(cairo_image_surface_create(CAIRO_FORMAT_ARGB32, w, h)); cairo expects integers, I wonder why we receive doubles that has been converted from integers > Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:308 > + gtk_render_background(context.get(), cr.get(), 0, 0, w, h); > + gtk_render_frame(context.get(), cr.get(), 0, 0, w, h); I think it's fine to pass integers here even when it expects doubles. > Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:313 > +static double getElementWidth(GRefPtr<GtkStyleContext> context) Same here, either pass the raw pointer or a ref instead of copying the GRefPtr. getElementWidth() -> elementWidth() > Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:316 > + gtk_style_context_get(context.get(), gtk_style_context_get_state(context.get()), "min-width", &width, NULL); NULL -> nullptr > Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:318 > + return (double) width; Use C++ style casts > Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:358 > + double width = m_webPageProxy.drawingArea()->size().width(); > + double height = m_webPageProxy.drawingArea()->size().height(); DrawingAreaProxy::size() returns an IntSize, why do we need to implicit convert it to double? Instead of getting the size twice, we could get it once, and use .width() and .height() when needed. > Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:419 > + shadowOpacity = (remainingSwipeDistance / m_swipeShadowSize); Parentheses are not needed here
Created attachment 380009 [details] Patch
(In reply to Carlos Garcia Campos from comment #7) > What do you think? Sure. > Why is this public? No reason, that was a mistake :) > DrawingAreaProxy::size() returns an IntSize, why do we need to implicit > convert it to double? Instead of getting the size twice, we could get it > once, and use .width() and .height() when needed. Because caching was a late change and it called gtk_render_background() etc directly in draw(). Doesn't make sense anymore indeed
Comment on attachment 380009 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380009&action=review LGTM, why didn't you ask for review this time? > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1450 > +#if GTK_CHECK_VERSION(3, 20, 0) > + gtk_widget_class_set_css_name(widgetClass, "webkitwebview"); > +#endif What happens when building with previous gtk? > Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:31 > +#include "WebCore/GRefPtrGtk.h" Use <WebCore/GRefPtrGtk.h> > Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:301 > +static cairo_pattern_t* createElementPattern(GRefPtr<GtkStyleContext> context, int width, int height) Sorry, I meant using the raw pointer for the context parameter instead of copying the GRefPtr, the smart pointer was ok for the return value.
> What happens when building with previous gtk? A lot of things would break. However... Isn't 3.22 the minimum, according to bug 199094? In which case the 3.20 guards arent' even needed. > LGTM, why didn't you ask for review this time? Ugh, I continue to forget to do it. Usually the upload script does it for me.
Created attachment 380031 [details] Patch
Created attachment 380036 [details] Patch Oops, fixed style
(In reply to Alexander Mikhaylenko from comment #11) > > What happens when building with previous gtk? > > A lot of things would break. However... Isn't 3.22 the minimum, according to > bug 199094? In which case the 3.20 guards arent' even needed. Right! I forgot we had bumped it. > > LGTM, why didn't you ask for review this time? > > Ugh, I continue to forget to do it. Usually the upload script does it for me.
Comment on attachment 380036 [details] Patch Clearing flags on attachment: 380036 Committed r250648: <https://trac.webkit.org/changeset/250648>
All reviewed patches have been landed. Closing bug.