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.
i think this is equivalent to bug 45326, which i have a patch for on the way
Created attachment 67686 [details] Proposed Patch
(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.
No, I think that change is actually better than changing the DRT, so go for it. CC: yael for qt heads-up
(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?
(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
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...
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.
(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.
(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
Yes, it would be fine if notifications-replace.html didn't use a real icon.
Created attachment 67733 [details] Proposed patch
(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.
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.
(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.
Comment on attachment 67733 [details] Proposed patch okiedokes
Comment on attachment 67733 [details] Proposed patch Clearing flags on attachment: 67733 Committed r68372: <http://trac.webkit.org/changeset/68372>
All reviewed patches have been landed. Closing bug.