| Summary: | Add identifying methods for date/time input types. | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Oli Lan <olilan> | ||||||||||||||||||
| Component: | New Bugs | Assignee: | Oli Lan <olilan> | ||||||||||||||||||
| Status: | RESOLVED WONTFIX | ||||||||||||||||||||
| Severity: | Normal | CC: | abarth, commit-queue, dglazkov, eric, fishd, jamesr, m.kurz+webkitbugs, peter, tkent, tkent+wkapi, webkit.review.bot, xpersany | ||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Oli Lan
2012-02-15 14:40:33 PST
*** This bug has been marked as a duplicate of bug 78745 *** Reopening to attach new patch. Created attachment 127240 [details]
Patch
*** Bug 78745 has been marked as a duplicate of this bug. *** Comment on attachment 127240 [details]
Patch
So the idea is that all the other InputTypes had these already, this is just filling in the missing ones?
Yes, all of the other FooInputType classes have isBlah() methods defined; this patch adds similar to the date/time types. Comment on attachment 127240 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127240&action=review WebCore part is OK. Needs fishd's approval for the WebKit/chromium/public/ part. > Source/WebKit/chromium/public/WebTextInputType.h:56 > + WebTextInputTypeDate, > + WebTextInputTypeDateTime, > + WebTextInputTypeDateTimeLocal, > + WebTextInputTypeMonth, > + WebTextInputTypeTime, > + WebTextInputTypeWeek, Are they *text* input types? It's debatable whether they are text input types, but I would argue that as they have a text value they can be considered text types, even if an implementation gives them a graphical interface. Also note that BaseDateAndTimeInputType extends TextFieldInputType. Attachment 127240 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource First, rewinding head to replay your work on top of it... Applying: dataTransfer.types (HTML5 drag & drop) should return DOMStringList Using index info to reconstruct a base tree... <stdin>:84: trailing whitespace. <stdin>:333: trailing whitespace. <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" <stdin>:334: trailing whitespace. width="300" height="300" onload="runRepaintTest()"> <stdin>:335: trailing whitespace. <script xlink:href="../../fast/repaint/resources/repaint.js" /> <stdin>:336: trailing whitespace. <script><![CDATA[ warning: squelched 16 whitespace errors warning: 21 lines add whitespace errors. Falling back to patching base and 3-way merge... Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging LayoutTests/platform/chromium/test_expectations.txt Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Failed to merge in the changes. Patch failed at 0001 dataTransfer.types (HTML5 drag & drop) should return DOMStringList When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style. Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API. Attachment 127240 [details] did not pass style-queue: Logging in as commit-queue@webkit.org... Fetching: https://bugs.webkit.org/attachment.cgi?id=127240&action=edit Fetching: https://bugs.webkit.org/show_bug.cgi?id=78746&ctype=xml Processing 1 patch from 1 bug. Processing patch 127240 from bug 78746. Failed to run "[u'/Users/abarth/git/webkit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /Users/abarth/git/webkit-queue/ Last 500 characters of output: ore/html/WeekInputType.h Hunk #1 FAILED at 53. 1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/html/WeekInputType.h.rej patching file Source/WebKit/chromium/public/WebTextInputType.h Hunk #1 FAILED at 48. 1 out of 1 hunk FAILED -- saving rejects to file Source/WebKit/chromium/public/WebTextInputType.h.rej patching file Source/WebKit/chromium/src/WebViewImpl.cpp Hunk #1 FAILED at 1630. 1 out of 1 hunk FAILED -- saving rejects to file Source/WebKit/chromium/src/WebViewImpl.cpp.rej Failed to run "[u'/Users/abarth/git/webkit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /Users/abarth/git/webkit-queue/ If any of these errors are false positives, please file a bug against check-webkit-style. Comment on attachment 127240 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127240&action=review >> Source/WebKit/chromium/public/WebTextInputType.h:56 >> + WebTextInputTypeWeek, > > Are they *text* input types? Maybe they are text input types since the fallback in browsers that do not support these would be a text input field? And, don't they end up generating a string representation of time? If this doesn't feel right, then maybe we should change this enum to be named WebInputTypeFoo? Comment on attachment 127240 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127240&action=review >>> Source/WebKit/chromium/public/WebTextInputType.h:56 >>> + WebTextInputTypeWeek, >> >> Are they *text* input types? > > Maybe they are text input types since the fallback in browsers that do not support these would be a text input field? And, don't they end up generating a string representation of time? > > If this doesn't feel right, then maybe we should change this enum to be named WebInputTypeFoo? ok, It would be reasonable. It's ok to go ahead the patch. Comment on attachment 127240 [details]
Patch
OK, R+
Looks like I cannot CQ+ due to patch conflicts.
Created attachment 135580 [details]
Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI. Attachment 135580 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5]
Total errors found: 1 in 20 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 135580 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135580&action=review Thanks for the patch. One process comment and one substantive comment below: >> Source/WebCore/ChangeLog:8 >> + No new tests. (OOPS!) > > You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Please also add some information to the ChangeLog about why you're making this change. The purpose of the ChangeLog is to communicate with the rest of the community about your change. A blank ChangeLog like this one doesn't communicate much... > Source/WebKit/chromium/public/WebTextInputType.h:56 > + WebTextInputTypeDate, > + WebTextInputTypeDateTime, > + WebTextInputTypeDateTimeLocal, > + WebTextInputTypeMonth, > + WebTextInputTypeTime, > + WebTextInputTypeWeek, Can we add an API test to make sure these are working correctly? http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/tests Created attachment 138963 [details]
Patch
Created attachment 138964 [details]
Patch
Comment on attachment 138964 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138964&action=review > Source/WebKit/chromium/tests/WebViewTest.cpp:276 > + testTextInputType(WebTextInputTypeText, "input_field_default.html"); > + testTextInputType(WebTextInputTypeEmail, "input_field_email.html"); > + testTextInputType(WebTextInputTypeSearch, "input_field_search.html"); > + testTextInputType(WebTextInputTypeNumber, "input_field_number.html"); > +#if ENABLE(INPUT_TYPE_DATE) > + testTextInputType(WebTextInputTypeDate, "input_field_date.html"); > +#endif > +#if ENABLE(INPUT_TYPE_DATETIME) > + testTextInputType(WebTextInputTypeDateTime, "input_field_datetime.html"); > +#endif > +#if ENABLE(INPUT_TYPE_TIME) > + testTextInputType(WebTextInputTypeTime, "input_field_time.html"); > +#endif > +#if ENABLE(INPUT_TYPE_WEEK) > + testTextInputType(WebTextInputTypeWeek, "input_field_week.html"); > +#endif Why don't you test password, tel, url, datetime-local, and month? Fair point; I will add tests for those types. Created attachment 138971 [details]
Patch
Comment on attachment 138971 [details]
Patch
ok
This patch looks like it's got a review+. Should we land it? Created attachment 140866 [details]
Patch for landing
Comment on attachment 140866 [details] Patch for landing Clearing flags on attachment: 140866 Committed r116499: <http://trac.webkit.org/changeset/116499> All reviewed patches have been landed. Closing bug. The test is failing: [ RUN ] WebViewTest.TextInputType [7942:7942:0509/002759:2969469502:FATAL:resource_bundle.cc(83)] Check failed: g_shared_instance_ != NULL. [7942:7942:0509/002759:2969469502:FATAL:resource_bundle.cc(83)] Check failed: g_shared_instance_ != NULL. base::debug::StackTrace::StackTrace() [0x86c423c] base::(anonymous namespace)::StackDumpSignalHandler() [0x86bbc1d] 0xb76e7400 0xb69fba82 base::debug::BreakDebugger() [0x86b8db8] logging::RawLog() [0x86ab455] base::TestSuite::UnitTestAssertHandler() [0x86cad37] logging::LogMessage::~LogMessage() [0x86ace91] ui::ResourceBundle::GetSharedInstance() [0x8ba628a] TestWebKitPlatformSupport::GetLocalizedString() [0x86cec2b] webkit_glue::WebKitPlatformSupportImpl::queryLocalizedString() [0x96e5cef] TestWebKitPlatformSupport::queryLocalizedString() [0x86d09e2] WebCore::query() [0x978abfc] WebCore::dateFormatMonthText() [0x978ac5c] WebCore::LocaleICU::initializeLocalizedDateFormatText() [0x8da4f68] WebCore::LocaleICU::localizedDateFormatText() [0x8da523e] WebCore::localizedDateFormatText() [0x8da71cb] WebCore::DateInputType::fixedPlaceholder() [0x8c99258] WebCore::TextFieldInputType::updatePlaceholderText() [0x8c31581] WebCore::HTMLInputElement::updatePlaceholderText() [0x8beb4c4] WebCore::HTMLTextFormControlElement::updatePlaceholderVisibility() [0x8c1b53b] WebCore::HTMLInputElement::updateInnerTextValue() [0x8bebe9d] WebCore::HTMLInputElement::updateType() [0x8bef79a] WebCore::HTMLInputElement::parseAttribute() [0x8beffff] WebCore::StyledElement::attributeChanged() [0x97c159b] WebCore::Element::parserSetAttributes() [0x870b7eb] WebCore::HTMLConstructionSite::createHTMLElement() [0x8cae9f9] WebCore::HTMLConstructionSite::insertSelfClosingHTMLElement() [0x8cafe84] WebCore::HTMLTreeBuilder::processStartTagForInBody() [0x8c71366] WebCore::HTMLTreeBuilder::processStartTag() [0x8c7212a] WebCore::HTMLTreeBuilder::constructTreeFromAtomicToken() [0x8c76731] WebCore::HTMLTreeBuilder::constructTreeFromToken() [0x8c76a43] WebCore::HTMLDocumentParser::pumpTokenizer() [0x8c589bd] WebCore::HTMLDocumentParser::append() [0x8c58e58] WebCore::DecodedDataDocumentParser::flush() [0x97bbef5] WebCore::DocumentWriter::end() [0x8fc387a] WebCore::DocumentLoader::finishedLoading() [0x8fbde35] WebCore::MainResourceLoader::didFinishLoading() [0x8fe38d9] WebCore::ResourceLoader::didFinishLoading() [0x8ff7ed6] WebCore::ResourceHandleInternal::didFinishLoading() [0x9be6437] WebURLLoaderMock::ServeAsynchronousRequest() [0x86d1c59] WebURLLoaderMockFactory::ServeAsynchronousRequests() [0x86ce746] webkit_support::ServeAsynchronousMockedRequests() [0x86cc453] WebKit::FrameTestHelpers::createWebViewAndLoad() [0x8442024] (anonymous namespace)::WebViewTest::testTextInputType() [0x85cf842] (anonymous namespace)::WebViewTest_TextInputType_Test::TestBody() [0x85cfde1] We need to initialize webkit_support. Because I know this is a test code issue. I won't roll out the patch and will disable the test. > We need to initialize webkit_support. Because I know this is a test code issue. I won't roll out the patch and will disable the test.
Thanks. I wonder why the the commit-queue didn't catch that problem.
Re-opening to fix the test problem. I'm not able to reproduce this problem: abarth@quadzen:~/svn/webkit$ ./out/Debug/webkit_unit_tests --chromium --gtest_filter=WebViewTest.TextInputType Note: Google Test filter = WebViewTest.TextInputType [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from WebViewTest [ RUN ] WebViewTest.TextInputType [ OK ] WebViewTest.TextInputType (109 ms) [----------] 1 test from WebViewTest (109 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test case ran. (109 ms total) [ PASSED ] 1 test. YOU HAVE 5 DISABLED TESTS YOU HAVE 3 tests with ignored failures (FAILS prefix) What am I doing wrong? Created attachment 140958 [details]
patch to re-enable test
I'm seeing the failure too, although it wasn't occurring when I created the test. It happens when DateInputType is tested, so you won't see it if you don't have ENABLE(INPUT_TYPE_DATE). I believe the issue started after https://bugs.webkit.org/show_bug.cgi?id=83872 was landed, as that added a call to localizedDateFormatText() which is failing because the resources aren't initialised. I'll upload a patch to fix. Created attachment 144058 [details]
Patch
Comment on attachment 144058 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144058&action=review > Source/WebKit/chromium/tests/WebViewTest.cpp:45 > +#include "ui/base/resource/resource_bundle.h" No, we must not call chromium code other than webkit/support/. OK, understood. What's the correct way to fix this? (In reply to comment #37) > OK, understood. What's the correct way to fix this? We need to fix webkit/support/. We call webkit_support::SetUpTestEnvironmentForUniTest() before running WebKit unit tests. We need to initialize localization resources in it. This bug doesn't need to block Bug 66687. It sounds like the right fix is on the Chromium side. Once we make that change, we can re-enable the test. Can someone please give an update on this issue? With Firefox 54 having date inputs behind a flag soon Safari will be the last major browser left lacking support for it. See http://caniuse.com/#feat=input-datetime This was a chromium-specific issue. |