[webkit-reviews] review denied: [Bug 58837] Fieldset disabled attribute does not work : [Attachment 125164] patch for review / feedback.

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Zeno Albisser <zeno at webkit.org>'s
request for review:
Bug 58837: Fieldset disabled attribute does not work
https://bugs.webkit.org/show_bug.cgi?id=58837

Attachment 125164: patch for review / feedback.
https://bugs.webkit.org/attachment.cgi?id=125164&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=125164&action=review



> LayoutTests/fast/forms/fieldset-disabled.html:10
> +<script src="script-tests/fieldset-disabled.js"></script>

Do not separate the .js file from the HTML file.

> LayoutTests/fast/forms/script-tests/fieldset-disabled.js:10
> +// Enabled by default, user can insertText into the text input field.

Such test information should be shown in the test result.
  debug('Fieldset is enabled by default. A user can insert text into the text
input field:');

> LayoutTests/fast/forms/script-tests/fieldset-disabled.js:44
> +// Disable fieldSet.
> +fieldSet.disabled = false;

Disable -> Enable

> LayoutTests/fast/forms/script-tests/fieldset-disabled.js:91
> +// Add 3000 text input for performance test.
> +for (var i = 0; i < 3000; i++) {
> +  var newTextInput = document.createElement('input');
> +  newTextInput.type = "text";
> +  td.appendChild(newTextInput);
> +}

You shouldn't add performance test here.  If you need to add it, it should be
in LayoutTests/perf or PerformanceTests/

> Source/WebCore/html/HTMLFieldSetElement.cpp:67
> +void HTMLFieldSetElement::attributeChanged(Attribute* attribute, bool
preserveDecls)
> +{
> +    HTMLFormControlElement::attributeChanged(attribute, preserveDecls);
> +    if (attribute->name() == disabledAttr) {
> +	   for (size_t i = 0; i < m_subordinatedFormControlElements.size();
i++) {
> +	       HTMLFormControlElement* control =
m_subordinatedFormControlElements.at(i);
> +	       control->setParentDisabled(disabled());
> +	   }
> +    }
> +}

We usually use parseMappedAttribute() instead of attributeChanged().

We need to call setNeedsStyleRecalc() for each of controls to apply
:disabled/:enabled style.

> Source/WebCore/html/HTMLFieldSetElement.h:48
> +    Vector<HTMLFormControlElement*> m_subordinatedFormControlElements;

I don't think we need to keep track of descendant form controls.

We need to call setNeedsStyleRecalc() for descendant form controls when
disabled attribute is change. I think it's ok to traverse all of descendant
nodes and we don't need to care about the performance of updating
HTMLFieldSetElement::disabled at this moment because this cost is not a
regression.

> Source/WebCore/html/HTMLFormControlElement.cpp:107
> +    HTMLFieldSetElement* fieldSet = findFieldSetAncestor();

I'm worry about the performance of this.  findFieldSetAncestor() is O(depth). 
This will be a performance regression even if a document has no <fieldset>
elements.

> Source/WebCore/html/HTMLFormControlElement.h:66
> +    virtual bool disabled() const { return m_disabled || m_parentDisabled; }


Because we have HTMLFormControlElement::m_fieldSetAncestor, we don't need to
have m_parentDisabled at all.  return m_disabled || (m_fieldSetAncestor &&
m_fieldSetAncestor->disabled()) is enough.