Bug 45443 – [GTK] Support for viewport meta tag

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Joone Hur]

WebKitGtk needs to support the viewport meta tag, which was introduced by mobile Safari.
Because many mobile web sites already use it and QWebKit and WebKitEFL also support it.

For more information about the viewport meta tag, please refer to the URL below: http://developer.apple.com/safari/library/documentation/appleapplications/reference/safariwebcontent/usingtheviewport/usingtheviewport.html

[Joone Hur]

Created attachment 67011 [details]
Proposed patch

[WebKit Review Bot]

Attachment 67011 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/gtk/webkit/webkitviewport.h:48:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewport.h:49:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewport.h:50:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewport.h:51:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewport.h:55:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewport.h:58:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewport.h:61:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewport.h:64:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewport.h:67:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewport.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewport.h:73:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewport.h:76:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 12 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joone Hur]

Created attachment 67362 [details]
updated GtkLauncher for testing

This patch makes GtkLauncher handle the viewport-changed signal when you connect web pages for iPhone.

[Martin Robinson]

Comment on attachment 67011 [details]
Proposed patch

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.

[Joone Hur]

Created attachment 68913 [details]
Proposed Patch2

During resolving this issue, Opera viewport CSS properties spec. applied to the mainline.
https://bugs.webkit.org/show_bug.cgi?id=44201
Until then, we just had got the viewport meta tag information, but this patch also allows to get the calculated viewport configuration from WebCore.
It means that we can get the available viewport size, initial/maximum/minimum scale value through the device width/height and desktop width.

In addition, this patch supports viewport test cases (fast/viewport) except viewport-45.html.
It fails as follows,

--- /tmp/layout-test-results/fast/viewport/viewport-45-expected.txt    2010-09-27 16:56:08.000000000 +0900
+++ /tmp/layout-test-results/fast/viewport/viewport-45-actual.txt    2010-09-27 16:56:08.000000000 +0900
@@ -1,2 +1,2 @@
-viewport size 3200x3520 scale 0.100000 with limits [0.100000, 0.100000]
+viewport size 3199x3519 scale 0.100000 with limits [0.100000, 0.100000]

Therefore, I let DRT test skips this test case for WebKitGtk.

[WebKit Review Bot]

Attachment 68913 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/gtk/webkit/webkitviewportconfig.h:48:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:49:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:50:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:51:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:55:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:61:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:64:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:67:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:73:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:76:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:79:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:82:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:85:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:88:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitwebview.h:405:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitwebview.h:408:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 17 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Rohde Christiansen]

Comment on attachment 68913 [details]
Proposed Patch2

Some comments for now... will look more later

View in context: https://bugs.webkit.org/attachment.cgi?id=68913&action=review

> ChangeLog:8
> +
> +        [GTK] Support for viewport meta tag
> +        https://bugs.webkit.org/show_bug.cgi?id=45443
> +
> +        * GNUmakefile.am: Added webkitviewportconfig.h webkitviewportconfig.cpp 

Would be good with a bit more elaborated ChangeLog

> WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:606
> +void ChromeClient::didReceiveViewportArguments(Frame* frame, const WebCore::ViewportArguments& arguments) const

This is going to change soon in another patch, so you might want to hold off for a bit. There is going to be a dispatchViewportDataChanged or similar.

> WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:610
> +    WebCore::FloatRect rect = core(webview)->chrome()->pageRect();

Remember that the device size has to be given as in portrait mode. or better said, device width = width in portrait, and device height = width in landscape (this is without any ui elements)

> WebKit/gtk/webkit/webkitviewportconfig.cpp:40
> +struct _WebKitViewportConfigPrivate {
> +    gint width;
> +    gint height;
> +    gfloat targetDensityDPI;
> +    gfloat initialScaleFactor;
> +    gfloat minimumScaleFactor;
> +    gfloat maximumScaleFactor;
> +    gboolean isUserScalable;
> +    gboolean isValid;
> +    gint deviceDPI;
> +    gint deviceWidth;
> +    gint deviceHeight;
> +    gint desktopWidth;
> +};

You might want a way to extend this struct without causing ABI breakage

> WebKit/gtk/webkit/webkitviewportconfig.cpp:56
> +    priv->targetDensityDPI = 0.0;

uh... that doesnt seem right. Maybe you want to use -1.0 for invalid?

[Kenneth Rohde Christiansen]

Comment on attachment 68913 [details]
Proposed Patch2

View in context: https://bugs.webkit.org/attachment.cgi?id=68913&action=review

Looks good, though we need a GTK+ guy to look at the API.

> WebKit/gtk/webkit/webkitviewportconfig.cpp:226
> +void webkit_viewport_config_find_configuration(WebKitViewportConfig* viewportConfig, WebKitWebFrame* mainFrame, gint available_width, gint available_height)

I'm thinking about renaming find to compute.

[WebKit Review Bot]

Attachment 68913 [details] did not build on win:
Build output: http://queues.webkit.org/results/4038156

[Joone Hur]

> > WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:610
> > +    WebCore::FloatRect rect = core(webview)->chrome()->pageRect();
> 
> Remember that the device size has to be given as in portrait mode. or better said, device width = width in portrait, and device height = width in landscape (this is without any ui elements)

The device size can be set when a WebKitViewportConfig object is created through webkit_viewport_config_new, so the return value of pageRect() is not relevant to the device size.

According to the opera spec, "available-width is the width of the visual viewport (device-width - decorations) at scale factor 1.0"
I think that pageRect() can be used for getting the available-size. Of course, it can have the same value of the device width if there is no any border.

> > WebKit/gtk/webkit/webkitviewportconfig.cpp:40
> > +struct _WebKitViewportConfigPrivate {
> > +    gint width;
> > +    gint height;
> > +    gfloat targetDensityDPI;
> > +    gfloat initialScaleFactor;
> > +    gfloat minimumScaleFactor;
> > +    gfloat maximumScaleFactor;
> > +    gboolean isUserScalable;
> > +    gboolean isValid;
> > +    gint deviceDPI;
> > +    gint deviceWidth;
> > +    gint deviceHeight;
> > +    gint desktopWidth;
> > +};
> 
> You might want a way to extend this struct without causing ABI breakage

This is a private structure defined in cpp file , so applications cannot access this structure directly.

Thank you for the review.

[Joone Hur]

Created attachment 69198 [details]
Proposed Patch3

I updated the patch with the changes commented by Kenneth.

[WebKit Review Bot]

Attachment 69198 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/gtk/webkit/webkitviewportconfig.h:48:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:49:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:50:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:51:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:55:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:61:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:64:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:67:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:73:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:76:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:79:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:82:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:85:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:88:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitwebview.h:405:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitwebview.h:408:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 17 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 69198 [details] did not build on win:
Build output: http://queues.webkit.org/results/4223014

[Joone Hur]

Created attachment 69762 [details]
Proposed Patch4

