[webkit-reviews] review denied: [Bug 91042] Form of FormAssociatedElement is not updated when id target changes. : [Attachment 152159] 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 152159: Patch
https://bugs.webkit.org/attachment.cgi?id=152159&action=review

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