RESOLVED FIXED 45824
Layout test notification-replace.html should not depend on platform
https://bugs.webkit.org/show_bug.cgi?id=45824
Summary Layout test notification-replace.html should not depend on platform
Victor Wang
Reported 2010-09-15 10:08:06 PDT
Layout test fast\notifications\notifications-replace.html uses local urls for icons, and this causes it needs different expectations on mac/linux and windows. Also on Windows, the actual result varies if the test runs from different disk drives. For example, the current chromium-win baseline expects the test runs from C drive, so this layout test fails on bots that are not running the test on C drive. The test should be modified to be platform independent.
Attachments
Proposed Patch (4.93 KB, patch)
2010-09-15 10:15 PDT, Victor Wang
no flags
Proposed patch (4.78 KB, patch)
2010-09-15 15:40 PDT, Victor Wang
no flags
John Gregg
Comment 1 2010-09-15 10:09:51 PDT
i think this is equivalent to bug 45326, which i have a patch for on the way
Victor Wang
Comment 2 2010-09-15 10:15:31 PDT
Created attachment 67686 [details] Proposed Patch
Victor Wang
Comment 3 2010-09-15 10:17:03 PDT
(In reply to comment #1) > i think this is equivalent to bug 45326, which i have a patch for on the way Cool. I already have the patch. But if you are also working on it and prefer to submit your patch. Fine with me.
John Gregg
Comment 4 2010-09-15 10:18:16 PDT
No, I think that change is actually better than changing the DRT, so go for it. CC: yael for qt heads-up
Yael
Comment 5 2010-09-15 13:26:24 PDT
(In reply to comment #4) > No, I think that change is actually better than changing the DRT, so go for it. CC: yael for qt heads-up Thanks for cc'ing me. In Qt, we load the icon in WebCore, and when the local file tries to load the http icon, we get "SECURITY_ERR: DOM Exception 18". Can the test move to LayoutTests/http/tests/notifications when you make this change?
Victor Wang
Comment 6 2010-09-15 13:56:33 PDT
(In reply to comment #5) > (In reply to comment #4) > > No, I think that change is actually better than changing the DRT, so go for it. CC: yael for qt heads-up > > Thanks for cc'ing me. > In Qt, we load the icon in WebCore, and when the local file tries to load the http icon, we get "SECURITY_ERR: DOM Exception 18". > > Can the test move to LayoutTests/http/tests/notifications when you make this change? Are you getting same error for the following notification tests in the same folder? fast\notifications\notifications-double-show.html fast\notifications\notifications-with-permission.html fast\notifications\notifications-without-permission.html
Victor Wang
Comment 7 2010-09-15 14:38:49 PDT
Hi John / Yael, We are not testing notification icon in this test, right? If so, could I set icon url to empty in this test? There are some notification tests in the same folder set icon url to "" so the test does not need to load icon files...
John Gregg
Comment 8 2010-09-15 14:40:35 PDT
notifications-with-permission.html does effectively test that the icon URL is being passed to the notification provider. We'd lose that coverage if we changed all the icons to be empty. But it could be in an http test using an http:// icon and accomplish the same task.
Yael
Comment 9 2010-09-15 14:55:58 PDT
(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > No, I think that change is actually better than changing the DRT, so go for it. CC: yael for qt heads-up > > > > Thanks for cc'ing me. > > In Qt, we load the icon in WebCore, and when the local file tries to load the http icon, we get "SECURITY_ERR: DOM Exception 18". > > > > Can the test move to LayoutTests/http/tests/notifications when you make this change? > > Are you getting same error for the following notification tests in the same folder? > fast\notifications\notifications-double-show.html > fast\notifications\notifications-with-permission.html > fast\notifications\notifications-without-permission.html Any test that would change from file:/// to http:// would cause this problem.
Victor Wang
Comment 10 2010-09-15 15:25:27 PDT
(In reply to comment #8) > notifications-with-permission.html does effectively test that the icon URL is being passed to the notification provider. We'd lose that coverage if we changed all the icons to be empty. But it could be in an http test using an http:// icon and accomplish the same task. notifications-with-permission.html tests the icon url, does notifications-replace.html need to cover that case? Looks to me we can't move it to http/tests/notifications as all tests at http/tests/notifications are skipped in Chromium. Here are the comments from LayoutTests\platform\chromium\test_expectations.txt: // Chromium does not use the icon loader in WebCore for loading notifications. WONTFIX SKIP : http/tests/notifications = FAIL
John Gregg
Comment 11 2010-09-15 15:26:32 PDT
Yes, it would be fine if notifications-replace.html didn't use a real icon.
Victor Wang
Comment 12 2010-09-15 15:40:16 PDT
Created attachment 67733 [details] Proposed patch
Victor Wang
Comment 13 2010-09-15 15:41:25 PDT
(In reply to comment #11) > Yes, it would be fine if notifications-replace.html didn't use a real icon. New patch uploaded, please take a look.
Yael
Comment 14 2010-09-15 17:52:53 PDT
The new patch looks ok to me. BTW, the tests in http/tests/notifications are tests I added for the icon loader. If you skip them individually, you could move some of the notifications tests to that folder.
Victor Wang
Comment 15 2010-09-23 16:25:42 PDT
(In reply to comment #14) > The new patch looks ok to me. > > BTW, the tests in http/tests/notifications are tests I added for the icon loader. > If you skip them individually, you could move some of the notifications tests to that folder. This won't work for notifications-replace test if we change the icon load urls and move it here. If moved, it has to be disabled in chromium for the same reason as the icon loader tests you added.
Adam Barth
Comment 16 2010-09-26 22:34:12 PDT
Comment on attachment 67733 [details] Proposed patch okiedokes
WebKit Commit Bot
Comment 17 2010-09-26 23:12:06 PDT
Comment on attachment 67733 [details] Proposed patch Clearing flags on attachment: 67733 Committed r68372: <http://trac.webkit.org/changeset/68372>
WebKit Commit Bot
Comment 18 2010-09-26 23:12:11 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.