Bug 76708 – [Qt][WK2] REGRESSION(r105517): It made 49 tests timeout

• Component: New Bugs
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

See http://build.webkit.sed.hu/results/x86-32%20Linux%20Qt%20Release%20WebKit2/r105518%20%2818907%29/results.html for details

[Csaba Osztrogonác]

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? :)

[Csaba Osztrogonác]

And a typo fix landed in http://trac.webkit.org/changeset/105523

[Csaba Osztrogonác]

Noam, Jocelyn, could you check these timeouts?

[Jocelyn Turcotte]

Currently looking.

[Jocelyn Turcotte]

Created attachment 123741 [details]
Patch

[Balazs Kelemen]

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?

[Jocelyn Turcotte]

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

[Noam Rosenthal]

Created attachment 123867 [details]
Draft

This is more like what I had in mind, but I wasn't able to test it yet...

[Jocelyn Turcotte]

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

[Simon Hausmann]

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.

[Balazs Kelemen]

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.

[Jocelyn Turcotte]

Created attachment 124122 [details]
Patch

[Kenneth Rohde Christiansen]

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?

[Jocelyn Turcotte]

Committed r106109: <http://trac.webkit.org/changeset/106109>

[Csaba Osztrogonác]

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

[Jocelyn Turcotte]

Comment on attachment 124122 [details]
Patch

Re-putting on the commit queue, the crashes weren't caused by this patch but by r106022.

[WebKit Review Bot]

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

[Jocelyn Turcotte]

Committed r106250: <http://trac.webkit.org/changeset/106250>