Bug 120404 - [GTK] [WK2] TestWebKitWebView snapshot fails
Summary: [GTK] [WK2] TestWebKitWebView snapshot fails
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brian Holt
URL:
Keywords:
Depends on: 121162
Blocks: 117689
  Show dependency treegraph
 
Reported: 2013-08-28 05:52 PDT by Brian Holt
Modified: 2015-01-19 05:34 PST (History)
7 users (show)

See Also:


Attachments
Patch (1.81 KB, patch)
2013-08-30 09:56 PDT, Brian Holt
no flags Details | Formatted Diff | Diff
Patch (8.55 KB, patch)
2015-01-19 04:26 PST, Carlos Garcia Campos
zan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Holt 2013-08-28 05:52:42 PDT
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)
Comment 1 Brian Holt 2013-08-30 01:46:44 PDT
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?
Comment 2 Carlos Garcia Campos 2013-08-30 04:51:56 PDT
You can try resizing the window instead of the view.
Comment 3 Zan Dobersek 2013-08-30 05:12:57 PDT
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?
Comment 4 Claudio Saavedra 2013-08-30 06:47:13 PDT
(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.
Comment 5 Brian Holt 2013-08-30 09:34:43 PDT
(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.
Comment 6 Brian Holt 2013-08-30 09:48:13 PDT
(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...
Comment 7 Brian Holt 2013-08-30 09:56:17 PDT
Created attachment 210125 [details]
Patch
Comment 8 WebKit Commit Bot 2013-08-30 09:57:22 PDT
Comment on attachment 210125 [details]
Patch

Rejecting attachment 210125 [details] from commit-queue.

brian.holt@samsung.com 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.
Comment 9 Brian Holt 2013-08-30 09:58:17 PDT
(In reply to comment #8)
> (From update of attachment 210125 [details])
> Rejecting attachment 210125 [details] from commit-queue.
> 
> brian.holt@samsung.com 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.
Comment 10 WebKit Commit Bot 2013-08-30 09:58:36 PDT
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
Comment 11 Darin Adler 2013-08-30 09:59:16 PDT
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.
Comment 12 Claudio Saavedra 2013-08-30 10:25:40 PDT
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.
Comment 13 WebKit Commit Bot 2013-08-30 10:59:34 PDT
Comment on attachment 210125 [details]
Patch

Clearing flags on attachment: 210125

Committed r154899: <http://trac.webkit.org/changeset/154899>
Comment 14 WebKit Commit Bot 2013-08-30 10:59:37 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Carlos Garcia Campos 2013-09-10 01:46:00 PDT
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.
Comment 16 Brian Holt 2013-09-11 08:45:39 PDT
(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.
Comment 17 WebKit Commit Bot 2013-09-11 08:48:06 PDT
Re-opened since this is blocked by bug 121162
Comment 18 Carlos Garcia Campos 2015-01-19 04:14:45 PST
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
Comment 19 Carlos Garcia Campos 2015-01-19 04:26:02 PST
Created attachment 244894 [details]
Patch
Comment 20 Carlos Garcia Campos 2015-01-19 05:34:03 PST
Committed r178644: <http://trac.webkit.org/changeset/178644>