https://bugs.webkit.org/show_bug.cgi?id=46755 landed, so I updated this patch.

[WebKit Review Bot]

Attachment 69762 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/gtk/webkit/webkitviewportconfig.h:48:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:49:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:50:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:51:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:55:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:61:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:64:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:67:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:73:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:76:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:79:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:82:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:85:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportconfig.h:88:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitwebview.h:405:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitwebview.h:408:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 17 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Rohde Christiansen]

Comment on attachment 69762 [details]
Proposed Patch4

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->viewportConfig.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

[Gustavo Noronha (kov)]

Comment on attachment 69762 [details]
Proposed Patch4

View in context: https://bugs.webkit.org/attachment.cgi?id=69762&action=review

> WebKit/gtk/webkit/webkitviewportconfig.h:58
> +WEBKIT_API WebKitViewportConfig* 
> +webkit_viewport_config_new                      (gint desktopWidth, 
> +                                                 gint deviceWidth, 
> +                                                 gint deviceHeight, 
> +                                                 gint deviceDPI);

I see no reason for the API user to create a new WebKitViewportConfig object, I would remove this. Also, perhaps naming this WebKitViewportFeatures, since we have WindowFeatures would make it easier to understand what this is about?

> WebKit/gtk/webkit/webkitviewportconfig.h:91
> +WEBKIT_API void 
> +webkit_viewport_config_compute_configuration    (WebKitViewportConfig* viewportConfig, 
> +                                                 WebKitWebFrame* mainFrame, 
> +                                                 gint available_width,
> +                                                 gint available_height);

I don't understand the purpose of this call. This feels like the user is having to manually do something that should be done automatically. How do you see API users using this?

