WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Proposed patch
(4.78 KB, patch)
2010-09-15 15:40 PDT
,
Victor Wang
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug