Bug 96480 - REGRESSION(r128270): It made fast/events/popup-blocking-timers.html flakey
Summary: REGRESSION(r128270): It made fast/events/popup-blocking-timers.html flakey
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: jochen
URL:
Keywords: Qt, QtTriaged
: 96517 (view as bug list)
Depends on:
Blocks: 79666 96475
  Show dependency treegraph
 
Reported: 2012-09-12 01:45 PDT by Csaba Osztrogonác
Modified: 2012-10-02 13:11 PDT (History)
8 users (show)

See Also:


Attachments
Patch (24.17 KB, patch)
2012-10-02 07:12 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch for landing (24.28 KB, patch)
2012-10-02 11:50 PDT, jochen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2012-09-12 01:45:34 PDT
r128270
--------

--- /home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/layout-test-results/fast/events/popup-blocking-timers-expected.txt
+++ /home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/layout-test-results/fast/events/popup-blocking-timers-actual.txt
@@ -1,16 +1,2 @@
-Click Here (6 times)
-Test calling window.open() directly. A popup should be allowed.
-PASS newWindow is non-null.
-Test calling window.open() with a 0 ms delay. A popup should be allowed.
-PASS newWindow is non-null.
-Test calling window.open() in a 100 ms interval. A popup should only be allowed on the first execution of the interval.
-PASS newWindow is non-null.
-Test calling window.open() in a 100 ms interval. A popup should only be allowed on the first execution of the interval.
-PASS newWindow is undefined.
-Test calling window.open() in a nested call to setTimeout(). A popup should not be allowed.
-PASS newWindow is undefined.
-Test calling window.open() with a 1000 ms delay. A popup should be allowed.
-PASS newWindow is non-null.
-Test calling window.open() with a 1001 ms delay. A popup should not be allowed.
-PASS newWindow is undefined.
+FAIL: Timed out waiting for notifyDone to be called
 

r128275
--------

--- /home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/layout-test-results/fast/events/popup-blocking-timers-expected.txt
+++ /home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/layout-test-results/fast/events/popup-blocking-timers-actual.txt
@@ -5,12 +5,12 @@
 PASS newWindow is non-null.
 Test calling window.open() in a 100 ms interval. A popup should only be allowed on the first execution of the interval.
 PASS newWindow is non-null.
+Test calling window.open() with a 1000 ms delay. A popup should be allowed.
+PASS newWindow is non-null.
+Test calling window.open() in a nested call to setTimeout(). A popup should not be allowed.
+PASS newWindow is undefined.
 Test calling window.open() in a 100 ms interval. A popup should only be allowed on the first execution of the interval.
 PASS newWindow is undefined.
-Test calling window.open() in a nested call to setTimeout(). A popup should not be allowed.
-PASS newWindow is undefined.
-Test calling window.open() with a 1000 ms delay. A popup should be allowed.
-PASS newWindow is non-null.
 Test calling window.open() with a 1001 ms delay. A popup should not be allowed.
 PASS newWindow is undefined.
