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

[bugzilla-daemon at webkit.org]

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 67011: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=67011&action=review

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

The patch looks good overall. I just have a few minor style nits. I'd really
like to see an expanded ChangeLog entry. Can you explain this feature in more
depth and what it is useful for?

> WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:616
> +    WebKitViewport* viewport = webkit_viewport_new(arguments);
It's probably best to use PlatformRefPtr here.

> WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:617
> +    g_signal_emit_by_name(webView, "viewport-changed", viewport); 
Don't need to cache webFrame or webView. Just do: 

 g_signal_emit_by_name(getViewFromFrame(kit(frame)), "viewport-changed",
viewport);

> WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:809
> +    // "viewport-changed" signal will be emitted when the main frame is
loaded.
Nit! All on one line.

> WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:812
> +	    WebKitViewport* viewport = webkit_viewport_new(arguments);
Again PlatformRefPtr.

> WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:813
> +	    g_signal_emit_by_name(webView, "viewport-changed", viewport); 
This can simply be g_signal_emit_by_name(webView, "viewport-changed",
ViewPortArguments());

> WebKit/gtk/webkit/webkitviewport.cpp:70
> +    viewport = WEBKIT_VIEWPORT(g_object_new(WEBKIT_TYPE_VIEWPORT, NULL));
Please make this declaration one line:
WebKitViewport* viewport = WEBKIT_VIEWPORT(g_object_new(WEBKIT_TYPE_VIEWPORT,
NULL));

> WebKit/gtk/webkit/webkitviewport.cpp:99
> +    WebKitViewportPrivate* priv = viewport->priv;
NULL should be 0 and no need to cache this, just do this:

g_return_val_if_fail(WEBKIT_IS_VIEWPORT(viewport), 0);
return viewport->priv->width;

> WebKit/gtk/webkit/webkitviewport.cpp:120
> +    return priv->width;
Same fix as above.