[Webkit-unassigned] [Bug 45443] [GTK] Support for viewport meta tag

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=45443


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #67011|review?                     |review-
               Flag|                            |




--- Comment #4 from Martin Robinson <mrobinson at webkit.org>  2010-09-13 00:32:18 PST ---
(From update of attachment 67011)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.