Bug 91042 – Form of FormAssociatedElement is not updated when id target changes.

• Component: Forms
• Status: RESOLVED FIXED

[Keishi Hattori]

When the element that the form attribute is pointing to has changed, it should update the form in FormAssociatedElement.

[Keishi Hattori]

Created attachment 151845 [details]
Patch

[Kent Tamura]

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()?

[Kent Tamura]

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().

[Keishi Hattori]

Created attachment 151928 [details]
Patch

[Keishi Hattori]

Created attachment 151942 [details]
Patch

[Keishi Hattori]

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.

[Kent Tamura]

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);

[Kent Tamura]

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".

[Keishi Hattori]

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.

[Keishi Hattori]

Created attachment 152159 [details]
Patch

[Kent Tamura]

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?

[Keishi Hattori]

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.

[Keishi Hattori]

Created attachment 152174 [details]
Patch

[Build Bot]

Comment on attachment 152174 [details]
Patch

Attachment 152174 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13235041

[Kent Tamura]

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.

[Keishi Hattori]

Created attachment 152181 [details]
Patch

[Kent Tamura]

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.

[Build Bot]

Comment on attachment 152181 [details]
Patch

Attachment 152181 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13221558

[Keishi Hattori]

Created attachment 152190 [details]
Patch

[Kent Tamura]

Comment on attachment 152190 [details]
Patch

Looks ok.  Please wait until all EWSes get green.

[WebKit Review Bot]

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

[Keishi Hattori]

Created attachment 152250 [details]
Patch

[WebKit Review Bot]

Comment on attachment 152250 [details]
Patch

Clearing flags on attachment: 152250

Committed r122584: <http://trac.webkit.org/changeset/122584>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.