[webkit-reviews] review denied: [Bug 188746] [GTK] Touchscreen pinch to zoom should scale the page like other platforms : [Attachment 347807] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Justin Michaud
<justin at justinmichaud.com>'s request for review:
Bug 188746: [GTK] Touchscreen pinch to zoom should scale the page like other
platforms
https://bugs.webkit.org/show_bug.cgi?id=188746

Attachment 347807: Patch

https://bugs.webkit.org/attachment.cgi?id=347807&action=review




--- Comment #38 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 347807
  --> https://bugs.webkit.org/attachment.cgi?id=347807
Patch

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

I would also avoid to add new API. Also note that all new API must include a
unit test. If apps only need to reset the scale, maybe it's better to only
expose webkit_web_view_reset_scale(). If we really want to allow users to scale
the page, we should provide a test case to ensure it's really possible with the
proposed API. The ephy example always passing 1.0 and 0,0 as center is not
enough. Note also that without new API we could merge this to 2.22 branch.

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3158
> + * @scale_level: the page scale

I would use scale_factor

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3166
> + * example, a pinch-to-zoom touch gesture). The point (origin_x, origin_y)
> + * should be in page coordinates, scaled by the new scale factor.

I don't think we should receive page coordinates here. This function could be
used to implement gestures differently, overriding the default WebKit behavior.
In that case the user will be handling widget coords.

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3168
> + * Since 2.24

Since:

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3170
> +void webkit_web_view_set_page_scale(WebKitWebView* webView, gdouble
scaleLevel, gint originX, gint originY)

set_page_scale sounds like you are setting a property, but page scale isn't and
the setter receives more parameters. I think this should be
webkit_web_view_scale().

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3174
> +    auto& page = getPage(webView);

Here we would need to convert to page coordinates.

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3179
> + * webkit_web_view_get_zoom_level:

get_page_scale

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3182
> + * Get the zoom level of @web_view, i.e. the factor by which the

zoom level -> page scale

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3187
> + * Since 2.24

Since:

> Source/WebKit/UIProcess/API/gtk/WebKitWebView.h:415
> +webkit_web_view_get_page_scale			(WebKitWebView	       
   *web_view);

wpe header would need to be updated too.