Bug 84351

Created attachment 138012 [details] Patch

Comment on attachment 138012 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138012&action=review > Source/WebCore/html/InputType.cpp:624 > + return theme->supportsDataListUI(formControlType()); We should ask RenderTheme about it only for supported types defined by the standard. > Source/WebCore/rendering/RenderTheme.h:135 > + virtual bool supportsDataListUI(const AtomicString& type) const { printf("XX\n"); return false; } printf > Source/WebCore/rendering/RenderThemeChromiumMac.mm:77 > + return type == InputTypeNames::text(); We supports email, tel, search, url, and number, right?

Created attachment 138024 [details] Patch

Comment on attachment 138024 [details] Patch Attachment 138024 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12483040

Comment on attachment 138024 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138024&action=review > Source/WebCore/ChangeLog:38 > + * rendering/RenderThemeChromiumSkia.h: > + (RenderThemeChromiumSkia): > + > +2012-04-19 Keishi Hattori <keishi@webkit.org> > + > + [Chromium] datalist: Inconsistent behavior of HTMLInputElement::list > + https://bugs.webkit.org/show_bug.cgi?id=84351 Duplicated ChangeLog entries > Source/WebCore/html/ColorInputType.cpp:164 > + Document* document = element()->document(); > + RefPtr<RenderTheme> theme = document->page() ? document->page()->theme() : RenderTheme::defaultTheme(); > + return theme->supportsDataListUI(formControlType()); Repeating this code is not good. How about adding a static function to InputType and call it from FooInputType::shouldRespectListAttribute, or implementing this in InputType and overriding in non-supported types? > Source/WebCore/rendering/RenderTheme.h:134 > + // A method asking if the theme is able to show datalist suggestions. In this case, "theme is able to show ..." is not correct; "The platform is able to show ..."? You should explain what is "type" argument. We should mention what types should be supported as per the standard. > Source/WebCore/rendering/RenderThemeChromiumMac.h:35 > + virtual bool supportsDataListUI(const AtomicString& type) const; should append OVERRIDE. > Source/WebCore/rendering/RenderThemeChromiumMac.mm:78 > + return type == InputTypeNames::text() || type == InputTypeNames::search() || type == InputTypeNames::url() > + || type == InputTypeNames::telephone() || type == InputTypeNames::email() || type == InputTypeNames::number(); We should add a FIXME comment about unsupported types. > Source/WebCore/rendering/RenderThemeChromiumSkia.cpp:133 > +bool RenderThemeChromiumSkia::supportsDataListUI(const AtomicString& type) const > +{ > + return type == InputTypeNames::text() || type == InputTypeNames::search() || type == InputTypeNames::url() > + || type == InputTypeNames::telephone() || type == InputTypeNames::email() || type == InputTypeNames::number(); > +} We shouldn't repeat the same code. We might want a file for such chromium-common code. RenderThemeChromiumCommon.h? > Source/WebCore/rendering/RenderThemeChromiumSkia.h:54 > + virtual bool supportsDataListUI(const AtomicString& type) const; should append OVERRIDE

We should add test cases to fast/forms/datalist/input-list.html.

Comment on attachment 138024 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138024&action=review >> Source/WebCore/rendering/RenderThemeChromiumSkia.cpp:133 >> +} > > We shouldn't repeat the same code. > We might want a file for such chromium-common code. RenderThemeChromiumCommon.h? Also, we have a problem with type=email (Bug 84346). We might need to unsupport it.

Created attachment 138717 [details] Patch

Comment on attachment 138717 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138717&action=review > Source/WebCore/rendering/RenderThemeChromiumMac.h:35 > + bool supportsDataListUI(const AtomicString& type) const OVERRIDE; Don't you add virtual? > Source/WebCore/rendering/RenderThemeChromiumSkia.h:55 > + bool supportsDataListUI(const AtomicString& type) const OVERRIDE; ditto. > LayoutTests/fast/forms/datalist/input-list.html:40 > <input type="text" id="i4" list="dl1"> > +<input type="search" id="i5" list="dl1"> > +<input type="url" id="i6" list="dl1"> > +<input type="telephone" id="i7" list="dl1"> > +<input type="email" id="i8" list="dl1"> > +<input type="datetime" id="i9" list="dl1"> > +<input type="date" id="i10" list="dl1"> > +<input type="month" id="i11" list="dl1"> > +<input type="week" id="i12" list="dl1"> > +<input type="time" id="i13" list="dl1"> > +<input type="datetime-local" id="i14" list="dl1"> > +<input type="number" id="i15" list="dl1"> > +<input type="range" id="i16" list="dl1"> > +<input type="color" id="i17" list="dl1"> > <!-- Unsupported type --> > -<input type="password" id="i5" list="dl1"> > +<input type="hidden" id="i18" list="dl1"> > +<input type="password" id="i19" list="dl1"> > +<input type="checkbox" id="i20" list="dl1"> > +<input type="radio" id="i21" list="dl1"> > +<input type="file" id="i22" list="dl1"> > +<input type="submit" id="i23" list="dl1"> > +<input type="image" id="i24" list="dl1"> > +<input type="reset" id="i25" list="dl1"> > +<input type="button" id="i26" list="dl1"> We had better rename their ID attribute values like id="text" id="search" and test them with var datalistElement = document.getElementById('dl1'); shouldBe('document.getElementById("text").list', 'datalistElement'); shouldBe('document.getElementById("search").list', 'datalistElement'); to improve readability of the test result. > LayoutTests/platform/chromium/test_expectations.txt:1101 > +// Implementation of datalist is incomplete. > +BUGWK27247 : fast/forms/datalist/input-list.html = PASS FAIL > + This test must not be flay. We should commit the ideal result as fast/forms/datalist/input-list-expected.txt, and commit the current Chromium result as platform/chromium/fast/forms/datalist/input-list-expected.txt. We shouldn't update test_expectations.txt.

(In reply to comment #9) > This test must not be flay. flay -> flaky. "PASS FAIL" means the test is flaky.

Created attachment 138733 [details] Patch

Comment on attachment 138733 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138733&action=review Looks ok > Source/WebCore/ChangeLog:3 > + [Chromium] datalist: Inconsistent behavior of HTMLInputElement::list This patch will resolve a Chromium issue, but touching non-Chromium files. So we should remove [Chromium].

Comment on attachment 138733 [details] Patch Attachment 138733 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12525542

Comment on attachment 138733 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138733&action=review > Source/WebCore/rendering/RenderThemeChromiumSkia.cpp:128 > +bool RenderThemeChromiumMac::supportsDataListUI(const AtomicString& type) const should be RenderThemeChromiumSkia::

Created attachment 138950 [details] Patch

Comment on attachment 138950 [details] Patch Rejecting attachment 138950 [details] from commit-queue. New failing tests: fast/forms/datalist/input-list.html fast/images/gif-large-checkerboard.html Full output: http://queues.webkit.org/results/12552123

Created attachment 139138 [details] Archive of layout-test-results from ec2-cq-03 The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: ec2-cq-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 139143 [details] Patch

Comment on attachment 139143 [details] Patch Rejecting attachment 139143 [details] from commit-queue. New failing tests: fast/images/gif-large-checkerboard.html Full output: http://queues.webkit.org/results/12567033

Created attachment 139179 [details] Archive of layout-test-results from ec2-cq-02 The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: ec2-cq-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 139143 [details] Patch Clearing flags on attachment: 139143 Committed r115704: <http://trac.webkit.org/changeset/115704>

All reviewed patches have been landed. Closing bug.