Summary: | Align HTMLLabelElement.prototype.form with the HTML specification | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||
Component: | DOM | Assignee: | Chris Dumez <cdumez> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | achristensen, ap, buildbot, cdumez, darin, rniwa, ryanhaddad, sam | ||||||||||||
Priority: | P2 | Keywords: | WebExposed | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
URL: | https://html.spec.whatwg.org/#dom-label-form | ||||||||||||||
Attachments: |
|
Description
Chris Dumez
2016-09-21 19:12:55 PDT
Created attachment 289506 [details]
Patch
Comment on attachment 289506 [details] Patch Attachment 289506 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2122464 New failing tests: imported/w3c/web-platform-tests/html/semantics/forms/form-control-infrastructure/form.html Created attachment 289512 [details]
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 289513 [details]
Patch
Comment on attachment 289513 [details] Patch Attachment 289513 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2123128 New failing tests: fast/images/move-image-to-new-document.html Created attachment 289520 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
(In reply to comment #6) > Created attachment 289520 [details] > Archive of layout-test-results from ews104 for mac-yosemite-wk2 > > The attached test failures were seen while running run-webkit-tests on the > mac-wk2-ews. > Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5 (In reply to comment #5) > Comment on attachment 289513 [details] > Patch > > Attachment 289513 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-queues.webkit.org/results/2123128 > > New failing tests: > fast/images/move-image-to-new-document.html This test seems flaky, it also failed on my other patch. (In reply to comment #8) > Caused by r206244, rolling it out. Thanks Alexey! Created attachment 289619 [details]
Patch
Anyone interested in reviewing? The new implementation matches the specification exactly. Also this aligns our behavior with both Chrome and Firefox, thus improving interoperability. Comment on attachment 289619 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=289619&action=review > Source/WebCore/html/HTMLLabelElement.h:35 > WEBCORE_EXPORT LabelableElement* control(); > - WEBCORE_EXPORT HTMLFormElement* form() const; > + WEBCORE_EXPORT HTMLFormElement* form(); We should make control const instead of form non-const. (In reply to comment #13) > Comment on attachment 289619 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=289619&action=review > > > Source/WebCore/html/HTMLLabelElement.h:35 > > WEBCORE_EXPORT LabelableElement* control(); > > - WEBCORE_EXPORT HTMLFormElement* form() const; > > + WEBCORE_EXPORT HTMLFormElement* form(); > > We should make control const instead of form non-const. I don't mind either way but you do realize this means I'll have to add a const_cast inside control(), right? In the past, you have complained about those. Comment on attachment 289619 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=289619&action=review >>> Source/WebCore/html/HTMLLabelElement.h:35 >>> + WEBCORE_EXPORT HTMLFormElement* form(); >> >> We should make control const instead of form non-const. > > I don't mind either way but you do realize this means I'll have to add a const_cast inside control(), right? In the past, you have complained about those. I think the patch is fine as is but change const-ness if Alex feels strongly either way. (In reply to comment #15) > Comment on attachment 289619 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=289619&action=review > > >>> Source/WebCore/html/HTMLLabelElement.h:35 > >>> + WEBCORE_EXPORT HTMLFormElement* form(); > >> > >> We should make control const instead of form non-const. > > > > I don't mind either way but you do realize this means I'll have to add a const_cast inside control(), right? In the past, you have complained about those. > > I think the patch is fine as is but change const-ness if Alex feels strongly > either way. The issue with making form() const is that it requires control() to be const as well. However, control() uses descendantsOfType() internally which uses a const iterator if |this| is const. Therefore, to make control() const I would need to either const_cast |this| before calling descendantsOfType() or const_cast the Element pointed to by the const_iterator returned by descendantsOfType(). Comment on attachment 289619 [details] Patch Clearing flags on attachment: 289619 Committed r206332: <http://trac.webkit.org/changeset/206332> All reviewed patches have been landed. Closing bug. |