Bug 45824 – Layout test notification-replace.html should not depend on platform

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Victor Wang]

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.

[John Gregg]

i think this is equivalent to bug 45326, which i have a patch for on the way

[Victor Wang]

Created attachment 67686 [details]
Proposed Patch

[Victor Wang]

(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]

No, I think that change is actually better than changing the DRT, so go for it.  CC: yael for qt heads-up

[Yael]

(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]

(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]

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]

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]

(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]

(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]

Yes, it would be fine if notifications-replace.html didn't use a real icon.

[Victor Wang]

Created attachment 67733 [details]
Proposed patch

[Victor Wang]

(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]

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]

(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 on attachment 67733 [details]
Proposed patch

okiedokes

[WebKit Commit Bot]

Comment on attachment 67733 [details]
Proposed patch

Clearing flags on attachment: 67733

Committed r68372: <http://trac.webkit.org/changeset/68372>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.