[webkit-reviews] review denied: [Bug 45443] [GTK] Support for viewport meta tag : [Attachment 71716] Proposed patch8

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Oct 26 18:43:16 PDT 2010


Martin Robinson <mrobinson at webkit.org> has denied Joone Hur <joone at kldp.org>'s
request for review:
Bug 45443: [GTK] Support for viewport meta tag
https://bugs.webkit.org/show_bug.cgi?id=45443

Attachment 71716: Proposed patch8
https://bugs.webkit.org/attachment.cgi?id=71716&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=71716&action=review

> WebKit/gtk/webkit/webkitviewportattributes.cpp:63
> +    /* viewport properties */

I think the comments in this structure can be removed. They don't add much
extra information.

> WebKit/gtk/webkit/webkitviewportattributes.cpp:159
> +gfloat
webkit_viewport_attributes_get_initial_scale_factor(WebKitViewportAttributes
*viewportAttributes)

Asterisk should be next to the type name.

> WebKit/gtk/webkit/webkitviewportattributes.cpp:175
> +gfloat
webkit_viewport_attributes_get_minimum_scale_factor(WebKitViewportAttributes
*viewportAttributes)

Ditto.

> WebKit/gtk/webkit/webkitviewportattributes.cpp:191
> +gfloat
webkit_viewport_attributes_get_maximum_scale_factor(WebKitViewportAttributes
*viewportAttributes)

Ditto.

> WebKit/gtk/webkit/webkitviewportattributes.cpp:207
> +gfloat
webkit_viewport_attributes_get_device_pixel_ratio(WebKitViewportAttributes
*viewportAttributes)

Ditto.

> WebKit/gtk/webkit/webkitviewportattributes.cpp:223
> +gboolean
webkit_viewport_attributes_get_is_user_scalable(WebKitViewportAttributes
*viewportAttributes)

Asterisk should be next to the type.

> WebKit/gtk/webkit/webkitviewportattributes.cpp:239
> +gboolean webkit_viewport_attributes_get_is_valid(WebKitViewportAttributes
*viewportAttributes)

Ditto.

> WebKit/gtk/webkit/webkitviewportattributes.cpp:253
> +/**
> + * webkit_viewport_attributes_recompute:
> + * @viewportAttributes: a #WebKitViewportAttributes
> + *
> + * Recompute the optimal viewport attributes when the size of the visible
viewport is changed. 
> + *
> + * Since: 1.3.5
> + */
> +void webkit_viewport_attributes_recompute(WebKitViewportAttributes
*viewportAttributes)

Ditto. I'm not sure this is clear as to why you'd want to call
webkit_viewport_attributes_recompute. If the UA is supposed to call this when
the size of the device viewport has changed, I think the comment should say
that explicitly.

> WebKit/gtk/webkit/webkitviewportattributes.h:78
> +webkit_viewport_attributes_get_device_pixel_ratio  
(WebKitViewportAttributes *viewportAttributes);
> +
> +WEBKIT_API gboolean 
> +webkit_viewport_attributes_get_is_user_scalable    
(WebKitViewportAttributes *viewportAttributes);
> +
> +WEBKIT_API gboolean

Ditto.

> WebKit/gtk/webkit/webkitwebview.cpp:3158
> +    priv->viewportAttributes =
adoptPlatformRef(webkit_viewport_attributes_new(priv->corePage, 980,
gdk_screen_get_width(screen), gdk_screen_get_height(screen),
webViewGetDPI(webView)));

Why is this initialized to 980 here? There should at least be a comment
explaining it.

> WebKit/gtk/webkit/webkitwebview.cpp:4533
> +void webkit_web_view_init_viewport_attributes(WebKitWebView* webView, gint
desktopWidth, gint deviceWidth, gint deviceHeight, gint deviceDPI)

As I mentioned on IRC, it's my belief that we should wait for the
DumpRenderTreeSupport code to land and have these private methods there.

> WebKitTools/DumpRenderTree/gtk/LayoutTestControllerGtk.cpp:837
> +    webkit_web_view_init_viewport_attributes(webView, 980, 320, 480, 160);
> +    WebKitViewportAttributes* viewportAttributes =
webkit_web_view_get_viewport_attributes(webView);
> +    webkit_viewport_attributes_compute_internal(viewportAttributes,
availableWidth, availableHeight);
> +    fprintf(stdout, "viewport size %dx%d scale %f with limits [%f, %f]\n",

Shouldn't webkit_web_view_init_viewport_attributes call
webkit_viewport_attributes_compute_internal itself? Is there a situation where
you'd want to call webkit_web_view_init_viewport_attributes without immediately
calling webkit_viewport_attributes_compute_internal? Is it possible to move
webkit_viewport_attributes_compute_internal into the body of
webkit_viewport_attribute_recompute and have
webkit_web_view_init_viewport_attributes call
webkit_viewport_attribute_recompute?


More information about the webkit-reviews mailing list