Bug 76708

Summary: [Qt][WK2] REGRESSION(r105517): It made 49 tests timeout
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: New BugsAssignee: Jocelyn Turcotte <jturcotte>
Severity: Critical CC: jturcotte, kbalazs, noam, ossy, webkit.review.bot, zoltan
Priority: P1 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 77302    
Bug Blocks: 76296    
Description Flags
Patch kenneth: review+, webkit.review.bot: commit-queue-

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]
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]

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]

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]

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]
Comment 13 Kenneth Rohde Christiansen 2012-01-26 08:52:36 PST
Comment on attachment 124122 [details]

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


> 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]

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]

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