[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