Bug 76708 - [Qt][WK2] REGRESSION(r105517): It made 49 tests timeout
Summary: [Qt][WK2] REGRESSION(r105517): It made 49 tests timeout
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Critical
Assignee: Jocelyn Turcotte
URL:
Keywords: Qt, QtTriaged
Depends on: 77302
Blocks: 76296
  Show dependency treegraph
 
Reported: 2012-01-20 09:05 PST by Csaba Osztrogonác
Modified: 2012-01-30 09:27 PST (History)
6 users (show)

See Also:


Attachments
Patch (14.45 KB, patch)
2012-01-24 08:32 PST, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Draft (12.08 KB, patch)
2012-01-24 18:09 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (20.97 KB, patch)
2012-01-26 08:45 PST, Jocelyn Turcotte
kenneth: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Csaba Osztrogonác 2012-01-20 09:09:35 PST
I painted the bot green not to leave it for all weekend: http://trac.webkit.org/changeset/105521

Could you check what happened and then fix? :)
Comment 2 Csaba Osztrogonác 2012-01-20 09:37:15 PST
And a typo fix landed in http://trac.webkit.org/changeset/105523
Comment 3 Csaba Osztrogonác 2012-01-23 08:28:08 PST
Noam, Jocelyn, could you check these timeouts?
Comment 4 Jocelyn Turcotte 2012-01-23 08:36:54 PST
Currently looking.
Comment 5 Jocelyn Turcotte 2012-01-24 08:32:53 PST
Created attachment 123741 [details]
Patch
Comment 6 Balazs Kelemen 2012-01-24 09:46:46 PST
Hm, I'm not against your change but don't you think it could be solved at test level or tool level? Maybe we could add feature of WTR to force rendering?
Comment 7 Jocelyn Turcotte 2012-01-24 10:05:08 PST
(In reply to comment #6)

Just adding a show() call to WrapperWindow also does the trick, but you end up will test windows all over the place and this isn't convenient.
There might be a way but since the Qt scene graph isn't ran at all unless a QQuickCanvas is shown, it isn't possible AFAIK.

An alternative might be to have a custom platform plugin that allows QWindow to wrap an offscreen surface instead, but we don't have anything like this yet so we need something else for now.
Comment 8 Noam Rosenthal 2012-01-24 18:09:20 PST
Created attachment 123867 [details]
Draft

This is more like what I had in mind, but I wasn't able to test it yet...
Comment 9 Jocelyn Turcotte 2012-01-25 05:35:53 PST
(In reply to comment #8)
> Created an attachment (id=123867) [details]
> Draft
> 

It looks less complex than I thought, and we would get pixel tests working at least for non-composited content. I'd prefer it to my patch if it works properly.
Comment 10 Simon Hausmann 2012-01-26 01:18:48 PST
Comment on attachment 123741 [details]
Patch

Yeah, Noam's solution is nice, rendering off-screen like that. Taking this patch out of the review queue then.
Comment 11 Balazs Kelemen 2012-01-26 01:41:27 PST
Comment on attachment 123867 [details]
Draft

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

Great approach!

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:167
> +    Q_Q(QQuickWebView);
> +    if (!shouldRenderToOffscreenBuffer()) {
> +        q->page()->update();
> +        return;
> +    }
> +
> +    // TODO: we can maintain a real pixmap here and use it for pixel tests. Right now this is used only for running the rendering code-path while running tests.
> +    QPixmap dummyPixmap(1, 1);
> +    QPainter painter(&dummyPixmap);
> +    q->page()->d->paint(&painter);

Why don't we use a more reasonable sized pixmap here (even when not runnning pixel tests)?
And I would put the dummy code path in the if so it's easier to understand that what is the normal and what is the special path.
Comment 12 Jocelyn Turcotte 2012-01-26 08:45:16 PST
Created attachment 124122 [details]
Patch
Comment 13 Kenneth Rohde Christiansen 2012-01-26 08:52:36 PST
Comment on attachment 124122 [details]
Patch

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

rs=me

> Source/WebKit2/ChangeLog:9
> +        to pass. WebkitTestRunner doesn't show it's wrapping QQuickView, which

its* ?

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:69
>      , m_navigatorQtObjectEnabled(false)
> +    , m_renderToOffscreenBuffer(false)
>  {

I guess we could use this for creating thumbnails etc in the future or for pixel tests?

> Tools/ChangeLog:10
> +        (WTR::PlatformWebView::PlatformWebView): Use software rendering of layers since the wrapping QQuickView isn't shown.

wrapped* ?

shown on screen? isn't visible?
Comment 14 Jocelyn Turcotte 2012-01-27 04:46:57 PST
Committed r106109: <http://trac.webkit.org/changeset/106109>
Comment 15 Csaba Osztrogonác 2012-01-29 15:25:47 PST
Reopen, because it was rolled out: http://trac.webkit.org/changeset/106199
Check the bot for details: http://build.webkit.sed.hu/builders/x86-32%20Linux%20Qt%20Release%20WebKit2?numbuilds=200
Comment 16 Jocelyn Turcotte 2012-01-30 09:14:53 PST
Comment on attachment 124122 [details]
Patch

Re-putting on the commit queue, the crashes weren't caused by this patch but by r106022.
Comment 17 WebKit Review Bot 2012-01-30 09:17:03 PST
Comment on attachment 124122 [details]
Patch

Rejecting attachment 124122 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
nt.cpp
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/WebKitTestRunner/qt/PlatformWebViewQt.cpp
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/platform/qt-wk2/Skipped
Hunk #1 FAILED at 460.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/qt-wk2/Skipped.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Kenneth Ro..." exit_code: 1

Full output: http://queues.webkit.org/results/11365656
Comment 18 Jocelyn Turcotte 2012-01-30 09:27:59 PST
Committed r106250: <http://trac.webkit.org/changeset/106250>