[webkit-reviews] review denied: [Bug 91042] Form of FormAssociatedElement is not updated when id target changes. : [Attachment 151845] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jul 11 20:55:06 PDT 2012
Kent Tamura <tkent at chromium.org> has denied Keishi Hattori
<keishi at webkit.org>'s request for review:
Bug 91042: Form of FormAssociatedElement is not updated when id target changes.
https://bugs.webkit.org/show_bug.cgi?id=91042
Attachment 151845: Patch
https://bugs.webkit.org/attachment.cgi?id=151845&action=review
------- Additional Comments from Kent Tamura <tkent at chromium.org>
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->fastGetA
ttribute(formAttr), this);
> setForm(0);
Isn't it removeObserver()?
More information about the webkit-reviews
mailing list