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

[bugzilla-daemon at webkit.org]

Xan Lopez <xan.lopez at gmail.com> has denied  review:
Bug 45443: [GTK] Support for viewport meta tag
https://bugs.webkit.org/show_bug.cgi?id=45443

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

------- Additional Comments from Xan Lopez <xan.lopez at gmail.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=70339&action=review

Aside from those comments (which are mostly about clarifying the API for the
users), this looks good. Please remember you need the r+ of two GTK+ reviewers
before adding new API to the port.

> LayoutTests/platform/gtk/Skipped:5895
>  

Why is this skipped? Open a bug about it or put a comment in the file at least.


> WebKit/gtk/webkit/webkitviewportattributes.cpp:22
> +

I'm really missing here (or maybe in the WebView signal?) a gtk-doc section
explaining what is this exactly and why would you want to use it, possibly with
links to some doc in the web somewhere. Otherwise it's pretty mysterious API in
general IMHO.

> WebKit/gtk/webkit/webkitwebview.cpp:2470
> +	* fit web content.

I don't think this helps in understanding what the signal is used for unless
you already know about ViewPort and stuff.

> WebKit/gtk/webkit/webkitwebview.cpp:2472
> +	* Since: 1.3.4

This (like all other Since: tags) has to be 1.3.5 now.

> WebKit/gtk/webkit/webkitwebview.cpp:4526
> +    webView->priv->viewportAttributes =
webkit_viewport_attributes_new(desktopWidth, deviceWidth, deviceHeight,
deviceDPI);

Missing adoptPlatformRef? Is this function mostly useful for testing? Perhaps
_set_ instead of _init_ ?

> WebKitTools/DumpRenderTree/win/LayoutTestControllerWin.cpp:1396
> +}

You only need to add this in Win?