| Summary: | Update some picker tests for input[type=date] | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Kent Tamura <tkent> | ||||||
| Component: | Tools / Tests | Assignee: | Kent Tamura <tkent> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | dbates, haraken, keishi, morrita, yutak | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 98226 | ||||||||
| Attachments: |
|
||||||||
|
Description
Kent Tamura
2012-10-04 02:44:56 PDT
Created attachment 167074 [details]
WIP
Created attachment 167103 [details]
Patch
Comment on attachment 167103 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167103&action=review This patch looks sane to me. On another note, I've noticed that the original test code seems to alternate between using single and double quotes. We should pick one style and stick with it for consistency. Also, some tests force a layout before calling openPicker() (using dateInput.offsetTop) and some don't (e.g. LayoutTests/fast/forms/date/calendar-picker-with-step.html). I briefly looked over some of these test files. I take it that it is intentional to force a layout in some tests and not force a layout it others. If I have some time, I'll look to read through the tests in their entirety. > LayoutTests/ChangeLog:25 > + ditto. Nit: ditto => Ditto > LayoutTests/ChangeLog:27 > + ditto. Nit: ditto => Ditto > LayoutTests/ChangeLog:31 > + Increase the internal time out becuase it was too short on my machine. Nit: "time out" => "timeout" And becuase => because > LayoutTests/fast/forms/date/calendar-picker-appearance-pre-100.html:28 > + dateInput.offsetTop; I know that the original test had this line. You may want to consider adding an inline comment here that explains that this forces a layout. Even better, explain why we need to force a layout. > LayoutTests/fast/forms/date/calendar-picker-appearance.html:18 > + dateInput.offsetTop; Ditto. > LayoutTests/platform/chromium/fast/forms/date/date-suggestion-picker-appearance-with-scroll-bar.html:61 > + dateInput.offsetTop; Ditto. > LayoutTests/platform/chromium/fast/forms/date/date-suggestion-picker-appearance.html:41 > + dateInput.offsetTop; Ditto. Thank you for the comments. (In reply to comment #3) > On another note, I've noticed that the original test code seems to alternate between using single and double quotes. We should pick one style and stick with it for consistency. Also, some tests force a layout before calling openPicker() (using dateInput.offsetTop) and some don't (e.g. LayoutTests/fast/forms/date/calendar-picker-with-step.html). I briefly looked over some of these test files. I take it that it is intentional to force a layout in some tests and not force a layout it others. If I have some time, I'll look to read through the tests in their entirety. Yeah, I also had a concern about single/double quotes. We had better improve it. I guess offsetTop hacks were needed for tests which opened a picker before 'load' event, and the hack is not needed because all of tests has been changed so that they open pickers on onload handler. I'm removing offsetTop in this patch, and will watch bots status. Committed r130433: <http://trac.webkit.org/changeset/130433> (In reply to comment #4) > I'm removing offsetTop in this patch, and will watch bots status. It failed. I added offsetTop again in http://trac.webkit.org/changeset/130458. |