Bug 81143 – [Qt][WK2] Implement PageClient::isViewWindowActive()

• Component: WebKit Qt
• Status: RESOLVED INVALID

[Jesus Sanchez-Palencia]

Since now QQuickCanvas provides API for telling us whether it is active or not, we can implement PageClient::isViewWindowActive.

[Jesus Sanchez-Palencia]

Created attachment 131942 [details]
Patch

[Jesus Sanchez-Palencia]

(In reply to comment #1)
> Created an attachment (id=131942) [details]
> Patch

This patch has a small workaround due to a segfault in Qt that happens everytime we try QQuickCanvas::isActive() on focusOutEvents. The issue was already reported and a fix for it is being reviewed. After it lands on Qt I can fix this and remove the //FIXME.

[Kenneth Rohde Christiansen]

Comment on attachment 131942 [details]
Patch

why no url to the Qt bug?

[Jesus Sanchez-Palencia]

(In reply to comment #3)
> (From update of attachment 131942 [details])
> why no url to the Qt bug?

http://codereview.qt-project.org/#change,20129

Sorry :)

[Kenneth Rohde Christiansen]

In the code with the FIXME

[Jesus Sanchez-Palencia]

(In reply to comment #5)
> In the code with the FIXME

I can fix when landing. Anything else?

[Jesus Sanchez-Palencia]

Comment on attachment 131942 [details]
Patch

I will upload a new version now that the Qt Bug has been fixed. As soon as it goes through CI and we pick the new hash, I will land it. (after the r+ of course... :)

[Jesus Sanchez-Palencia]

Created attachment 132081 [details]
Patch

[Simon Hausmann]

Comment on attachment 132081 [details]
Patch

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

> Source/WebKit2/UIProcess/qt/QtPageClient.cpp:259
> +    return m_webView->canvas()->isActive();

What does it mean for web content if the view window is active? Should this perhaps be tied to the QQuickItem instead of the window?

[Jesus Sanchez-Palencia]

(In reply to comment #9)
> (From update of attachment 132081 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132081&action=review
> 
> > Source/WebKit2/UIProcess/qt/QtPageClient.cpp:259
> > +    return m_webView->canvas()->isActive();
> 
> What does it mean for web content if the view window is active? Should this perhaps be tied to the QQuickItem instead of the window?

The PageClient API seems to have been thought for returning the Window state (PageClient::isViewWindowActive) for WebPageProxy (see WebPageProxy::viewStateDidChange). I can see this being tied to the Page Visibility Spec (http://www.w3.org/TR/page-visibility/), for example, in which there are states related to the state of the window itself (QQuickCanvas, in our case) and not directly to the view one.

For instance, let's say we have multiple tabs (webviews) but only one is in foreground and, therefore, has focus. This one will considered active while the others will be considered not active even if their top level window (canvas()) still is. I believe that is why we differentiate these states in WebPageProxy as ViewIsFocused and ViewWindowIsActive (besides ViewIsVisible and ViewIsInWindow).

[Jesus Sanchez-Palencia]

(In reply to comment #10)
> The PageClient API seems to have been thought for returning the Window state (PageClient::isViewWindowActive) for WebPageProxy (see WebPageProxy::viewStateDidChange). I can see this being tied to the Page Visibility Spec (http://www.w3.org/TR/page-visibility/), for example, in which there are states related to the state of the window itself (QQuickCanvas, in our case) and not directly to the view one.

By the way, such work is being tracked by:

https://bugs.webkit.org/show_bug.cgi?id=81154
https://bugs.webkit.org/show_bug.cgi?id=81164
and https://bugs.webkit.org/show_bug.cgi?id=69554

[Jesus Sanchez-Palencia]

Created attachment 132903 [details]
Patch

[Jesus Sanchez-Palencia]

(In reply to comment #12)
> Created an attachment (id=132903) [details]
> Patch

Updated the patch due to a check that was missing.

[WebKit Review Bot]

Comment on attachment 132903 [details]
Patch

Clearing flags on attachment: 132903

Committed r111855: <http://trac.webkit.org/changeset/111855>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Reopen, because it broke 30+ tests - http://build.webkit.sed.hu/results/x86-32%20Linux%20Qt%20Release%20WebKit2/r111856%20%2821981%29/results.html

[Jesus Sanchez-Palencia]

(In reply to comment #16)
> Reopen, because it broke 30+ tests - http://build.webkit.sed.hu/results/x86-32%20Linux%20Qt%20Release%20WebKit2/r111856%20%2821981%29/results.html

I've found what caused the crashes and I'm now looking for a way to tweak the WebKitTestRunner and its WrapperWindow in our favor.

I believe the problem is not with the patch itself but with some missing pieces in our PlatformWebView. I will update this bug as soon as I find out the root cause.

[Jesus Sanchez-Palencia]

Created attachment 133579 [details]
Patch

[Jesus Sanchez-Palencia]

Comment on attachment 133579 [details]
Patch

Let's give it a try...

[WebKit Review Bot]

Comment on attachment 133579 [details]
Patch

Clearing flags on attachment: 133579

Committed r111993: <http://trac.webkit.org/changeset/111993>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Reopen, because it broke many tests, see https://bugs.webkit.org/show_bug.cgi?id=82167 for details.

[Csaba Osztrogonác]

Rollout landed in http://trac.webkit.org/changeset/112073 (rs=darktears)

[Jesus Sanchez-Palencia]

Comment on attachment 133579 [details]
Patch

We need to wait the next round of Qt5 updates on the bots before landing this.

[Kenneth Rohde Christiansen]

Comment on attachment 133579 [details]
Patch

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

> Tools/WebKitTestRunner/qt/PlatformWebViewQt.cpp:83
> +    // QWindow does not create the underlying platform
> +    // specific bits unless QWindow::setVisible() or QWindow::show() are called,
> +    // and without it QWindow::isActive() will always return false.
> +    // For these cases QWindow::create() is provided.

// QWindow delays creating of the underlying platform specific bits until ::setVisible()
// or ::show() are called. This makes ::isActive() always return false. In order to force
// the creation ::create() exists, so we use it here.

[Jesus Sanchez-Palencia]

Comment on attachment 133579 [details]
Patch

CQ has issues, I will land it manually.

[Jesus Sanchez-Palencia]

Committed r113714: <http://trac.webkit.org/changeset/113714>

[Csaba Osztrogonác]

Reopen, because it broke API tests:

FAIL!  : qmltests::DesktopWebViewLinkHovered::test_linkHovered() 'wait for signal linkHovered' returned FALSE. ()
   Loc: [/home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/Source/WebKit2/UIProcess/API/qt/tests/qmltests/DesktopBehavior/tst_linkHovered.qml(49)]
FAIL!  : qmltests::DesktopWebViewLinkHovered::test_linkHoveredDoesntEmitRepeated() 'wait for signal linkHovered' returned FALSE. ()
   Loc: [/home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/Source/WebKit2/UIProcess/API/qt/tests/qmltests/DesktopBehavior/tst_linkHovered.qml(68)]
FAIL!  : qmltests::DesktopWebViewLoadHtml::test_baseUrlAfterLoadHtml() 'wait for signal linkHovered' returned FALSE. ()
   Loc: [/home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/Source/WebKit2/UIProcess/API/qt/tests/qmltests/DesktopBehavior/tst_loadHtml.qml(48)]
   

Could you check and fix it please?

[Csaba Osztrogonác]

Additionally it broke a layout test:
--- /ramdisk/qt-linux-32-release-webkit2/build/layout-test-results/fast/events/mouseout-on-window-expected.txt 
+++ /ramdisk/qt-linux-32-release-webkit2/build/layout-test-results/fast/events/mouseout-on-window-actual.txt 
@@ -1,2 +1,2 @@
 This test ensures that mouse out events are sent to the window.
-PASS: Received mouseout event.
+FAIL: No mouseout was sent.

[Csaba Osztrogonác]

And it made 24 new flakey test :(

[Jesus Sanchez-Palencia]

Wow! Ok, I will have a look at it now. I will roll it out if I need more time, Ossy.  Thanks!

[Jesus Sanchez-Palencia]

(In reply to comment #28)
> Reopen, because it broke API tests:
>
> Could you check and fix it please?

So, Ossy, I was debugging it with Alexis and we realized that the problem is happening because we run the tests in parallel. If you run "run-qtwebkit-tests -j1" you will see that the tests doesn't fail. 

Our belief is that running tests in parallel that depend on their window state is a potential mistake, which has now been flagged because this is exactly what this patch fixes.


(In reply to comment #29)
> Additionally it broke a layout test:

This I can't reproduce this locally. =/

[Csaba Osztrogonác]

(In reply to comment #32)
> So, Ossy, I was debugging it with Alexis and we realized that the problem is happening because we run the tests in parallel. If you run "run-qtwebkit-tests -j1" you will see that the tests doesn't fail. 
> 
> Our belief is that running tests in parallel that depend on their window state is a potential mistake, which has now been flagged because this is exactly what this patch fixes.

I tried it locally, but I got same results as the bot results.
Did you run the tests in XFVB as the bots do?

xvfb-run -a --server-args="-screen 0 1024x768x24" python ./Tools/Scripts/run-qtwebkit-tests --output-file=qt-unit-tests.html --do-not-open-results --timeout=120 WebKitBuild/Release/Source/WebKit2/UIProcess/API/qt/tests/

I got same results with -j1 and without -j1 ...

> (In reply to comment #29)
> > Additionally it broke a layout test:
> 
> This I can't reproduce this locally. =/

It's hard to believe. :( Did you run all tests with Tools/Scripts/run-webkit-tests on the reference platform? (Ubuntu 11.10)
If you do it, fast/events/mouseout-on-window fails and there are
24 new flakey tests after your patch.

[Jesus Sanchez-Palencia]

(In reply to comment #33)
> I tried it locally, but I got same results as the bot results.
> Did you run the tests in XFVB as the bots do?

Nope.

> 
> > (In reply to comment #29)
> > > Additionally it broke a layout test:
> > 
> > This I can't reproduce this locally. =/
> 
> It's hard to believe. :( Did you run all tests with Tools/Scripts/run-webkit-tests on the reference platform? (Ubuntu 11.10)
> If you do it, fast/events/mouseout-on-window fails and there are
> 24 new flakey tests after your patch.

Hard to believe?!
It was tested on two different machines (api tests with -j1 and this layout test specifically) but none of them use Ubuntu. Plus, a fast/event test that is called "mouseout-on-window" shouldn't have any platform-specific issues, right?

Anyway, I'm having a deeper look in order to find a workaround for it somehow.

[Jesus Sanchez-Palencia]

(In reply to comment #34)
> It was tested on two different machines (api tests with -j1 and this layout test specifically) but none of them use Ubuntu. Plus, a fast/event test that is called "mouseout-on-window" shouldn't have any platform-specific issues, right?

By "platform-specific" here I meant "distro-specific".

[Csaba Osztrogonác]

Here is the new flakey tests after this patch:
-----------------------------------------------
  editing/pasteboard/copy-crash.html = TEXT PASS
  fast/block/positioning/absolute-appended-to-inline.html = TEXT PASS
  fast/css/hover-affects-child.html = TEXT PASS
  fast/css/pseudo-any.html = TEXT PASS
  fast/dom/shadow/shadow-boundary-crossing.html = TEXT PASS
  fast/dom/shadow/shadow-contents-event.html = TEXT PASS
  fast/events/dont-loose-last-event.html = TEXT PASS
  fast/events/event-sender-mouse-moved.html = TEXT PASS
  fast/events/iframe-onmousemove.html = TEXT PASS
  fast/events/mouse-moved-remove-frame-crash.html = TEXT PASS
  fast/events/mousemove-after-drag-over-scrollbar.html = TEXT PASS
  fast/events/mouseout-dead-node.html = TEXT PASS
  fast/events/mouseout-dead-subframe.html = TEXT PASS
  fast/events/mouseover-mouseout.html = TEXT PASS
  fast/events/mouseover-mouseout2.html = TEXT PASS
  fast/events/touch/touch-before-pressing-spin-button.html = TEXT PASS
  fast/forms/autofocus-input-css-style-change.html = TEXT PASS
  fast/forms/input-step-as-double.html = TEXT PASS
  fast/forms/number/spin-button-events.html = TEXT PASS
  fast/forms/number/spin-button-state.html = TEXT PASS
  fast/forms/number/spin-in-multi-column.html = TEXT PASS
  fast/forms/range/slider-mouse-events.html = TEXT PASS
  fast/frames/flattening/iframe-flattening-fixed-width.html = TEXT PASS
  fast/overflow/overflow-focus-ring.html = TEXT PASS
  fast/replaced/image-map-2.html = TEXT PASS
  scrollbars/overflow-custom-scrollbar-crash.html = TEXT PASS
  scrollbars/scrollbar-iframe-click-does-not-blur-content.html = TEXT PASS
  svg/custom/foreign-object-skew.svg = TEXT PASS
  svg/custom/js-update-container2.svg = TEXT PASS
  svg/custom/pointer-events-invalid-fill.svg = TEXT PASS
  svg/custom/use-instanceRoot-as-event-target.xhtml = TEXT PASS
  svg/zoom/page/zoom-foreignObject.svg = TEXT PASS
  svg/zoom/text/zoom-foreignObject.svg = TEXT PASS

And the new failing test:
--------------------------
  fast/css/hover-update.html = TEXT
  fast/css/nested-layers-with-hover.html = TEXT
  fast/events/mouseout-on-window.html = TEXT

[Csaba Osztrogonác]

(In reply to comment #34)
> > > This I can't reproduce this locally. =/
> > 
> > It's hard to believe. :( Did you run all tests with Tools/Scripts/run-webkit-tests on the reference platform? (Ubuntu 11.10)
> > If you do it, fast/events/mouseout-on-window fails and there are
> > 24 new flakey tests after your patch.
> 
> Hard to believe?!
> It was tested on two different machines (api tests with -j1 and this layout test specifically) but none of them use Ubuntu. Plus, a fast/event test that is called "mouseout-on-window" shouldn't have any platform-specific issues, right?

I think it is the main problem. All developers should run tests on the _reference_ platform. And at least you should have at least one Ubuntu
to be able reproduce QtWebKit bugs occured on the buildbots.
 
> Anyway, I'm having a deeper look in order to find a workaround for it somehow.
Thanks for taking care.

To be clarify I won't roll out QtWebKit patches, obviously buggy patches, 
I don't wan't to be the grumpy bad guy who blocks the rapid developing.
But in this case our Qt5-WK2 bots aren't in a good shape. And it's hard 
to catch new flakey and new failing tests until it is fixed. :-/ But
if it's OK for you guys let's leave the bot in this bad shape.

[Csaba Osztrogonác]

I modified our master.cfg to run API tests with -j1. The runtime was 
increased from 0.5 minutes to 2.5 minutes, but tests still fail.
It's strange, because it worked for me locally in the same environment.
But layout tests might break something in the environment and it might
break API tests ... I have no idea what happens ...

[Jesus Sanchez-Palencia]

(In reply to comment #38)
> I modified our master.cfg to run API tests with -j1. The runtime was 
> increased from 0.5 minutes to 2.5 minutes, but tests still fail.
> It's strange, because it worked for me locally in the same environment.
> But layout tests might break something in the environment and it might
> break API tests ... I have no idea what happens ...

I will be investigating the issues today and I will update the bug with new comments.

Thanks!

[Jesus Sanchez-Palencia]

(In reply to comment #39)
> I will be investigating the issues today and I will update the bug with new comments.

An issue with my xvfb version and nvidia driver is preventing to run this as the bots do. I will need a bit more time to investigate and fix this. =/

[Csaba Osztrogonác]

(In reply to comment #38)
> I modified our master.cfg to run API tests with -j1. The runtime was 
> increased from 0.5 minutes to 2.5 minutes, but tests still fail.
> It's strange, because it worked for me locally in the same environment.
> But layout tests might break something in the environment and it might
> break API tests ... I have no idea what happens ...

After -j1 we got very strange results on the 32 bit Qt5-WK2 bot:
31 passed, 3 failed, 0 skipped, 3 crashed

http://build.webkit.sed.hu/builders/x86-32%20Linux%20Qt%20Release%20WebKit2/builds/22770/steps/API%20tests/logs/stdio

CRASHED: WebKitBuild/Release/Source/WebKit2/UIProcess/API/qt/tests/qmltests/tst_qmltests_WebView
CRASHED: WebKitBuild/Release/Source/WebKit2/UIProcess/API/qt/tests/qquickwebview/tst_qquickwebview
CRASHED: WebKitBuild/Release/Source/WebKit2/UIProcess/API/qt/tests/publicapi/tst_publicapi

Great! :(

[Jesus Sanchez-Palencia]

(In reply to comment #41)
> Great! :(

So remove the -j1 for the time being. But don't you think that not running the tests in parallel shouldn't be causing crashes?!!

As I said, Xvfb is not working properly with my nvidia card on arch linux. I'm fixing this and setting up a virtual machine with the "reference platform" so I can debug the issues. Before that there is nothing I can do. If the patch is causing too much trouble on the bots, just roll it out as well and it will come back later when we all have understand what is going on with it. I would do it myself if I had X running right now. (and I would try -j1 without this patch just to see how the bots would behave as well)

Other than that, if you really want to help, try to understand what we have explained on irc and on this bug report and help us to debug it as well. There is something way too wrong and not only with the patch if even you said that you are getting different behavior on the bots and on your machine, which is running the "reference platform".

thanks!

[Csaba Osztrogonác]

(In reply to comment #42)
> (In reply to comment #41)
> > Great! :(
> 
> So remove the -j1 for the time being. But don't you think that not running the tests in parallel shouldn't be causing crashes?!!

I removed -j1 and now the runtime is 27 sec again and there isn't any crashes. Very strange.
 
> As I said, Xvfb is not working properly with my nvidia card on arch linux. I'm fixing this and setting up a virtual machine with the "reference platform" so I can debug the issues. Before that there is nothing I can do. If the patch is causing too much trouble on the bots, just roll it out as well and it will come back later when we all have understand what is going on with it. I would do it myself if I had X running right now. (and I would try -j1 without this patch just to see how the bots would behave as well)
> 
> Other than that, if you really want to help, try to understand what we have explained on irc and on this bug report and help us to debug it as well. There is something way too wrong and not only with the patch if even you said that you are getting different behavior on the bots and on your machine, which is running the "reference platform".

I'm terrible sorry, but I don't have too much free time for debugging when
I'm filing bugs about new regressions, rebase expected files instead of
authors, digging which patch broke what in 24/7. :(

I agree, something is really wrong with run-qtwebkit-tests script near -j1 option ... I'll check it soon with and without your patch.

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.