When the element that the form attribute is pointing to has changed, it should update the form in FormAssociatedElement.
Created attachment 151845 [details] Patch
Comment on attachment 151845 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151845&action=review > Source/WebCore/ChangeLog:35 > + (WebCore::Node::compareDocumentPosition): Added trustInDocument argument, because we need to use this > + when the DOM tree has been constructed but the inDocument property hasn't been updated yet. Would you explain more about this? What case does it happen? > Source/WebCore/ChangeLog:62 > + * dom/Node.h: > + (Node): > + * dom/TreeScope.cpp: > + (WebCore::TreeScope::TreeScope): > + (WebCore::TreeScope::addElementById): > + (WebCore::TreeScope::removeElementById): > + * dom/TreeScope.h: > + (WebCore::TreeScope::idTargetObserverRegistry): > + (TreeScope): > + * html/FormAssociatedElement.cpp: > + (WebCore::FormAssociatedElement::~FormAssociatedElement): > + (WebCore::FormAssociatedElement::didMoveToNewDocument): > + (WebCore::FormAssociatedElement::insertedInto): > + (WebCore::FormAssociatedElement::removedFrom): > + (WebCore::FormAssociatedElement::formAttributeChanged): > + (WebCore::FormAssociatedElement::idTargetChanged): > + (WebCore): > + * html/FormAssociatedElement.h: > + (FormAssociatedElement): > + * html/FormController.cpp: > + * html/FormController.h: > + (FormController): > + * html/HTMLFormElement.cpp: > + (WebCore::HTMLFormElement::removedFrom): > + (WebCore::HTMLFormElement::formElementIndexWithFormAttribute): > + * html/HTMLFormElement.h: > + (HTMLFormElement): Please write per-function change description. > Source/WebCore/dom/IdTargetObserverRegistry.cpp:41 > + HashSet<IdTargetObserver*>* set = m_registry.get(id.impl()); > + if (!set) { > + set = new HashSet<IdTargetObserver*>(); > + m_registry.add(id.impl(), set); get-and-add is not efficient. You should call just HashMap::add(id.impl(), 0) and refer to the resultant AddResult. > Source/WebCore/dom/IdTargetObserverRegistry.cpp:89 > + for (HashSet<IdTargetObserver*>::const_iterator it = set->begin(); it != set->end(); ++it) > + (*it)->idTargetChanged(id); Is it safe to iterate over the 'set'? I'm afraid an idTargetChanged() implementation can call addObserver() and removeObserver(). > Source/WebCore/dom/IdTargetObserverRegistry.cpp:99 > +void IdTargetObserverRegistry::notifyAllObservers() > +{ > + if (m_registry.isEmpty()) > + return; > + > + for (HashMap<AtomicStringImpl*, HashSet<IdTargetObserver*>* >::const_iterator it = m_registry.begin(); it != m_registry.end(); ++it) > + notifyObservers(it->first); > +} This function is not used. > Source/WebCore/dom/IdTargetObserverRegistry.h:48 > + HashMap<AtomicStringImpl*, HashSet<IdTargetObserver*>* > m_registry; Please add typedefs for readability. typedef HashSet<IdTargetObserver*> ObserverSet; typedef HashMap<AtomicStringImpl*, ObserverSet> IdToObserverSetMap; > Source/WebCore/dom/Node.cpp:1957 > -unsigned short Node::compareDocumentPosition(Node* otherNode) > +unsigned short Node::compareDocumentPosition(Node* otherNode, bool trustInDocument) Do not add bool argument. You should define an enum. > Source/WebCore/dom/TreeScope.h:30 > +#include "IdTargetObserverRegistry.h" I don't think we need to include IdTargetObserverRegistry.h. "class IdTargetObserverRegsitry;" should be enough. > Source/WebCore/dom/TreeScope.h:86 > + IdTargetObserverRegistry* idTargetObserverRegistry() const { return m_idTargetObserverRegistry.get(); } The return type should be IdTargetObserverRegistry& because it never be null. > Source/WebCore/html/FormAssociatedElement.cpp:48 > FormAssociatedElement::~FormAssociatedElement() > { > + HTMLElement* element = toHTMLElement(this); > + element->treeScope()->idTargetObserverRegistry()->addObserver(element->fastGetAttribute(formAttr), this); > setForm(0); Isn't it removeObserver()?
Comment on attachment 151845 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151845&action=review > Source/WebCore/dom/TreeScope.cpp:57 > + , m_idTargetObserverRegistry(adoptPtr(new IdTargetObserverRegistry)) We should have IdTargetObserverRegistry::create().
Created attachment 151928 [details] Patch
Created attachment 151942 [details] Patch
Comment on attachment 151845 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151845&action=review >> Source/WebCore/ChangeLog:62 >> + (HTMLFormElement): > > Please write per-function change description. Done. >> Source/WebCore/dom/IdTargetObserverRegistry.cpp:41 >> + m_registry.add(id.impl(), set); > > get-and-add is not efficient. You should call just HashMap::add(id.impl(), 0) and refer to the resultant AddResult. Done. >> Source/WebCore/dom/IdTargetObserverRegistry.cpp:89 >> + (*it)->idTargetChanged(id); > > Is it safe to iterate over the 'set'? I'm afraid an idTargetChanged() implementation can call addObserver() and removeObserver(). Used copyToVector and m_notifyingObserversInSet so that indexes don't disappear and observers don't get destroyed. >> Source/WebCore/dom/IdTargetObserverRegistry.cpp:99 >> +} > > This function is not used. Removed. >> Source/WebCore/dom/IdTargetObserverRegistry.h:48 >> + HashMap<AtomicStringImpl*, HashSet<IdTargetObserver*>* > m_registry; > > Please add typedefs for readability. > typedef HashSet<IdTargetObserver*> ObserverSet; > typedef HashMap<AtomicStringImpl*, ObserverSet> IdToObserverSetMap; Done. >> Source/WebCore/dom/Node.cpp:1957 >> +unsigned short Node::compareDocumentPosition(Node* otherNode, bool trustInDocument) > > Do not add bool argument. You should define an enum. Abandoned this change and went with another route. >> Source/WebCore/dom/TreeScope.cpp:57 >> + , m_idTargetObserverRegistry(adoptPtr(new IdTargetObserverRegistry)) > > We should have IdTargetObserverRegistry::create(). Done. >> Source/WebCore/dom/TreeScope.h:30 >> +#include "IdTargetObserverRegistry.h" > > I don't think we need to include IdTargetObserverRegistry.h. "class IdTargetObserverRegsitry;" should be enough. Done. >> Source/WebCore/dom/TreeScope.h:86 >> + IdTargetObserverRegistry* idTargetObserverRegistry() const { return m_idTargetObserverRegistry.get(); } > > The return type should be IdTargetObserverRegistry& because it never be null. Done. >> Source/WebCore/html/FormAssociatedElement.cpp:48 >> setForm(0); > > Isn't it removeObserver()? Done.
Comment on attachment 151942 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151942&action=review Please post a valid patch. Your patches made EWS purple. > Source/WebCore/dom/IdTargetObserver.cpp:41 > +void IdTargetObserver::setRegistry(IdTargetObserverRegistry* registry) > +{ > + m_registry = registry; Should add ASSERT(!m_registry || m_registry == registry); We had better add a comment to somewhere; "we can't register an IdTargetObserver to multiple registries." BTW, do you suppose we have a case that single IdTargetObserver observes multiple IDs in single TreeScope? > Source/WebCore/dom/IdTargetObserver.h:38 > + ~IdTargetObserver(); should be virtual. > Source/WebCore/dom/IdTargetObserverRegistry.cpp:94 > +void IdTargetObserverRegistry::notifyObservers(const AtomicString& id) > +{ > + ASSERT(!m_notifyingObserversInSet); > + if (id.isEmpty() || m_registry.isEmpty()) > + return; > + Because this function is called from a very hot place, we should be careful for performance. I propose: - Define notifyObservers() in IdTargetObserverRegistry.h for inlining. However, it just have emptiness checkes and calling notifyObserversInternal(). - Add notifyObserersInternal(), which should be defined in IdTargetObserverRegistry.cpp. > Source/WebCore/dom/TreeScope.cpp:98 > + nit: I don't think we need a blank line here. > Source/WebCore/dom/TreeScope.cpp:105 > + nit: ditto. > Source/WebCore/html/FormAssociatedElement.cpp:42 > - : m_form(0) > + : IdTargetObserver() > + , m_form(0) I think we don't need this change. > LayoutTests/fast/forms/update-form-attribute-element.html:66 > +function test4() { > + debug("Test 4: Order."); > + input1 = createInput("test3"); > + input2 = createInput("test3"); > + form = createForm("test3"); > + input3 = document.createElement("input"); > + form.appendChild(input3); > + input4 = createInput("test3"); > + input5 = createInput("test3"); using "test" in test4() is confusing. > LayoutTests/fast/forms/update-form-attribute-element.html:91 > +</html> Do we have a test for a case that a form control with form attribute is appended to the document with the form? e.g. form = document.createElement('"form"); form.id = "test5"; form.innerHTML = "<textarea><input form=test5><select>"; test.appendChild(form);
Comment on attachment 151942 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151942&action=review >> LayoutTests/fast/forms/update-form-attribute-element.html:66 >> + input5 = createInput("test3"); > > using "test" in test4() is confusing. I meant using "test3".
Comment on attachment 151942 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151942&action=review >> Source/WebCore/dom/IdTargetObserver.cpp:41 >> + m_registry = registry; > > Should add > ASSERT(!m_registry || m_registry == registry); > > We had better add a comment to somewhere; "we can't register an IdTargetObserver to multiple registries." > BTW, do you suppose we have a case that single IdTargetObserver observes multiple IDs in single TreeScope? This method couldn't support a sigle IdTargetObserver observing multiple ids, so I changed it so there is a new instance of IdTargetObserver for every id that you want to observe. >> Source/WebCore/dom/IdTargetObserverRegistry.cpp:94 >> + > > Because this function is called from a very hot place, we should be careful for performance. > I propose: > > - Define notifyObservers() in IdTargetObserverRegistry.h for inlining. However, it just have emptiness checkes and calling notifyObserversInternal(). > - Add notifyObserersInternal(), which should be defined in IdTargetObserverRegistry.cpp. Done. >> Source/WebCore/dom/TreeScope.cpp:98 >> + > > nit: I don't think we need a blank line here. Done. >> Source/WebCore/dom/TreeScope.cpp:105 >> + > > nit: ditto. Done. >>> LayoutTests/fast/forms/update-form-attribute-element.html:66 >>> + input5 = createInput("test3"); >> >> using "test" in test4() is confusing. > > I meant using "test3". Done. >> LayoutTests/fast/forms/update-form-attribute-element.html:91 >> +</html> > > Do we have a test for a case that a form control with form attribute is appended to the document with the form? e.g. > > form = document.createElement('"form"); > form.id = "test5"; > form.innerHTML = "<textarea><input form=test5><select>"; > test.appendChild(form); This test revealed a bug. compareDocumentPosition was returning DOCUMENT_POSITION_FOLLOWING | DOCUMENT_POSITION_CONTAINED_BY.
Created attachment 152159 [details] Patch
Comment on attachment 152159 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152159&action=review Please don't make EWS purple. > Source/WebCore/dom/IdTargetObserver.h:37 > + IdTargetObserver(IdTargetObserverRegistry&, const AtomicString& id); The constructor should be protected. > Source/WebCore/html/FormAssociatedElement.cpp:227 > +void FormAssociatedElement::setNewFormAttributeTargetObserver() nit: "setNew" sound redundant. We can say "reset" or "update" > Source/WebCore/html/FormAttributeTargetObserver.h:35 > +class FormAttributeTargetObserver : IdTargetObserver { nit: This class is small enough and used only by FormAssociatedElement. It's ok to put this class into FormAssociatedElement.cpp. > Source/WebCore/html/FormAttributeTargetObserver.h:38 > + virtual void idTargetChanged(); should have OVERRIDE > LayoutTests/fast/forms/update-form-attribute-element.html:83 > +function test5() { You'd like to have debug('\nTest 5'); for consistency?
Comment on attachment 152159 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152159&action=review >> Source/WebCore/dom/IdTargetObserver.h:37 >> + IdTargetObserver(IdTargetObserverRegistry&, const AtomicString& id); > > The constructor should be protected. Done. >> Source/WebCore/html/FormAssociatedElement.cpp:227 >> +void FormAssociatedElement::setNewFormAttributeTargetObserver() > > nit: "setNew" sound redundant. We can say "reset" or "update" Done. >> Source/WebCore/html/FormAttributeTargetObserver.h:35 >> +class FormAttributeTargetObserver : IdTargetObserver { > > nit: This class is small enough and used only by FormAssociatedElement. It's ok to put this class into FormAssociatedElement.cpp. Done >> Source/WebCore/html/FormAttributeTargetObserver.h:38 >> + virtual void idTargetChanged(); > > should have OVERRIDE Done. >> LayoutTests/fast/forms/update-form-attribute-element.html:83 >> +function test5() { > > You'd like to have debug('\nTest 5'); for consistency? Done.
Created attachment 152174 [details] Patch
Comment on attachment 152174 [details] Patch Attachment 152174 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13235041
Comment on attachment 152174 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152174&action=review > Source/WebCore/html/FormAssociatedElement.h:83 > + void resetFormAttributeTargetObserver(); This should be private.
Created attachment 152181 [details] Patch
Comment on attachment 152181 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152181&action=review > Source/WebCore/WebCore.vcproj/WebCore.vcproj:51206 > + RelativePath="..\dom\IdTargetObserver.cpp" > + > > + <FileConfiguration > + Name="Debug|Win32" > + ExcludedFromBuild="true" Because you specifies ExcludedFromBuild=true, you need to include IdTargetObserver.cpp in DOMAllInOne.cpp. > Source/WebCore/WebCore.vcproj/WebCore.vcproj:51262 > + RelativePath="..\dom\IdTargetObserverRegistry.cpp" > + > > + <FileConfiguration > + Name="Debug|Win32" > + ExcludedFromBuild="true" ditto.
Comment on attachment 152181 [details] Patch Attachment 152181 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13221558
Created attachment 152190 [details] Patch
Comment on attachment 152190 [details] Patch Looks ok. Please wait until all EWSes get green.
Comment on attachment 152190 [details] Patch Rejecting attachment 152190 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: Kit/chromium/third_party/yasm/source/patched-yasm --revision 134927 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' 48>At revision 134927. ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/13207999
Created attachment 152250 [details] Patch
Comment on attachment 152250 [details] Patch Clearing flags on attachment: 152250 Committed r122584: <http://trac.webkit.org/changeset/122584>
All reviewed patches have been landed. Closing bug.