[webkit-reviews] review denied: [Bug 91042] Form of FormAssociatedElement is not updated when id target changes. : [Attachment 151942] Patch

[bugzilla-daemon at webkit.org]

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 151942: Patch
https://bugs.webkit.org/attachment.cgi?id=151942&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
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);