Bug 120404 – [GTK] [WK2] TestWebKitWebView snapshot fails

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Brian Holt]

The unit test for snapshots fails on the expected size of the snapshot.

ERROR:../../Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:553:void testWebViewSnapshot(SnapshotWebViewTest*, gconstpointer): assertion failed (width <= 50): (200 <= 50)

[Brian Holt]

The problem is that gtk_widget_size_allocate() no longer resizes the widget in WebViewTest::resizeView().

Its not clear to me what the right function is, or if its even possible to resize a widget that's not a window.

Options now are
1) find a function that resizes a widget 
2) disable the test
3) rewrite the test to avoid resizing (although Claudio felt that this was necessary to test the functionality)

What is the best course of action here?

[Carlos Garcia Campos]

You can try resizing the window instead of the view.

[Zan Dobersek]

GTK's DumpRenderTree works around the resizing issues by spinning the main loop while the events are pending, simply because resizing is done asynchronously.
http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/gtk/DumpRenderTree.cpp#L622

Could you give that a try?

[Claudio Saavedra]

(In reply to comment #3)
> GTK's DumpRenderTree works around the resizing issues by spinning the main loop while the events are pending, simply because resizing is done asynchronously.
> http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/gtk/DumpRenderTree.cpp#L622
> 
> Could you give that a try?

That probably needs to be added to the resizeView() method (or whatever is called) in WebViewTest.

[Brian Holt]

(In reply to comment #3)
> GTK's DumpRenderTree works around the resizing issues by spinning the main loop while the events are pending, simply because resizing is done asynchronously.
> http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/gtk/DumpRenderTree.cpp#L622
> 
> Could you give that a try?

Just tried that -> it successfully pumps all the events but the webview doesn't get resized.

[Brian Holt]

(In reply to comment #2)
> You can try resizing the window instead of the view.

That solves it, but only when used with the event pump! Thanks everyone for the help, patch coming soon...

[Brian Holt]

Created attachment 210125 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 210125 [details]
Patch

Rejecting attachment 210125 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Brian Holt]

(In reply to comment #8)
> (From update of attachment 210125 [details])
> Rejecting attachment 210125 [details] from commit-queue.
> 
> [e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.
> 
> - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.
> 
> - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

Sorry, that was a mistake -> wanted to set the cq? flag.

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Darin Adler]

Comment on attachment 210125 [details]
Patch

While I’m not a GTK expert, I feel I know enough to OK this, especially given that it’s in the test code.

[Claudio Saavedra]

If we don't understand what exactly changed in WK that requires this method to be changed, it seems to me that this is more a workaround than an actual fix.

[WebKit Commit Bot]

Comment on attachment 210125 [details]
Patch

Clearing flags on attachment: 210125

Committed r154899: <http://trac.webkit.org/changeset/154899>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Garcia Campos]

Comment on attachment 210125 [details]
Patch

Sorry to be late here (again), but this is not correct. WebViewTest::resizeView() might be used by tests not using a window, so we can't assume m_parentWindow is non-NULL pointer. This also broke the inspector unit tests here that also use resizeView, but I don't know exactly why, because inspector test uses a window.

[Brian Holt]

(In reply to comment #15)
> (From update of attachment 210125 [details])
> Sorry to be late here (again), but this is not correct. WebViewTest::resizeView() might be used by tests not using a window, so we can't assume m_parentWindow is non-NULL pointer. This also broke the inspector unit tests here that also use resizeView, but I don't know exactly why, because inspector test uses a window.

After discussion with Carlos on IRC it seems we should roll this out.

It looks like the patch has exactly the same behaviour as the previous code
 the snapshot test fails first time under xvfb for both old and new code, but passes on subsequent runs, whereas the test passes every time under X for both old and new.  So its an Xvfb issue and this patch doesn't resolve it.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 121162

[Carlos Garcia Campos]

There are at least problems here:

 1.- Resizing the web view works, but since we are using a size smaller than the window size, the web view is then resized again to fill the parent container. That's why we need to resize the window instead.

 2.- When the WebView is resized, a message is sent to the WebProcess, but since we use injected bundle message to implement snapshots, it can happen that the GetSnapshot message is received before the resize one, and we create the snapshot with the previous size. We should make sure the web view is mapped in the window at the desired size.


I also think we should use fixed values for the document size and visible area, setting the doc size using CSS and disabling the scrollbars in CSS as well. I'll rework the test, it has been skipped for too long

[Carlos Garcia Campos]

Created attachment 244894 [details]
Patch

[Carlos Garcia Campos]

Committed r178644: <http://trac.webkit.org/changeset/178644>