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

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth 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 69762: Proposed Patch4
https://bugs.webkit.org/attachment.cgi?id=69762&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=69762&action=review

Seems fine with the exception of calling the signal when the mainframe is
constructed.

> WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:915
> +
> +	   /* In order to reset the viewport, 
> +	    * The "viewport-config-changed" signal will be emitted when the
main frame is loaded.
> +	    */
> +	   WebCore::ViewportArguments arguments;
> +	   WebKitWebViewPrivate* webviewPriv =
WEBKIT_WEB_VIEW_GET_PRIVATE(webView);
> +	  
webkit_viewport_config_compute_configuration_internal(webviewPriv->viewportConf
ig.get(), arguments, WebCore::IntSize(0, 0));
> +	   g_signal_emit_by_name(webView, "viewport-config-changed",
webviewPriv->viewportConfig.get()); 

This shouldn't be needed any more. the dispatchViewportDataDidChange is called
when the body is inserted into the document if a viewport meta tag hasn't been
encountered at that point

> WebKit/gtk/webkit/webkitviewportconfig.cpp:35
> +    gboolean isValid;

It is always valid now, unless you can construct one your self.

> WebKit/gtk/webkit/webkitviewportconfig.cpp:49
> +static void webkit_viewport_config_init(WebKitViewportConfig* viewport)

which is seems that you can

> WebKit/gtk/webkit/webkitviewportconfig.cpp:219
> + * @available_width: the available width of the viewport
> + * @available_height: the available height of the viewport

Maybe make it clear that this is without any ui components, fixed or not. The
iPhone for instance has non fixed ui components