> WebKit/gtk/webkit/webkitwebview.cpp:4497
> +void webkit_web_view_set_viewport_config(WebKitWebView* webView, WebKitViewportConfig* viewportConfig)
> +{
> +    g_return_if_fail(WEBKIT_IS_WEB_VIEW(webView));
> +    g_return_if_fail(WEBKIT_IS_VIEWPORT_CONFIG(viewportConfig));
> +
> +    webView->priv->viewportConfig = viewportConfig;

It looks like we don't communicate the change to WebCore - I believe there is no real point to, anyway, right? Unless you change the viewport stuff through the DOM itself. I wouldn't allow setting the viewport features into the view.

[Kenneth Rohde Christiansen]

> I don't understand the purpose of this call. This feels like the user is having to manually do something that should be done automatically. How do you see API users using this?

We also do this semi automatically in qt, we the user still has to provide the visible area (it changes after rotation)

[Gustavo Noronha (kov)]

(In reply to comment #18)
> > I don't understand the purpose of this call. This feels like the user is having to manually do something that should be done automatically. How do you see API users using this?
> 
> We also do this semi automatically in qt, we the user still has to provide the visible area (it changes after rotation)

That makes sense! Do you see any reason not to obtain this automatically from the widget allocation?

[Kenneth Rohde Christiansen]

(In reply to comment #19)
> (In reply to comment #18)
> > > I don't understand the purpose of this call. This feels like the user is having to manually do something that should be done automatically. How do you see API users using this?
> > 
> > We also do this semi automatically in qt, we the user still has to provide the visible area (it changes after rotation)
> 
> That makes sense! Do you see any reason not to obtain this automatically from the widget allocation?

Because we cannot find a generic way of doing this. The viewport is different because we use tiling, thus the actual webview is the size of the contents and we use a view to pan it (only updating visible tiles). There can also be UI components on top.

We tried obtaining this manually and it has just been a pain in the a*** and not proved very flexible.

[Joone Hur]

(In reply to comment #17)
> (From update of attachment 69762 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69762&action=review
>
> > WebKit/gtk/webkit/webkitviewportconfig.h:58
> > +WEBKIT_API WebKitViewportConfig*
> > +webkit_viewport_config_new                      (gint desktopWidth,
> > +                                                 gint deviceWidth,
> > +                                                 gint deviceHeight,
> > +                                                 gint deviceDPI);
>
> I see no reason for the API user to create a new WebKitViewportConfig object, I would remove this. Also, perhaps naming this WebKitViewportFeatures, since we have WindowFeatures would make it easier to understand what this is about?

Yes, right.  I will remove this function from the public header file and change WebKitViewportConfig to WebKitViewportFeatures.


> > WebKit/gtk/webkit/webkitwebview.cpp:4497
> > +void webkit_web_view_set_viewport_config(WebKitWebView* webView, WebKitViewportConfig* viewportConfig)
> > +{
> > +    g_return_if_fail(WEBKIT_IS_WEB_VIEW(webView));
> > +    g_return_if_fail(WEBKIT_IS_VIEWPORT_CONFIG(viewportConfig));
> > +
> > +    webView->priv->viewportConfig = viewportConfig;
>
> It looks like we don't communicate the change to WebCore - I believe there is no real point to, anyway, right? Unless you change the viewport stuff through the DOM itself. I wouldn't allow setting the viewport features into the view.

As Kenneth said, when the size of the viewport is changed, we need to compute the optimal viewport configuration again. Therefore, there should be a way of accessing the viewportConfig via the view like getting the WindowFeatures.

[Kenneth Rohde Christiansen]

> As Kenneth said, when the size of the viewport is changed, we need to compute the optimal viewport configuration again. Therefore, there should be a way of accessing the viewportConfig via the view like getting the WindowFeatures.

Be careful what you call the viewport. There is the actual visible viewport (which might grow, like when the address bar scrolls out on the iphone) and there is the layout viewport.

When calculating the "viewport features", as you are calling it, we need the minimum actual visible viewport as input and it will output the actual layout viewport, plus min, max, initial scale etc.

I'm not sure "viewport features" is the best name. We are currently considering calling it viewport attributes for Qt - finding a proper name for this is hard :-)

[Joone Hur]

(In reply to comment #22)
> I'm not sure "viewport features" is the best name. We are currently considering calling it viewport attributes for Qt - finding a proper name for this is hard :-)

kov mentioned that he also likes ViewportAttributes as the name from #47202 
So, I will use WebKitViewportAttributes instead of WebKitViewportConfig.

[Joone Hur]

Created attachment 70074 [details]
Proposed Patch5

- renamed WebKitViewportConfig to WebKitViewportAttributes.
- renamed viewport-config-changed signal to viewport-attributes-changed.
- not allowed to emit viewport-attributes-changed signal when dispatchDidCommitLoad() was called.
- moved webkit_viewport_attributes_new() function declaration to webkitprivate.h
- changed webkit_web_view_set_viewport_config() to webkit_web_view_init_viewport_attributes

[WebKit Review Bot]

Attachment 70074 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/gtk/webkit/webkitviewportattributes.h:48:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:49:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:50:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:51:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:55:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:58:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:61:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:64:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:67:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:73:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:76:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:79:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:82:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitwebview.h:405:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitwebview.h:406:  Extra space between gint and desktopWidth  [whitespace/declaration] [3]
WebKit/gtk/webkit/webkitwebview.h:407:  Extra space between gint and deviceWidth  [whitespace/declaration] [3]
WebKit/gtk/webkit/webkitwebview.h:408:  Extra space between gint and deviceHeight  [whitespace/declaration] [3]
WebKit/gtk/webkit/webkitwebview.h:409:  Extra space between gint and deviceDPI  [whitespace/declaration] [3]
WebKit/gtk/webkit/webkitwebview.h:412:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 20 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Rohde Christiansen]

Comment on attachment 70074 [details]
Proposed Patch5

View in context: https://bugs.webkit.org/attachment.cgi?id=70074&action=review

Looks good to me, but please pass the API through Gustavo or another GTK+ reviewer.

Also you need to change one call before this can be committed.

> WebKit/gtk/webkit/webkitviewportattributes.cpp:248
> +    WebCore::ViewportAttributes attrs = WebCore::findConfigurationForViewportData(arguments, priv->desktopWidth, priv->deviceWidth, 

The API recently changed to computeViewportAttribute.

[Joone Hur]

Created attachment 70339 [details]
Proposed Patch6

* Renamed WebCore::findConfigurationForViewportData to WebCore::computeViewportAttributes.
* Renamed layoutViewport to layoutSize in the ViewportAttributes Structure.

Thanks!

[WebKit Commit Bot]

Comment on attachment 70339 [details]
Proposed Patch6

Rejecting patch 70339 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']" exit_code: 2
Last 500 characters of output:
....ok
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnPropertyValue..........ok
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/prepareParsedPatch.............ok
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/runPatchCommand................ok
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/setChangeLogDateAndReviewer....ok
All tests successful.
Files=14, Tests=304,  1 wallclock secs ( 0.73 cusr +  0.17 csys =  0.90 CPU)
Running build-dumprendertree
Compiling DumpRenderTree failed!

Full output: http://queues.webkit.org/results/4343003

[Xan Lopez]

Comment on attachment 70339 [details]
Proposed Patch6

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?

[Joone Hur]

Created attachment 70527 [details]
Proposed Patch7

I updated this patch with the changes commented by Xan as follows, (https://bugs.webkit.org/show_bug.cgi?id=45443#c29)
* Added a bug number in the Skipped file about fast/viewport/viewport-45.html. (https://bugs.webkit.org/show_bug.cgi?id=47481)
* Added an introduction to the WebKitViewportAttributes for gtk-doc section.
* Changed the API version to 1.3.5.
* Called webkit_viewport_attributes_new through adoptPlatformRef.
* Renamed webkit_web_view_init_viewport_attributes to webkit_web_view_set_viewport_attributes.
* Added the configurationForViewport method in Mac,Wx port.

[WebKit Review Bot]

Attachment 70527 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/gtk/webkit/webkitviewportattributes.h:48:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:49:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:50:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:51:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:55:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:58:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:61:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:64:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:67:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:73:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:76:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:79:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:82:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitwebview.h:405:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitwebview.h:406:  Extra space between gint and desktopWidth  [whitespace/declaration] [3]
WebKit/gtk/webkit/webkitwebview.h:407:  Extra space between gint and deviceWidth  [whitespace/declaration] [3]
WebKit/gtk/webkit/webkitwebview.h:408:  Extra space between gint and deviceHeight  [whitespace/declaration] [3]
WebKit/gtk/webkit/webkitwebview.h:409:  Extra space between gint and deviceDPI  [whitespace/declaration] [3]
WebKit/gtk/webkit/webkitwebview.h:412:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 20 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Comment on attachment 70074 [details]
Proposed Patch5

Cleared Kenneth Rohde Christiansen's review+ from obsolete attachment 70074 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Martin Robinson]

Comment on attachment 70527 [details]
Proposed Patch7

View in context: https://bugs.webkit.org/attachment.cgi?id=70527&action=review

Great patch! The API changes look fine to me and the documentation is awesome. I have a few very small nits.

> WebKit/gtk/ChangeLog:10
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [GTK] Support for viewport meta tag
> +        https://bugs.webkit.org/show_bug.cgi?id=45443
> +
> +        This patch allows to get the viewport attributes to set the width and initial scale of the viewport
> +        used to display web content.
> +

I think this ChangeLog should be a bit bigger. This patch effectively adds viewport support to the GTK+ port, so it should probably have at least one sentence about what the viewport support does.

> WebKit/gtk/webkit/webkitviewportattributes.cpp:116
> + * Returns the width of the #WebKitViewportAttributes in pixels.
> + *
> + * Returns: the width of the #WebKitViewportAttributes in pixels

Does this really need to be repeated for all these methods?

> WebKit/gtk/webkit/webkitviewportattributes.cpp:124
> +    g_return_val_if_fail(WEBKIT_IS_VIEWPORT_ATTRIBUTES(viewportAttributes), 0);
> +
> +    return viewportAttributes->priv->width;

I'd say remove the extra line here and in the rest of the two-line methods.

> WebKit/gtk/webkit/webkitviewportattributes.cpp:268
> +// for internal use only

New comments should be complete sentences.

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

I'd say remove the extra line from the two line methods again.

[Gustavo Noronha (kov)]

Comment on attachment 70527 [details]
Proposed Patch7

View in context: https://bugs.webkit.org/attachment.cgi?id=70527&action=review

> WebKit/gtk/webkit/webkitviewportattributes.h:85
> +WEBKIT_API void 
> +webkit_viewport_attributes_computation              (WebKitViewportAttributes* viewportAttributes, 
> +                                                     WebKitWebFrame* mainFrame, 
> +                                                     gint availableWidth,
> +                                                     gint availableHeight);

Computation is a noun, so it's a bad name for this method (which should always be verbs). Having to provide the main frame here is also a bit weird. Perhaps the viewport should have a weak reference to the view?

> WebKit/gtk/webkit/webkitwebview.h:409
> +WEBKIT_API void 
> +webkit_web_view_set_viewport_attributes         (WebKitWebView        *webView,
> +                                                 gint                 desktopWidth, 
> +                                                 gint                 deviceWidth, 
> +                                                 gint                 deviceHeight, 
> +                                                 gint                 deviceDPI);

I dislike this because it breaks the expectation that a get will return an object and a set would set the same object. I do not really understand why you would want to set these values manually. Why do we need this function?

[Gustavo Noronha (kov)]

(In reply to comment #22)
> > As Kenneth said, when the size of the viewport is changed, we need to compute the optimal viewport configuration again. Therefore, there should be a way of accessing the viewportConfig via the view like getting the WindowFeatures.
> 
> Be careful what you call the viewport. There is the actual visible viewport (which might grow, like when the address bar scrolls out on the iphone) and there is the layout viewport.
> 
> When calculating the "viewport features", as you are calling it, we need the minimum actual visible viewport as input and it will output the actual layout viewport, plus min, max, initial scale etc.

OK, let me try to describe this process so my thinking is clearer:

Say you load a page that has a viewport tag. ChromeClient::dispatchViewportDataDidChange will eventually be called. At this point you will want to know the following:

The size of the screen (deviceHeigh/deviceWidth), the size of the are visible to the user (availableWidth/availableHeight), the screen DPI, the "desktop width", so that you can provide WebCore::computeViewportAttributes with them.

Screen size and DPI should be obtainable automatically. I don't see a point in only providing available width and height when you call compute manually and using the contents size in the automatic computation, and I'm really lost on the meaning of desktop width to be honest.

So what I'm thinking is this:

1. We provide a viewport-attributes-changed signal in the view; while handling that signal you have to feed the ViewportAttributes with the data it requires for the computation by setting properties on it - after handling this you just need to get the ViewportAttributes object that is owned by the view and make use of the computed values as needed; we can have a viewport-attributes-available signal in the ViewportAttributes object, or a state tracking enum that we can monitor for notify signals
2. We throw away webkit_web_view_set_viewport_attributes and webkit_viewport_attributes_computation entirely

Are there any use cases that would not be covered by this that you can see? I think it makes the API more elegant and simple.

[Gustavo Noronha (kov)]

We discussed on IRC and came up with webkit_viewport_recompute which would trigger the normal process, by using the same code as the automatic path, including firing the signal.

[Joone Hur]

Thank you for your comments.
Before updating the patch, I'd like to confirm the following issues,

1) Add "void webkit_web_view_recompute_viewport_attributes(WebKitWebView* webView, gint availableWidth, gint availableHeight) instead of webkit_viewport_recompute to trigger a viewport-attribute-changed signal.

It seems to be easier to trigger the signal in WebKitWebView.  Also, we still need the availableWidth/Height parameters although these values are able to be obtained in WebKit. Because, DRT needs this function given arbitrary availableWidth/height for testing. 
Instead, we can remove webkit_viewport_attributes_computation.

2) Rename webkit_web_view_set_viewport_attributes to webkit_web_view_init_viewport_attributes.

We need this function to set the deskopWidth, deviceWidth/Height, and deviceDPI. Because WebKit cannot get these values by itself. In addition, if these values are changed, a UA should call this function to initialize the viewport attributes again.

As kov commented, we usually expect that a set would set the same object used by the get. So I'm going to rename this function as above.
 
Any suggestions are appreciated.

[Joone Hur]

(In reply to comment #37) 
> 2) Rename webkit_web_view_set_viewport_attributes to webkit_web_view_init_viewport_attributes.
> 
> We need this function to set the deskopWidth, deviceWidth/Height, and deviceDPI. Because WebKit cannot get these values by itself. In addition, if these values are changed, a UA should call this function to initialize the viewport attributes again.

We are able to get the deviceWidth/Height and deviceDPI in WebKitGtk as follow,

static void webkit_web_view_init(WebKitWebView* webView)
{
...
 // Attach a new WebKitViewportAttrbutes object given the viewport configuration values.
    GdkScreen* screen = gtk_widget_get_screen(GTK_WIDGET(webView));
    priv->viewportAttributes = adoptPlatformRef(webkit_viewport_attributes_new(980, gdk_screen_get_width(screen), gdk_screen_get_height(screen), webViewGetDPI(webView)));

Therefore, we don't need to call webkit_web_view_init_viewport_attributes on real devices, but DRT still needs  this function to initialize the viewport attributes manually.

[Gustavo Noronha (kov)]

(In reply to comment #37)
> Thank you for your comments.
> Before updating the patch, I'd like to confirm the following issues,
> 
> 1) Add "void webkit_web_view_recompute_viewport_attributes(WebKitWebView* webView, gint availableWidth, gint availableHeight) instead of webkit_viewport_recompute to trigger a viewport-attribute-changed signal.
> 
> It seems to be easier to trigger the signal in WebKitWebView.  Also, we still need the availableWidth/Height parameters although these values are able to be obtained in WebKit. Because, DRT needs this function given arbitrary availableWidth/height for testing. 
> Instead, we can remove webkit_viewport_attributes_computation.

No, my idea is that WebKitViewportAttributes will keep a weak reference to the WebView. We will have this function:

void webkit_viewport_attributes_recompute(WebKitViewport*);

It will trigger the signal. While handling the signal you have to set availableWidth/Height on the ViewportAttributes object. No need for any other function for these.

If there's something DRT needs it's fine to have, but it should _not_ be public API just because DRT needs it. There are plenty of functions that we already use in DRT that are not public API (they are in webkitprivate.h), you can use them as example =).

[Joone Hur]

Created attachment 71716 [details]
Proposed patch8

I updated this patch with the changes commented by Martin and kov.
Thank you.

[WebKit Review Bot]

Attachment 71716 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/gtk/webkit/webkitviewportattributes.h:48:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:49:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:50:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:51:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:55:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:58:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:61:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:64:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:67:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:73:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:76:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:79:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:82:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitwebview.h:405:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 15 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 71716 [details]
Proposed patch8

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?

[Gustavo Noronha (kov)]

Comment on attachment 71716 [details]
Proposed patch8

View in context: https://bugs.webkit.org/attachment.cgi?id=71716&action=review

I agree with Martin we should make those functions part of DRTSupport if possible, but I'm not against pushing them if it takes a while. Also, I'm not against having comments. Just make sure they have only one * at the first line, that way gtk-doc will skip them. We're almost there regarding API usability, but it still looks a bit backwards. Hopefully my comments will help a bit =)

> WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:613
> +void ChromeClient::dispatchViewportDataDidChange(const ViewportArguments& arguments) const
> +{
> +    WebKitViewportAttributes* viewportAttributes = webkit_web_view_get_viewport_attributes(m_webView);
> +    FloatRect rect = core(m_webView)->chrome()->pageRect();
> +    webkit_viewport_attributes_compute_internal(viewportAttributes, static_cast<gint>(rect.width()), static_cast<gint>(rect.height()));
> +
> +    g_signal_emit_by_name(m_webView, "viewport-attributes-changed", viewportAttributes); 

You should just call webkit_viewport_recompute here =).

> WebKit/gtk/webkit/webkitviewportattributes.cpp:261
> +    g_return_if_fail(WEBKIT_IS_VIEWPORT_ATTRIBUTES(viewportAttributes));
> +
> +    WebKitWebView* webView = kit(viewportAttributes->priv->page);
> +    FloatRect rect = viewportAttributes->priv->page->chrome()->pageRect();
> +    webkit_viewport_attributes_compute_internal(viewportAttributes, static_cast<gint>(rect.width()), static_cast<gint>(rect.height()));
> +
> +    g_signal_emit_by_name(webView, "viewport-attributes-changed", viewportAttributes); 

This seems to be the other way around than what is needed... You first need the information, to then make the calculation. What's the point of computing the attributes as if the available size was the pageRect first, and how do you use the available size provided afterwards?

Also, you should get the DPI and screen size here instead of at the time the object is created, otherwise it is not very useful to call this when screen stuff changes.

[Martin Robinson]

(In reply to comment #43)
> I agree with Martin we should make those functions part of DRTSupport if possible, but I'm not against pushing them if it takes a while. Also, I'm not against having comments. Just make sure they have only one * at the first line, that way gtk-doc will skip them. We're almost there regarding API usability, but it still looks a bit backwards. Hopefully my comments will help a bit =)

Just a heads up, DRTSupport just landed landed last night. :) I agree with your comments about the documentation, though I think we need to be extra careful. I believe that private stuff has snuck into our documentation in the past.

[Joone Hur]

(In reply to comment #43)
> (From update of attachment 71716 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71716&action=review

> > WebKit/gtk/webkit/webkitviewportattributes.cpp:261
> > +    g_return_if_fail(WEBKIT_IS_VIEWPORT_ATTRIBUTES(viewportAttributes));
> > +
> > +    WebKitWebView* webView = kit(viewportAttributes->priv->page);
> > +    FloatRect rect = viewportAttributes->priv->page->chrome()->pageRect();
> > +    webkit_viewport_attributes_compute_internal(viewportAttributes, static_cast<gint>(rect.width()), static_cast<gint>(rect.height()));
> > +
> > +    g_signal_emit_by_name(webView, "viewport-attributes-changed", viewportAttributes); 
> 
> This seems to be the other way around than what is needed... You first need the information, to then make the calculation. What's the point of computing the attributes as if the available size was the pageRect first, and how do you use the available size provided afterwards?

The available size means the visible area of a web page, so I call the pageRect to get the visible area because it returns the webview size.  In addition, the available size is used to calculate the initial and minimum scale value through WebCore::computeViewportAttributes with the given desktopWidth, deviceWidth/height, and DeviceDPI.

> Also, you should get the DPI and screen size here instead of at the time the object is created, otherwise it is not very useful to call this when screen stuff changes.
I assumed that the DPI and screen size are constant values, but we need to get the screen(device) size again as well as the available size when the screen is rotated.

Thanks, Martin and Kov, I will update the patch soon.

[Joone Hur]

(In reply to comment #42)
> (From update of attachment 71716 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71716&action=review
> 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?

It is possible to move webkit_viewport_attributes_compute_internal into the body of webkit_viewport_attribute_recompute if not consider DRT. In addition, I am going to remove webkit_web_view_init_viewport_attributes function because it is only used for DRT. Instead, I'll add a similar function in DumpRenderTreeSupportGtk as you told.

Sharing the same functionalities between WebKit and DRT seems very hard in this case.

[Joone Hur]

Created attachment 72287 [details]
Proposed Patch9

I applied Martin and Kov's comments to the patch.
Also, I have renamed webViewGetDPI to webkit_web_view_get_dpi and made it a private function to be called by webkit_viewport_attributes_recompute.
I am wondering if it makes sense.

Any suggestions are appreciated.
Thank you.

[WebKit Review Bot]

Attachment 72287 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/gtk/webkit/webkitviewportattributes.h:48:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:49:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:50:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:51:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:55:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:58:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:61:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:64:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:67:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:73:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:76:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:79:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitviewportattributes.h:82:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitwebview.h:405:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 15 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 72287 [details]
Proposed Patch9

View in context: https://bugs.webkit.org/attachment.cgi?id=72287&action=review

> WebKit/gtk/webkit/webkitprivate.h:326
> +    void
> +    webkit_viewport_attributes_set(WebKitViewportAttributes* viewportAttributes, const WebCore::ViewportAttributes& attrs, gboolean userScalable);

This may be uneeded. See below.

> WebKitTools/DumpRenderTree/LayoutTestController.h:287
> +    void configurationForViewport(int availableWidth, int availableHeight);

This method actually dumps the viewport so it should be called dumpConfigurationForViewport.

> WebKitTools/DumpRenderTree/gtk/LayoutTestControllerGtk.cpp:836
> +    DumpRenderTreeSupportGtk::updateViewportAttributes(webView, availableWidth, availableHeight);

I'm a little worried because the Qt implementation doesn't actually set the viewport attributes on the view. Seemingly it just calculates the appropriate viewport given the size. Shouldn't we try to replicate that as closely as possible?

Perhaps this should just call a method like this:

WebKitViewportAttributes* attributes = DumpRenderTreeSupportGtk::computeViewportAttributesForSize(availbleWidth, availableHeight);

[Joone Hur]

(In reply to comment #49)
> (From update of attachment 72287 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72287&action=review
> 
> > WebKit/gtk/webkit/webkitprivate.h:326
> > +    void
> > +    webkit_viewport_attributes_set(WebKitViewportAttributes* viewportAttributes, const WebCore::ViewportAttributes& attrs, gboolean userScalable);
> 
> This may be uneeded. See below.

If we don't need it, we should use the setters or GObject property methods.

> 
> > WebKitTools/DumpRenderTree/LayoutTestController.h:287
> > +    void configurationForViewport(int availableWidth, int availableHeight);
> 
> This method actually dumps the viewport so it should be called dumpConfigurationForViewport.
> 
> > WebKitTools/DumpRenderTree/gtk/LayoutTestControllerGtk.cpp:836
> > +    DumpRenderTreeSupportGtk::updateViewportAttributes(webView, availableWidth, availableHeight);
> 
> I'm a little worried because the Qt implementation doesn't actually set the viewport attributes on the view. Seemingly it just calculates the appropriate viewport given the size. Shouldn't we try to replicate that as closely as possible?

Do we need to create a new WebKitViewportAttributes object whenever the browser loads the viewport meta tags or the device screen changes? If so, it seems a bit overhead, but I will try it.

> Perhaps this should just call a method like this:
> 
> WebKitViewportAttributes* attributes = DumpRenderTreeSupportGtk::computeViewportAttributesForSize(availbleWidth, availableHeight);

[Antonio Gomes]

Please add stubs to WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp|h too.

[Joone Hur]

Created attachment 73192 [details]
Proposed Patch10

Some changes have been made to the patch according to Martin's suggestions.
- webkit_viewport_attributes_set has been removed.
- webkit_webview_get_viewport_attributes has been removed.
- Instances of the WebKitViewportAttributes are created on the fly when they need.
- All getters of the WebKitViewportAttributes has been removed.

Thank you!

[Joone Hur]

(In reply to comment #51)
> Please add stubs to WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp|h too.
It doesn't seems to need because QWebKit already has dumpConfigurationForViewport function and it doesn't share the same base class.

[Gustavo Noronha (kov)]

Comment on attachment 73192 [details]
Proposed Patch10

View in context: https://bugs.webkit.org/attachment.cgi?id=73192&action=review

> WebKit/gtk/webkit/webkitviewportattributes.cpp:301
> + */
> +void webkit_viewport_attributes_recompute(WebKitWebView* webView)
> +{
> +    g_return_if_fail(WEBKIT_IS_WEB_VIEW(webView));
> +
> +    GdkScreen* screen = gtk_widget_get_screen(GTK_WIDGET(webView));
> +    FloatRect rect = webView->priv->corePage->chrome()->pageRect();
> +    ViewportArguments arguments = webView->priv->corePage->mainFrame()->document()->viewportArguments();
> +    // The DesktopWidth has 980, because this value works well for most web pages designed for desktop user agents.
> +    ViewportAttributes attrs = computeViewportAttributes(arguments, 980, gdk_screen_get_width(screen), gdk_screen_get_height(screen), webkit_web_view_get_dpi(webView), IntSize(static_cast<gint>(rect.width()), static_cast<gint>(rect.height())));
> +    PlatformRefPtr<WebKitViewportAttributes> viewportAttributes(adoptPlatformRef(webkit_viewport_attributes_new(attrs, arguments.userScalable)));
> +
> +    g_signal_emit_by_name(webView, "viewport-attributes-changed", viewportAttributes.get()); 

I still don't understand your choice of only firing the signal after you have computed the values. Can you describe to me how you would use this API in a browser?

> WebKit/gtk/webkit/webkitviewportattributes.h:58
> +WEBKIT_API void 
> +webkit_viewport_attributes_recompute                (WebKitWebView* webView); 

This should take a WebKitViewPort as the argument, not a web view.

> WebKit/gtk/webkit/webkitwebview.cpp:1394
> -static gdouble webViewGetDPI(WebKitWebView* webView)
> +gdouble webkit_web_view_get_dpi(WebKitWebView* webView)

I suggest keeping the name, using GTK+ style for non-API stuff is actually discouraged (despite it being everywhere =().

[Joone Hur]

(In reply to comment #54)
> (From update of attachment 73192 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73192&action=review
> 
> > WebKit/gtk/webkit/webkitviewportattributes.cpp:301
> > + */
> > +void webkit_viewport_attributes_recompute(WebKitWebView* webView)
> > +{
> > +    g_return_if_fail(WEBKIT_IS_WEB_VIEW(webView));
> > +
> > +    GdkScreen* screen = gtk_widget_get_screen(GTK_WIDGET(webView));
> > +    FloatRect rect = webView->priv->corePage->chrome()->pageRect();
> > +    ViewportArguments arguments = webView->priv->corePage->mainFrame()->document()->viewportArguments();
> > +    // The DesktopWidth has 980, because this value works well for most web pages designed for desktop user agents.
> > +    ViewportAttributes attrs = computeViewportAttributes(arguments, 980, gdk_screen_get_width(screen), gdk_screen_get_height(screen), webkit_web_view_get_dpi(webView), IntSize(static_cast<gint>(rect.width()), static_cast<gint>(rect.height())));
> > +    PlatformRefPtr<WebKitViewportAttributes> viewportAttributes(adoptPlatformRef(webkit_viewport_attributes_new(attrs, arguments.userScalable)));
> > +
> > +    g_signal_emit_by_name(webView, "viewport-attributes-changed", viewportAttributes.get()); 
> 
> I still don't understand your choice of only firing the signal after you have computed the values. Can you describe to me how you would use this API in a browser?

The example is included in the source file.

* <informalexample><programlisting>
 * /<!-- -->* Connect to the viewport-attributes-changes signal *<!-- -->/
 * g_signal_connect (webView, "viewport-attributes-changed", G_CALLBACK (viewport_change_cb), webView);
 g_signal_connect (web_view, "size-allocate", G_CALLBACK(window_size_change_cb), web_view);
 *
 * /<!-- -->* Handle the signal to get the initial scale value *<!-- -->/
 * static void
 * viewport_change_cb(WebKitWebView* webView, WebKitViewportAttributes* viewportAttributes)
 * {
 *     gfloat initialScale; 
 *     g_object_get(G_OBJECT(viewportAttributes), "initial-scale-factor", &initialScale, NULL);
 *     webkit_web_view_set_full_content_zoom(webView, TRUE);
 *     webkit_web_view_set_zoom_level(webView, initialScale);
 * }
 * </programlisting></informalexample>
 */

When the size of the viewport changes, we need to recompute the viewport attribute as follows,

static int window_width = 0;
static void
window_size_change_cb(WebKitWebView* web_view, GtkAllocation* allocation, gpointer user_data)
{
    if (window_width != allocation->width)
      webkit_viewport_attributes_recompute(web_view);   
    window_width = allocation->width;
}

This example code makes the browser work like Safari on the iPhone.

[Joone Hur]

Created attachment 73466 [details]
viewport patch with example code

I am trying to update the patch according to the following requirements.
- WebkitWebView doesn't contain a WebKitViewportAttributes instance.
- Try to minimize additional new pubic APIs.


In addition, I'd like to let you know there are two ways to get a WebKitViewportAttributes instance in this patch.

1) get an a WebKitViewportAttributes instance through the viewport-attributes-changed signal when the web page has the viewport meta tags.
2) create a WebKitViewportAttributes instance directly when the viewport configuration(device size,dpi..) changes.

This patch includes an example code, so you could understand how the user agents use the viewport attributes to make web pages fit the device screen.

Thank you.

[Gustavo Noronha (kov)]

Created attachment 73751 [details]
proposed API

ok, since I'm having trouble explaining my proposed API, I decided to write it, so you can understand what I propose and point me to any issues you see with it, and we can move forward with this instead (I did not modify ChangeLogs or the sample code comments for now).

As you will see from the code, this is what happens:

* Page with viewport tag is loaded
* viewport-attributes-recompute-requested is emitted, giving the application the possibility of providing available size, and even override screen size if required
* viewport attributes computation is performed
* viewport attributes object is marked as valid
* viewport-attributes-available is emitted, which the application can use to apply any required change to the window, etc

When the window size changes this is what happens:

* application calls webkit_viewport_recompute
* all of the steps described above happens

Open questions:

* Qt uses a hardcoded DPI of 160, why is that?
* www.engadget.com seems to work fine, except it has scrolling 100% of the time, probably because our calculation will have to take the space occupied by the scrollbars into account =(
* most sites apparently do some crazy stuff, providing very big width and height values, and a very small scale factor - it looks like using full content zoom alone is not enough!
* full content zoom fails miserably at resizing text in http://touch.facebook.com/, which is not fun

So, do you guys see any trouble with this API? What use cases are not covered? What was better in the previous approach?

[Gustavo Noronha (kov)]

Created attachment 73753 [details]
proposed API

Oops, this is the correct patch - I forgot to go back to using webViewGetDPI after testing using 160 hardcoded =P. The rest of the previous comment still applies.

[Joone Hur]

> * viewport-attributes-recompute-requested is emitted, giving the application the possibility of providing available size, and even override screen size if required

It seems quite good that adding the viewport-attributes-recompute-requested signal allows the user to override the screenWidth/height and availableWidth/height.
However, according to the viewport meta-tag spec, the device-width/height keyword is used instead of the screen-width/height. So, I think that using the device-width/height would avoid confusion for the developers.

> * Qt uses a hardcoded DPI of 160, why is that?
I'm not sure, but Nokia device may use the value.

Is it wrong to call webViewGetDPI() to set the deviceDPI value basically? 

> * www.engadget.com seems to work fine, except it has scrolling 100% of the time, probably because our calculation will have to take the space occupied by the scrollbars into account =(
> * most sites apparently do some crazy stuff, providing very big width and height values, and a very small scale factor - it looks like using full content zoom alone is not enough!

Your patch related to GtkLauncher doesn't scale web pages if the web pages don't have viewport meta tags. So, do you mean that you tested the web sites, which have viewport meta tags?
In case of apple.com, it has viewport meta tags, and works well.

> * full content zoom fails miserably at resizing text in http://touch.facebook.com/, which is not fun

Most mobile sites for the iPhone are not zoomed-in/out because user-scalable = 0 as follows,

<meta name="viewport" content="width=device-width; initial-scale=1.0; maximum-scale=1.0; user-scalable=0;" /> 

> So, do you guys see any trouble with this API? What use cases are not covered? What was better in the previous approach?

We need to let the desktopWidth, deviceDPI also be overridden, because there would be some cases for the user to set these values manually.

Thanks kov, your patch improved my patch so much.

[Kenneth Rohde Christiansen]

> * Qt uses a hardcoded DPI of 160, why is that?

It is the dpi of the original iPhone and Android devices, so that it what web authors has been designing for.

> * most sites apparently do some crazy stuff, providing very big width and height values, and a very small scale factor - it looks like using full content zoom alone is not enough!

Are you using ; in  the  metatag instead of , ? I had that issue on facebook etc. Google Android used to support ; but they say that they have removed that support.

[Gustavo Noronha (kov)]

(In reply to comment #59)
> > * viewport-attributes-recompute-requested is emitted, giving the application the possibility of providing available size, and even override screen size if required
> 
> It seems quite good that adding the viewport-attributes-recompute-requested signal allows the user to override the screenWidth/height and availableWidth/height.
> However, according to the viewport meta-tag spec, the device-width/height keyword is used instead of the screen-width/height. So, I think that using the device-width/height would avoid confusion for the developers.

Sounds good to me =).

> 
> > * Qt uses a hardcoded DPI of 160, why is that?
> I'm not sure, but Nokia device may use the value.
> 
> Is it wrong to call webViewGetDPI() to set the deviceDPI value basically? 

I have no idea =(. I would follow Qt and change it later if required, in this case, I believe.

> > * www.engadget.com seems to work fine, except it has scrolling 100% of the time, probably because our calculation will have to take the space occupied by the scrollbars into account =(
> > * most sites apparently do some crazy stuff, providing very big width and height values, and a very small scale factor - it looks like using full content zoom alone is not enough!
> 
> Your patch related to GtkLauncher doesn't scale web pages if the web pages don't have viewport meta tags. So, do you mean that you tested the web sites, which have viewport meta tags?
> In case of apple.com, it has viewport meta tags, and works well.

Yes, I only tested sites with viewport meta tags. This one, for instance:

http://www.iui-js.org/samples/prefs.html

It doesn't work as I expected it would.

> > * full content zoom fails miserably at resizing text in http://touch.facebook.com/, which is not fun
> 
> Most mobile sites for the iPhone are not zoomed-in/out because user-scalable = 0 as follows,
> 
> <meta name="viewport" content="width=device-width; initial-scale=1.0; maximum-scale=1.0; user-scalable=0;" /> 

Right, I see, so that means for touch.facebook.com we should not zoom?

> > So, do you guys see any trouble with this API? What use cases are not covered? What was better in the previous approach?
> 
> We need to let the desktopWidth, deviceDPI also be overridden, because there would be some cases for the user to set these values manually.

Sure! Just need to make them properties, and they can be overriden in viewport-attributes-recompute-requested =)
 
> Thanks kov, your patch improved my patch so much.

\o/ hopefully we'll get this in soon now, feel free to make the changes you suggested (use device instead of screen for the name, and add desktopWidth/deviceDPI as properties), and I'll get it reviewed once more =D

(In reply to comment #60)
> > * Qt uses a hardcoded DPI of 160, why is that?
> 
> It is the dpi of the original iPhone and Android devices, so that it what web authors has been designing for.

I see. So perhaps we should go with it indeed.
 
> > * most sites apparently do some crazy stuff, providing very big width and height values, and a very small scale factor - it looks like using full content zoom alone is not enough!
> 
> Are you using ; in  the  metatag instead of , ? I had that issue on facebook etc. Google Android used to support ; but they say that they have removed that support.

This is the meta tag in the page I mentioned above:

<meta name="viewport" content="width=device-width; initial-scale=1.0; maximum-scale=1.0; user-scalable=0;"/>

So perhaps what is wrong is we should not use zoom at all because user-scalable is 0 (the viewport computation says we should scale to 0.25 for some reason!).

[Kenneth Rohde Christiansen]

> > Are you using ; in  the  metatag instead of , ? I had that issue on facebook etc. Google Android used to support ; but they say that they have removed that support.
> 
> This is the meta tag in the page I mentioned above:
> 
> <meta name="viewport" content="width=device-width; initial-scale=1.0; maximum-scale=1.0; user-scalable=0;"/>
> 
> So perhaps what is wrong is we should not use zoom at all because user-scalable is 0 (the viewport computation says we should scale to 0.25 for some reason!).

The parser will barf on this one and misinterpret it. There is a patch in bugzilla somewhere having a patch adding support for ;. You can try testing with that, or give your input on whether we need to support ; or not.

iPhone apparently also does not support ;, which is supported by most Android phones out there.

[Andreas Kling]

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

[Joone Hur]

Created attachment 74400 [details]
Proposed patch11

I updated the patch to let the desktopWidth, deviceDPI also be overridden.

[Gustavo Noronha (kov)]

(In reply to comment #62)
> The parser will barf on this one and misinterpret it. There is a patch in bugzilla somewhere having a patch adding support for ;. You can try testing with that, or give your input on whether we need to support ; or not.
> 
> iPhone apparently also does not support ;, which is supported by most Android phones out there.

This tag was in a "iphone UI" html/css/js library, so it's surprising it is using something that iphone would not handle =(. I have no strong opinion about whether we should support this, I guess that's up to the standards masters =)

[Gustavo Noronha (kov)]

Comment on attachment 74400 [details]
Proposed patch11

View in context: https://bugs.webkit.org/attachment.cgi?id=74400&action=review

I believe this is good to go, now. Let's just fix the Since tags, and see if we can test it using the signal instead of the new DRTSupport function.

> WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:91
> +// For testing fast/viewport.
> +void DumpRenderTreeSupportGtk::computeViewportAttributesForSize(WebKitWebView* webView, gint availableWidth, gint availableHeight)
> +{
> +    g_return_if_fail(WEBKIT_IS_WEB_VIEW(webView));
> +
> +    ViewportArguments arguments = webView->priv->corePage->mainFrame()->document()->viewportArguments();
> +    // desktopWidth = 980, deviceWidth = 320, deviceHeight = 480, deviceDPI = 160
> +    ViewportAttributes attrs = computeViewportAttributes(arguments, 980, 320, 480, 160, IntSize(availableWidth, availableHeight));
> +
> +    fprintf(stdout, "viewport size %dx%d scale %f with limits [%f, %f]\n", attrs.layoutSize.width(), attrs.layoutSize.height(), attrs.initialScale, attrs.minimumScale, attrs.maximumScale);
> +}

With the API we have now, all of the values used in this call can be overriden by handling the signal, can't they? If that is the case, I think it would be much more interesting to use the signal for the test =)

> WebKit/gtk/webkit/webkitwebview.cpp:2913
> +    * Since: 1.3.5

All of these should now be 1.3.8 =)

[Joone Hur]

Created attachment 75718 [details]
P

[Joone Hur]

Created attachment 75721 [details]
Proposed Patch12

We can't use the new signals instead of the new DRTSupport function, because the signals are already emitted before we set the available width/height in each test case of the viewport meta tags, so I just updated new APIs version to 1.3.8. 

Finally, we need to get the API comments reviewed by Martin.

[Gustavo Noronha (kov)]

Comment on attachment 75721 [details]
Proposed Patch12

I'll say r+, and wait for Martin or Xan to take a look at the API/English and mark cq+ if it's OK.

[Martin Robinson]

Comment on attachment 75721 [details]
Proposed Patch12

View in context: https://bugs.webkit.org/attachment.cgi?id=75721&action=review

> WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:382
> +void DumpRenderTreeSupportGtk::computeViewportAttributesForSize(WebKitWebView* webView, gint availableWidth, gint availableHeight)

I think this should called dumpConfigurationViewport.

> WebKit/gtk/webkit/webkitviewportattributes.cpp:216
> +     * WebKitViewportAttributs:desktop-width:

Move the default below here and the comment into the documentation.

> WebKit/gtk/webkit/webkitviewportattributes.cpp:239
> +     * WebKitViewportAttributs:device-dpi:

Set the default here and also move the comments below into the documentation. Maybe clean up the line breaking a little.

> WebKit/gtk/webkit/webkitviewportattributes.cpp:266
> +     * The width of the viewport.
> +     * Before getting this property, you need to make sure that 
> +     * #WebKitViewportAttributes is valid.

Maybe clean up these line breaks a little.

> WebKit/gtk/webkit/webkitviewportattributes.cpp:427
> +static void webkit_viewport_attributes_get_property(GObject* object, guint prop_id, GValue* value, GParamSpec* pspec)

Expand the abbreviations in these variable names.

> WebKit/gtk/webkit/webkitviewportattributes.cpp:481
> +static void webkit_viewport_attributes_set_property(GObject* object, guint prop_id, const GValue* value, GParamSpec *pspec)

Same here and move the asterisk.

> WebKit/gtk/webkit/webkitviewportattributes.cpp:527
> +    // This value works well for most web pages designed for desktop browsers.
> +    priv->desktopWidth = 980;
> +    // It is the dpi of the original iPhone and Android devices.
> +    priv->deviceDPI = 160;

Change these into default property variables.

> WebKit/gtk/webkit/webkitviewportattributes.cpp:550
> +    // Now let the application know it is safe to use the new
> +    // values.

This can be one line.

> WebKit/gtk/webkit/webkitwebview.cpp:-176
> -

I think you can remove this line deletion.

> WebKit/gtk/webkit/webkitwebview.cpp:2737
> +     * The #WebKitWebView::viewport-attributes-recompute-requested
> +     * signal will be emitted whenever recomputing the values of the
> +     * viewport attributes is required. That may happen because the
> +     * viewport meta tag of a page has changed, or it may happen as a
> +     * consequence of an explicit browser request (caused by screen
> +     * rotation, resizing, or similar reasons).

The #WebKitWebView::viewport-attributes-recompute-requested signal will be emitted when a page with a viewport metatag loads and when webkit_recompute_viewport_attributes is called.

> WebKit/gtk/webkit/webkitwebview.cpp:2753
> +            NULL, NULL,

These should be 0's.

> WebKit/gtk/webkit/webkitwebview.cpp:2766
> +     * The #WebKitWebView::viewport-attributes-available will be emitted with @viewport_attributes
> +     * when the viewport attributes are updated in the case of loading web pages contain
> +     * the viewport properties and calling webkit_viewport_attributes_recompute.
> +     * @viewport_attributes is used to control the viewport layout to make a web page fit the device screen.

The #WebKitWebView::viewport-attributes-(changed or updated or recomputed) signal will be emitted after the emission of #WebKitWebView::viewport-attributes-recompute-requested and the subsequent viewport attribute recomputation. At this point, if the WebKitViewportAttributes are valid, the viewport attributes are available.

> WebKit/gtk/webkit/webkitwebview.cpp:2774
> +            NULL, NULL,

These should be 0's.

> WebKit/gtk/webkit/webkitwebview.cpp:3612
> + * #WebKitWebViewpor_Attributes object attached to it as soon as it is

#WebKitWebViewportAttributes

> WebKitTools/DumpRenderTree/LayoutTestController.h:288
> +    void configurationForViewport(int availableWidth, int availableHeight);

This should be called dumpConfigurationForViewport.

[Joone Hur]

Created attachment 75888 [details]
Proposed Patch13

I updated this patch with the changes commented by Martin.

[Joone Hur]

Created attachment 75919 [details]
Proposed Patch14

[WebKit Commit Bot]

Comment on attachment 75919 [details]
Proposed Patch14

Rejecting patch 75919 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'apply-attachment', '--force-clean', '--non-interactive', 75919]" exit_code: 2
Last 500 characters of output:
ntroller.cpp
patching file WebKitTools/DumpRenderTree/LayoutTestController.h
patching file WebKitTools/DumpRenderTree/gtk/LayoutTestControllerGtk.cpp
patching file WebKitTools/DumpRenderTree/mac/LayoutTestControllerMac.mm
patching file WebKitTools/DumpRenderTree/win/LayoutTestControllerWin.cpp
patching file WebKitTools/DumpRenderTree/wx/LayoutTestControllerWx.cpp

Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Martin Robinson', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/6909001

[Joone Hur]

Created attachment 76041 [details]
Proposed Patch15

Submitted the patch again due to the above reason.

[WebKit Commit Bot]

Comment on attachment 76041 [details]
Proposed Patch15

Clearing flags on attachment: 76041

Committed r73608: <http://trac.webkit.org/changeset/73608>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.