Summary: | Redraw slider tick marks when datalist changes. | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Keishi Hattori <keishi> | ||||||||||||||||||||||||||
Component: | Forms | Assignee: | Keishi Hattori <keishi> | ||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
Severity: | Normal | CC: | dglazkov, gustavo, mifenton, philn, rakuco, tkent, webkit.review.bot, xan.lopez | ||||||||||||||||||||||||||
Priority: | P2 | Keywords: | WebExposed | ||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||
Bug Blocks: | 87844 | ||||||||||||||||||||||||||||
Attachments: |
|
Description
Keishi Hattori
2012-06-19 19:11:20 PDT
Created attachment 148494 [details]
Patch
Here is the performance test results for setting id. The variance between runs is too high to tell a difference. https://docs.google.com/spreadsheet/ccc?key=0AoaiM9jljUy8dDZfT3d6YzJ1UFFES2RmbHQ0Skw2bFE Please upload an EWS-cookable patch. Can you use dom/WebKitMutationObserver.* ? Created attachment 148769 [details]
Patch
Comment on attachment 148769 [details] Patch Attachment 148769 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13025097 Comment on attachment 148769 [details] Patch Attachment 148769 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13033084 Created attachment 148788 [details]
Patch
Comment on attachment 148788 [details] Patch Attachment 148788 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13028130 Comment on attachment 148788 [details] Patch Attachment 148788 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13029120 Comment on attachment 148788 [details] Patch Attachment 148788 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13037121 Created attachment 148986 [details]
Patch
Comment on attachment 148986 [details] Patch Attachment 148986 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13071011 New failing tests: fast/forms/datalist/update-range-with-datalist.html Created attachment 149000 [details]
Archive of layout-test-results from ec2-cr-linux-01
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 152747 [details]
Patch
Comment on attachment 152747 [details] Patch Attachment 152747 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13279140 Comment on attachment 152747 [details] Patch Attachment 152747 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13276135 Comment on attachment 152747 [details] Patch Attachment 152747 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13284127 Comment on attachment 152747 [details] Patch Attachment 152747 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13279141 Comment on attachment 152747 [details] Patch Attachment 152747 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13276138 Comment on attachment 152747 [details] Patch Attachment 152747 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13274146 Comment on attachment 152747 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152747&action=review > Source/WebCore/html/HTMLInputElement.cpp:693 > - } > #if ENABLE(DATALIST) > - else if (attribute.name() == listAttr) > + } else if (attribute.name() == listAttr) { I recommend not to change '}' position. > Source/WebCore/html/HTMLInputElement.cpp:701 > #endif > #if ENABLE(INPUT_SPEECH) > - else if (attribute.name() == webkitspeechAttr) { > + } else if (attribute.name() == webkitspeechAttr) { I recommend move '}' before #endif. > Source/WebCore/html/HTMLInputElement.cpp:1517 > + m_listAttributeTargetObserver = ListAttributeTargetObserver::create(fastGetAttribute(listAttr), this); Do we need to create LIstAttributeTargetObserver when the input element is !inDocument()? > Source/WebCore/html/HTMLInputElement.cpp:1523 > + if (isRangeControl()) > + setNeedsStyleRecalc(); Should introduce new virtual member function in InputType, or call setNeedsStyleRecalc() unconditionally. > Source/WebCore/html/HTMLOptionElement.cpp:282 > + ContainerNode* dataList = parentNode(); > + while (dataList && !dataList->hasTagName(datalistTag)) > + dataList = dataList->parentNode(); > + > + if (!dataList) > + return 0; > + > + return static_cast<HTMLDataListElement*>(dataList); Readability of this code isn't good. - Not following an idiom of parent traversal - static_cast<> is far from its requirement (hasTagName(datalistTag)) How about the following? for (ContainerNode parent = parentNode(); parent ; parent = parent->parentNode()) { if (parent->hasTagName(datalistTag)) return static_cast<HTMLDataListElement*>(dataList); } return 0; > LayoutTests/ChangeLog:12 > + * fast/forms/datalist/update-range-with-datalist-expected.txt: Added. > + * fast/forms/datalist/update-range-with-datalist.html: Added. > + * platform/chromium-mac/fast/forms/datalist/update-range-with-datalist-expected.png: Added. This can be a ref-test. Created attachment 152984 [details]
Patch
Comment on attachment 152747 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152747&action=review >> Source/WebCore/html/HTMLInputElement.cpp:693 >> + } else if (attribute.name() == listAttr) { > > I recommend not to change '}' position. Done. >> Source/WebCore/html/HTMLInputElement.cpp:701 >> + } else if (attribute.name() == webkitspeechAttr) { > > I recommend move '}' before #endif. Done. >> Source/WebCore/html/HTMLInputElement.cpp:1517 >> + m_listAttributeTargetObserver = ListAttributeTargetObserver::create(fastGetAttribute(listAttr), this); > > Do we need to create LIstAttributeTargetObserver when the input element is !inDocument()? No. Added check. >> Source/WebCore/html/HTMLInputElement.cpp:1523 >> + setNeedsStyleRecalc(); > > Should introduce new virtual member function in InputType, or call setNeedsStyleRecalc() unconditionally. We will be doing different things for input type text and color so I added a new virtual member function to InputType. >> Source/WebCore/html/HTMLOptionElement.cpp:282 >> + return static_cast<HTMLDataListElement*>(dataList); > > Readability of this code isn't good. > - Not following an idiom of parent traversal > - static_cast<> is far from its requirement (hasTagName(datalistTag)) > > How about the following? > > for (ContainerNode parent = parentNode(); parent ; parent = parent->parentNode()) { > if (parent->hasTagName(datalistTag)) > return static_cast<HTMLDataListElement*>(dataList); > } > return 0; Done. >> LayoutTests/ChangeLog:12 >> + * platform/chromium-mac/fast/forms/datalist/update-range-with-datalist-expected.png: Added. > > This can be a ref-test. Done. Comment on attachment 152984 [details] Patch Attachment 152984 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13278488 Comment on attachment 152984 [details] Patch Attachment 152984 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13277512 Comment on attachment 152984 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152984&action=review > Source/WebCore/html/HTMLInputElement.cpp:90 > +class ListAttributeTargetObserver : IdTargetObserver { > +public: > + static PassOwnPtr<ListAttributeTargetObserver> create(const AtomicString& id, HTMLInputElement*); > + virtual void idTargetChanged() OVERRIDE; > + > +private: > + ListAttributeTargetObserver(const AtomicString& id, HTMLInputElement*); > + > + HTMLInputElement* m_element; > +}; should be enclosed with #if ENABLE(DATALIST) > Source/WebCore/html/HTMLInputElement.cpp:1522 > +{ > + if (!inDocument()) > + return; > + m_listAttributeTargetObserver = ListAttributeTargetObserver::create(fastGetAttribute(listAttr), this); > +} I know this works well. However this code might make a reader nervous because of possibility of leak of a wrong ListAttributeTargetObserver object. I recommend clearing m_listAttributeTargetObserver if !inDocument(), and use resetListAttributeTargetObserver() in removedFrom() too. > Source/WebCore/html/HTMLInputElement.cpp:1811 > +PassOwnPtr<ListAttributeTargetObserver> ListAttributeTargetObserver::create(const AtomicString& id, HTMLInputElement* element) > +{ > + return adoptPtr(new ListAttributeTargetObserver(id, element)); > +} > + > +ListAttributeTargetObserver::ListAttributeTargetObserver(const AtomicString& id, HTMLInputElement* element) > + : IdTargetObserver(element->treeScope()->idTargetObserverRegistry(), id) > + , m_element(element) > +{ > +} > + > +void ListAttributeTargetObserver::idTargetChanged() > +{ > + m_element->listAttributeTargetChanged(); > +} should be enclosed with #if ENABLE(DATALIST) > Source/WebCore/html/HTMLInputElement.h:398 > + OwnPtr<ListAttributeTargetObserver> m_listAttributeTargetObserver; should be enclosed with #if ENABLE(DATALIST) Created attachment 153160 [details]
Patch
Comment on attachment 153160 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153160&action=review > Source/WebCore/html/HTMLInputElement.cpp:1446 > +#if ENABLE(DATALIST) > + resetListAttributeTargetObserver(); > +#endif > HTMLTextFormControlElement::removedFrom(insertionPoint); Node::removedFrom() clears inDocument flag. So this should be HTMLTextFormControlElement::removedFrom(insertionPoint); ASSERT(!inDocument()); #if ENABLE(DATALIST) resetListAttributeTargetObserver(); #endif Created attachment 153168 [details]
Patch
Comment on attachment 153168 [details]
Patch
ok
Created attachment 153181 [details]
Patch
Comment on attachment 153181 [details] Patch Rejecting attachment 153181 [details] from commit-queue. New failing tests: fast/forms/datalist/update-range-with-datalist.html Full output: http://queues.webkit.org/results/13273776 Created attachment 153192 [details]
Archive of layout-test-results from gce-cq-03
The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: gce-cq-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 153200 [details]
Patch
Comment on attachment 153200 [details] Patch Clearing flags on attachment: 153200 Committed r123081: <http://trac.webkit.org/changeset/123081> All reviewed patches have been landed. Closing bug. |