Bug 71693 - [Qt] http/tests/notifications tests make fast/notifications/notifications-click-event.html fail
Summary: [Qt] http/tests/notifications tests make fast/notifications/notifications-cli...
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Kristóf Kosztyó
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 64002
  Show dependency treegraph
 
Reported: 2011-11-07 08:32 PST by Csaba Osztrogonác
Modified: 2013-11-15 05:02 PST (History)
5 users (show)

See Also:


Attachments
proposed fix (6.26 KB, patch)
2012-01-04 04:16 PST, Kristóf Kosztyó
hausmann: review+
webkit.review.bot: commit-queue-
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 2011-11-07 08:32:35 PST
You can easily reproduce this fail:
$Tools/Scripts/old-run-webkit-tests http/tests/notifications/icon-does-not-exist.html fast/notifications/notifications-click-event.html

fail:
Comment 1 Csaba Osztrogonác 2011-11-07 08:36:33 PST
fail:
--- /tmp/layout-test-results/fast/notifications/notifications-click-event-expected.txt	2011-11-07 16:35:36.762379664 +0000
+++ /tmp/layout-test-results/fast/notifications/notifications-click-event-actual.txt	2011-11-07 16:35:36.762379664 +0000
@@ -1,8 +1,6 @@
 DESKTOP NOTIFICATION: icon , title New E-mail, text Meet me tonight at 8!
-DESKTOP NOTIFICATION CLOSED: New E-mail
 Showing notifications.
 
 To exercise manually, grant notification permissions and load this page, then click on the notification. You should see a "PASS" message.
 
-PASS: click event fired.
Comment 2 Csaba Osztrogonác 2011-12-01 06:01:40 PST
One more culprit test which makes the other fail:

$ Tools/Scripts/old-run-webkit-tests --wait-for-httpd http/tests/notifications/icon-exists-show-alert-during-load.html fast/notifications/notifications-click-event.html
Comment 3 Kristóf Kosztyó 2012-01-04 04:16:54 PST
Created attachment 121094 [details]
proposed fix

This patch clear the m_notifications in the resetToConsistentStateBeforeTesting() so the notification tests don't cause side effect in the next notification test
Comment 4 Simon Hausmann 2012-01-11 07:34:56 PST
Your patch makes sense, but I'm curious about one bit: Why do the other ports not need this? Where do _they_ reset the state of their notification presenter?

AFAICS disconnectFrame() only resets permissions, but does that imply cancellation of any visible notifications, too?
Comment 5 Kristóf Kosztyó 2012-01-11 09:33:17 PST
(In reply to comment #4)
> Your patch makes sense, but I'm curious about one bit: Why do the other ports not need this? Where do _they_ reset the state of their notification presenter?

I have to look after it how it works on the other platforms.
This problem is caused by the m_notifications which is still containing the notifications from the previous tests when the flaky one is running so the "click" sometimes goes to the wrong notification.

> 
> AFAICS disconnectFrame() only resets permissions, but does that imply cancellation of any visible notifications, too?

I don't know, I will check this too.
Comment 6 Kristóf Kosztyó 2012-01-24 05:21:31 PST
(In reply to comment #5)
> (In reply to comment #4)
> > Your patch makes sense, but I'm curious about one bit: Why do the other ports not need this? Where do _they_ reset the state of their notification presenter?
> 
> I have to look after it how it works on the other platforms.
> This problem is caused by the m_notifications which is still containing the notifications from the previous tests when the flaky one is running so the "click" sometimes goes to the wrong notification.
> 
> > 
> > AFAICS disconnectFrame() only resets permissions, but does that imply cancellation of any visible notifications, too?
> 
> I don't know, I will check this too.

I looked after it in the chromium, and I found this in the changelog:
+        Adds necessary hooks to chromium's DRT so that clicks on desktop notifications
+        can be simulated during a layout test.  Requires storing a list of active
+        notifications so that they can be referred to later for clicking.
+        https://bugs.webkit.org/show_bug.cgi?id=44800

That means it is normal to the previous test's notification is still reachable btw there isn't any tests what work this way and this can cause sideeffects like this one.
Comment 7 Yael 2012-02-03 07:53:53 PST
(In reply to comment #4)
> Your patch makes sense, but I'm curious about one bit: Why do the other ports not need this? Where do _they_ reset the state of their notification presenter?
> 
Other ports do not run http/notifications tests, so they don't have this problem.
Comment 8 Yael 2012-02-03 07:59:43 PST
Comment on attachment 121094 [details]
proposed fix

This patch makes sense to me.
We identify notifications by their title, but all of the test notifications use the same title. That's what causes the side effect.
IMO it is ok to clear the notifications between tests, even if in the real world we would not cancel notifications when navigating away from the page that created them.
Comment 9 WebKit Review Bot 2012-03-07 11:52:36 PST
Comment on attachment 121094 [details]
proposed fix

Rejecting attachment 121094 [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:
ines).
patching file Source/WebKit/qt/WebCoreSupport/NotificationPresenterClientQt.cpp
patching file Source/WebKit/qt/WebCoreSupport/NotificationPresenterClientQt.h
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp
Hunk #1 succeeded at 555 (offset -4 lines).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Simon Haus..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/11856198
Comment 10 Csaba Osztrogonác 2012-07-11 09:18:29 PDT
Arghhhh ... why wasn't it committed? Kristóf?

Otherwise http/tests/notifications tests were removed from trunk - https://trac.webkit.org/changeset/115153

Do we still need this fix?
Comment 11 Csaba Osztrogonác 2012-11-22 03:29:45 PST
ping?
Comment 12 Csaba Osztrogonác 2013-11-15 05:02:38 PST
notifications and Qt isn't in WebKit trunk long time ago.