Bug 81143 - [Qt][WK2] Implement PageClient::isViewWindowActive()
Summary: [Qt][WK2] Implement PageClient::isViewWindowActive()
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jesus Sanchez-Palencia
URL:
Keywords: Qt, QtTriaged
Depends on: 82053 82167 82184 83887
Blocks: 70236
  Show dependency treegraph
 
Reported: 2012-03-14 12:34 PDT by Jesus Sanchez-Palencia
Modified: 2014-02-03 03:20 PST (History)
6 users (show)

See Also:


Attachments
Patch (4.01 KB, patch)
2012-03-14 15:41 PDT, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
Patch (1.67 KB, patch)
2012-03-15 11:18 PDT, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
Patch (1.71 KB, patch)
2012-03-20 14:53 PDT, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
Patch (3.39 KB, patch)
2012-03-23 16:03 PDT, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jesus Sanchez-Palencia 2012-03-14 12:34:00 PDT
Since now QQuickCanvas provides API for telling us whether it is active or not, we can implement PageClient::isViewWindowActive.
Comment 1 Jesus Sanchez-Palencia 2012-03-14 15:41:53 PDT
Created attachment 131942 [details]
Patch
Comment 2 Jesus Sanchez-Palencia 2012-03-14 15:45:29 PDT
(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.
Comment 3 Kenneth Rohde Christiansen 2012-03-15 02:55:09 PDT
Comment on attachment 131942 [details]
Patch

why no url to the Qt bug?
Comment 4 Jesus Sanchez-Palencia 2012-03-15 06:28:55 PDT
(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 :)
Comment 5 Kenneth Rohde Christiansen 2012-03-15 06:38:53 PDT
In the code with the FIXME
Comment 6 Jesus Sanchez-Palencia 2012-03-15 06:45:03 PDT
(In reply to comment #5)
> In the code with the FIXME

I can fix when landing. Anything else?
Comment 7 Jesus Sanchez-Palencia 2012-03-15 10:43:51 PDT
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... :)
Comment 8 Jesus Sanchez-Palencia 2012-03-15 11:18:15 PDT
Created attachment 132081 [details]
Patch
Comment 9 Simon Hausmann 2012-03-15 13:38:22 PDT
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?
Comment 10 Jesus Sanchez-Palencia 2012-03-15 14:04:12 PDT
(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).
Comment 11 Jesus Sanchez-Palencia 2012-03-15 14:05:15 PDT
(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
Comment 12 Jesus Sanchez-Palencia 2012-03-20 14:53:12 PDT
Created attachment 132903 [details]
Patch
Comment 13 Jesus Sanchez-Palencia 2012-03-20 14:55:46 PDT
(In reply to comment #12)
> Created an attachment (id=132903) [details]
> Patch

Updated the patch due to a check that was missing.
Comment 14 WebKit Review Bot 2012-03-23 06:27:22 PDT
Comment on attachment 132903 [details]
Patch

Clearing flags on attachment: 132903

Committed r111855: <http://trac.webkit.org/changeset/111855>
Comment 15 WebKit Review Bot 2012-03-23 06:27:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Csaba Osztrogonác 2012-03-23 07:10:32 PDT
Reopen, because it broke 30+ tests - http://build.webkit.sed.hu/results/x86-32%20Linux%20Qt%20Release%20WebKit2/r111856%20%2821981%29/results.html
Comment 17 Jesus Sanchez-Palencia 2012-03-23 14:10:57 PDT
(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.
Comment 18 Jesus Sanchez-Palencia 2012-03-23 16:03:05 PDT
Created attachment 133579 [details]
Patch
Comment 19 Jesus Sanchez-Palencia 2012-03-24 08:17:30 PDT
Comment on attachment 133579 [details]
Patch

Let's give it a try...
Comment 20 WebKit Review Bot 2012-03-24 08:24:10 PDT
Comment on attachment 133579 [details]
Patch

Clearing flags on attachment: 133579

Committed r111993: <http://trac.webkit.org/changeset/111993>
Comment 21 WebKit Review Bot 2012-03-24 08:24:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Csaba Osztrogonác 2012-03-25 22:23:35 PDT
Reopen, because it broke many tests, see https://bugs.webkit.org/show_bug.cgi?id=82167 for details.
Comment 23 Csaba Osztrogonác 2012-03-26 04:08:14 PDT
Rollout landed in http://trac.webkit.org/changeset/112073 (rs=darktears)
Comment 24 Jesus Sanchez-Palencia 2012-03-26 05:54:27 PDT
Comment on attachment 133579 [details]
Patch

We need to wait the next round of Qt5 updates on the bots before landing this.
Comment 25 Kenneth Rohde Christiansen 2012-04-10 07:03:14 PDT
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.
Comment 26 Jesus Sanchez-Palencia 2012-04-10 07:07:32 PDT
Comment on attachment 133579 [details]
Patch

CQ has issues, I will land it manually.
Comment 27 Jesus Sanchez-Palencia 2012-04-10 08:15:40 PDT
Committed r113714: <http://trac.webkit.org/changeset/113714>
Comment 28 Csaba Osztrogonác 2012-04-10 08:55:12 PDT
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?
Comment 29 Csaba Osztrogonác 2012-04-10 09:05:22 PDT
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.
Comment 30 Csaba Osztrogonác 2012-04-10 09:18:39 PDT
And it made 24 new flakey test :(
Comment 31 Jesus Sanchez-Palencia 2012-04-10 10:39:38 PDT
Wow! Ok, I will have a look at it now. I will roll it out if I need more time, Ossy.  Thanks!
Comment 32 Jesus Sanchez-Palencia 2012-04-10 12:13:28 PDT
(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. =/
Comment 33 Csaba Osztrogonác 2012-04-10 12:55:48 PDT
(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.
Comment 34 Jesus Sanchez-Palencia 2012-04-10 13:43:17 PDT
(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.
Comment 35 Jesus Sanchez-Palencia 2012-04-10 14:11:51 PDT
(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".
Comment 36 Csaba Osztrogonác 2012-04-11 08:54:04 PDT
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
Comment 37 Csaba Osztrogonác 2012-04-11 09:00:05 PDT
(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.
Comment 38 Csaba Osztrogonác 2012-04-11 09:02:45 PDT
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 ...
Comment 39 Jesus Sanchez-Palencia 2012-04-11 10:35:36 PDT
(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!
Comment 40 Jesus Sanchez-Palencia 2012-04-11 16:10:27 PDT
(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. =/
Comment 41 Csaba Osztrogonác 2012-04-12 06:47:16 PDT
(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! :(
Comment 42 Jesus Sanchez-Palencia 2012-04-12 07:05:46 PDT
(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!
Comment 43 Csaba Osztrogonác 2012-04-12 08:30:27 PDT
(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.
Comment 44 Jocelyn Turcotte 2014-02-03 03:20:22 PST
=== 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.