-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add test for timezonechange event #26555
base: master
Are you sure you want to change the base?
Add test for timezonechange event #26555
Conversation
FYI for WPT folk: This PR is blocked on an RFC for the new testdriver extension, which is in turn blocked on whatwg/html#3047 being updated. Frank is aware of this, and we aren't expecting anything to land until we get those two sorted. |
Agree. I would like to get more other early feedback about the tests itself beside that issue from this PR. |
@zbraniecki - Would you be able to find someone from Mozilla to support this move too? |
To be clear, Mozilla is supportive of this effort. I'll defer to @jgraham et al for the technical details though. |
The RFC is now posted at web-platform-tests/rfcs#73, although it is still will not be able to land until whatwg/html#3047 at least has the webdriver text in the PR (preferably also that the PR is landed). However we're making progress here, so I will start reviewing the WPT test infra side of this PR :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small nits, but this LGTM from the WPT infrastructure perspective, thanks!
I'm marking it request changes currently just until the RFC is merged, then I will switch to Approve.
ff5f8f5
to
361b97a
Compare
Updated. Please take another look |
PTAL |
This CL is NOT intended to be merge into chromium. I just want to use the chromium tryBot to validate the code is correct. The WPT PR is in #26555 Bug: 1144403 Change-Id: Ie3c062d1d9e59ee3f9087c7af99df7d2d29b056d
PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % some minor suggestions
let timeZone = "Asia/Taipei"; | ||
let timeZone2 = "Asia/Hong_Kong"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are both GMT+8 (i.e. same local time). Can we use two timezones that actually differ to make the difference more clear? Say, 'Asia/Taipei'
and 'Europe/Berlin'
(GMT+1), or 'Pacific/Fakaofo'
(GMT+13) as you use in the other tests.
let timeZone = "Asia/Taipei"; | |
let timeZone2 = "Asia/Hong_Kong"; | |
const timeZone = "Asia/Taipei"; | |
const timeZone2 = "Pacific/Fakaofo"; |
WDYT?
@@ -0,0 +1,14 @@ | |||
self.addEventListener('message', function(e) { | |||
const message = e.data; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: indentation should be 2 spaces
window.addEventListener('timezonechange', r); | ||
window.test_driver.set_time_zone(newTimeZone); | ||
}).then(e => { | ||
assert_equals(defaultTimeZone(), newTimeZone); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: indentation
assert_equals(defaultTimeZone(), newTimeZone); | |
assert_equals(defaultTimeZone(), newTimeZone); |
port.postMessage("SUCCESS:" + timezone); | ||
}; | ||
port.postMessage("READY:" + oldtimezone); // (the html will change the timezone) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
} | |
}; |
ontimezonechange = evt => { | ||
let timezone = (new Intl.DateTimeFormat()).resolvedOptions().timeZone; | ||
postMessage("SUCCESS:" + timezone); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | |
}; |
b318f8c
to
2b3590e
Compare
It is so hard to merge with the master via git command so I just did a git diff and then hand merge and hard push. Please review it again. Thanks |
Add tests to test whatwg/html#3047
WebDriver extension proposal in whatwg/html#3047
@stephenmcgruer @mathiasbynens @littledan