Comment 1 Csaba Osztrogonác 2012-09-12 03:41:18 PDT
I skipped it - https://trac.webkit.org/changeset/128288
Please unskip it with the proper fix.
Comment 2 jochen 2012-09-17 04:42:21 PDT
(In reply to comment #1)
> I skipped it - https://trac.webkit.org/changeset/128288
> Please unskip it with the proper fix.

I checked the chromium test logs, and the test used to be flaky before (esp. this case when the ordering of events changes).

I could imagine that it is less flaky, if we split it up in six individual tests, wdyt?
Comment 3 Adam Barth 2012-09-17 11:23:44 PDT
Sure, we can give that a try.
Comment 4 jochen 2012-10-02 07:09:46 PDT
*** Bug 96517 has been marked as a duplicate of this bug. ***
Comment 5 jochen 2012-10-02 07:12:05 PDT
Created attachment 166686 [details]
Patch
Comment 6 Daniel Bates 2012-10-02 09:16:13 PDT
Comment on attachment 166686 [details]
Patch

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

Looks sane to me.

> LayoutTests/fast/events/popup-blocking-timers1.html:31
> +        function clickButton() {
> +            var button = document.getElementById("test");
> +            var buttonX = button.offsetLeft + button.offsetWidth / 2;
> +            var buttonY = button.offsetTop + button.offsetHeight / 2;
> +            if (window.eventSender) {
> +                eventSender.mouseMoveTo(buttonX, buttonY);
> +                eventSender.mouseDown();
> +                eventSender.mouseUp();
> +            }
> +        }        

I know that you derived this function from the function of the same name in the original test. I take it that it's necessary to use EventSender to actually simulate a mouse click of a person as opposed to calling click() on the HTML button element?

> LayoutTests/fast/events/popup-blocking-timers2.html:1
> +<head>

Unless you intend to test quirks mode, I suggest that we include a DOCTYPE, say <!DOCTYPE html>.

> LayoutTests/fast/events/popup-blocking-timers2.html:33
> +        function clickButton() {
> +            var button = document.getElementById("test");
> +            var buttonX = button.offsetLeft + button.offsetWidth / 2;
> +            var buttonY = button.offsetTop + button.offsetHeight / 2;
> +            if (window.eventSender) {
> +                eventSender.mouseMoveTo(buttonX, buttonY);
> +                eventSender.mouseDown();
> +                eventSender.mouseUp();
> +            }
> +        }        

Ditto.

> LayoutTests/fast/events/popup-blocking-timers3.html:41
> +        function clickButton() {
> +            var button = document.getElementById("test");
> +            var buttonX = button.offsetLeft + button.offsetWidth / 2;
> +            var buttonY = button.offsetTop + button.offsetHeight / 2;
> +            if (window.eventSender) {
> +                eventSender.mouseMoveTo(buttonX, buttonY);
> +                eventSender.mouseDown();
> +                eventSender.mouseUp();
> +            }
> +        }        

Ditto.

> LayoutTests/fast/events/popup-blocking-timers4.html:35
> +        function clickButton() {
> +            var button = document.getElementById("test");
> +            var buttonX = button.offsetLeft + button.offsetWidth / 2;
> +            var buttonY = button.offsetTop + button.offsetHeight / 2;
> +            if (window.eventSender) {
> +                eventSender.mouseMoveTo(buttonX, buttonY);
> +                eventSender.mouseDown();
> +                eventSender.mouseUp();
> +            }
> +        }        

Ditto.

> LayoutTests/fast/events/popup-blocking-timers5.html:36
> +        function clickButton() {
> +            var button = document.getElementById("test");
> +            var buttonX = button.offsetLeft + button.offsetWidth / 2;
> +            var buttonY = button.offsetTop + button.offsetHeight / 2;
> +            if (window.eventSender) {
> +                eventSender.mouseMoveTo(buttonX, buttonY);
> +                eventSender.mouseDown();
> +                eventSender.mouseUp();
> +            }
> +        }        

Ditto.

> LayoutTests/fast/events/popup-blocking-timers6.html:36
> +        function clickButton() {
> +            var button = document.getElementById("test");
> +            var buttonX = button.offsetLeft + button.offsetWidth / 2;
> +            var buttonY = button.offsetTop + button.offsetHeight / 2;
> +            if (window.eventSender) {
> +                eventSender.mouseMoveTo(buttonX, buttonY);
> +                eventSender.mouseDown();
> +                eventSender.mouseUp();
> +            }
> +        }        

Ditto.
Comment 7 Daniel Bates 2012-10-02 09:18:51 PDT
(In reply to comment #6)
> > LayoutTests/fast/events/popup-blocking-timers2.html:1
> > +<head>
> 
> Unless you intend to test quirks mode, I suggest that we include a DOCTYPE, say <!DOCTYPE html>.

Feel free to disregard this comment as the original test omitted a <!DOCTYPE> declaration and I don't anticipate that the result of this test will change in quirks mode.
Comment 8 jochen 2012-10-02 11:50:43 PDT
Created attachment 166724 [details]
Patch for landing
Comment 9 jochen 2012-10-02 11:51:39 PDT
(In reply to comment #6)
> (From update of attachment 166686 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166686&action=review
> 
> Looks sane to me.
> 
> > LayoutTests/fast/events/popup-blocking-timers1.html:31
> > +        function clickButton() {
> > +            var button = document.getElementById("test");
> > +            var buttonX = button.offsetLeft + button.offsetWidth / 2;
> > +            var buttonY = button.offsetTop + button.offsetHeight / 2;
> > +            if (window.eventSender) {
> > +                eventSender.mouseMoveTo(buttonX, buttonY);
> > +                eventSender.mouseDown();
> > +                eventSender.mouseUp();
> > +            }
> > +        }        
> 
> I know that you derived this function from the function of the same name in the original test. I take it that it's necessary to use EventSender to actually simulate a mouse click of a person as opposed to calling click() on the HTML button element?

The call to click() would not be counted as a user action, and therefore no popups would be allowed at all

> 
> > LayoutTests/fast/events/popup-blocking-timers2.html:1
> > +<head>
> 
> Unless you intend to test quirks mode, I suggest that we include a DOCTYPE, say <!DOCTYPE html>.

done
Comment 10 WebKit Review Bot 2012-10-02 13:10:59 PDT
Comment on attachment 166724 [details]
Patch for landing

Clearing flags on attachment: 166724

Committed r130200: <http://trac.webkit.org/changeset/130200>
Comment 11 WebKit Review Bot 2012-10-02 13:11:04 PDT
All reviewed patches have been landed.  Closing bug.