1375599 - Consider caching nsGenericHTMLFormElement::IsDisabled() in a boolean bit flag

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[(no longer active)]

See this profile for example: http://bit.ly/2t0mKqg

There are a couple of options for finding more flag free space:

http://searchfox.org/mozilla-central/rev/ae94cfb36d804727dfb38f2750bfcaab4621df6e/dom/html/nsGenericHTMLElement.h#1196-1198
And bug 1338723

Comment 1

[Boris Zbarsky [:bzbarsky]]

So some thoughts.

First of all, if this is only called for cases when CanBeDisabled() is true, then we can use the existing NS_EVENT_STATE_DISABLED flag for this.  Right now this is updated quite late in the attr-setting process (by the final UpdateState() call, basically), but we could do it more eagerly like we're handling the "dir" attribute.  Basically, we need to make sure we update it in AfterSetAttr before any of the consumers of IsDisabled() in AfterSetAttr run.

If this is called for cases when !CanBeDisabled(), then we need a separate boolean flag.  Luckily I just freed up a few on nsINode, so we can in fact have one if needed.

Comment 2

[Jessica Jong [:jessica] (inactive)]

(In reply to Boris Zbarsky [:bz] from comment #1)
> So some thoughts.
> 
> First of all, if this is only called for cases when CanBeDisabled() is true,
> then we can use the existing NS_EVENT_STATE_DISABLED flag for this.  Right
> now this is updated quite late in the attr-setting process (by the final
> UpdateState() call, basically), but we could do it more eagerly like we're
> handling the "dir" attribute.  Basically, we need to make sure we update it
> in AfterSetAttr before any of the consumers of IsDisabled() in AfterSetAttr
> run.
> 
> If this is called for cases when !CanBeDisabled(), then we need a separate
> boolean flag.  Luckily I just freed up a few on nsINode, so we can in fact
> have one if needed.

I think nsGenericHTMLFormElement::IsDisabled() is only called for cases when CanBeDisabled() is true, since form elements that can not be disabled - output and object, have overridden IsDisabled() to return false. That means, we are safe to use NS_EVENT_STATE_DISABLED flag, right?

Updating NS_EVENT_STATE_DISABLED flag in nsGenericHTMLFormElement::AfterSetAttr() will still be too late since it's called in the last part of HTMLInputElement::AfterSetAttr(), and there are already consumers of IsDisabled() in that function. So, we will need to update the flag at the beginning of each of the derived class' AfterSetAttr(). Does that sound okay?

Comment 3

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v1.

Hi bz, could you take a look to see if this is the right way? Thanks.
Details are in the commit message.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8886747 [details] [diff] [review]
patch, v1.

Jessica, I'm sorry for the lag...

Some thoughts:

1)  I think some of the bits from comment 2 about why it's safe to use the flag instead of calling IsDisabled() should probably be in the commit message.  Specifically the part about how the flag is set by nsGenericHTMLFormElement::IntrinsicState if and only if IsDisabled() in all cases when CanBeDisabled() is true, and when CanBeDisabled() is false then IsDisabled() is always false and the flag not set, if the element is nsGenericHTMLFormElement.

For non-form-elements, <optgroup> has the flag matching IsDisabled(), but looks like <option> doesn't: IsDisabled() just considers the option, but the flag considers the optgroup.  That's arguably a bug, actually, but may not be relevant to us here if we're not touching the IsDisabled() callers outside nsGenericHTMLFormElement::IntrinsicState.

2) It looks like HTMLLabelElement::IsDisabled can just be removed, right?

>+++ b/dom/html/HTMLButtonElement.cpp

Should probably document that nsGenericHTMLFormElementWithState::FieldSetDisabledChanged needs to be called before UpdateBarredFromConstraintValidation, because the latter uses IsDisabled().  Similar in the other FieldSetDisabledChanged methods.

>+        // This *has* to be called *before* validity state check, as it may make

It's not clear what "it" is.  How about: "because UpdateBarredFromConstraintValidation depends on our disabled state"?

>+++ b/dom/html/HTMLFieldSetElement.cpp
>+    // This *has* to be called *before* validity state check, as it may make use

Which "it"?  What validity state check?  There is no validity state check here...

I assume this should talk about how this should happen before we call FieldSetDisabledChanged() on our controls.

>+++ b/dom/html/HTMLInputElement.cpp
>+        // This *has* to be called *before* validity state check, as it may make

Again, ambiguous "it".

> HTMLInputElement::FieldSetDisabledChanged(bool aNotify)

Needs a comment like for <button>.

>+++ b/dom/html/HTMLOptGroupElement.cpp
>+    if (aNotify) {
>+      AddStates(aValue ? NS_EVENT_STATE_DISABLED : NS_EVENT_STATE_ENABLED);
>+      RemoveStates(aValue ? NS_EVENT_STATE_ENABLED
>+                          : NS_EVENT_STATE_DISABLED);

That doesn't seem right to me. What if we already had a "disabled" attr and we're just changing its value, so our NS_EVENT_STATE_DISABLED state is NOT changing?  This will notify a change for it.

It would be better if we did something where we figure out which states aer changing and then use ToggleStates as needed.

>+++ b/dom/html/HTMLOptionElement.cpp
>+HTMLOptionElement::UpdateDisabledState(bool aNotify)
>+  bool isDisabled =
>+    (HasAttr(kNameSpaceID_None, nsGkAtoms::disabled) ? true : false);

How about:

  bool isDisabled = HasAttr(kNameSpaceID_None, nsGkAtoms::disabled);

?

>+  nsIContent* parent = GetParent();

Should only do this whole block if !isDisabled, right?  And then you can avoid doing:

    isDisabled = isDisabled || optGroupElement->IsDisabled();

and just do:

    isDisabled = optGroupElement->IsDisabled();

>+    AddStates(isDisabled ? NS_EVENT_STATE_DISABLED : NS_EVENT_STATE_ENABLED);

This has the same problem as for optgroup.  Again, we want to figure out what states, if any, actually changed, then ToggleStates.

>+++ b/dom/html/HTMLOptionElement.h

Please document the new methods here.

>+++ b/dom/html/HTMLSelectElement.cpp
>+      // This *has* to be called *before* validity state check, as it may make

Ambiguous "it".

>HTMLSelectElement::FieldSetDisabledChanged(bool aNotify)

Needs comments, as above.

>+++ b/dom/html/HTMLTextAreaElement.cpp
>+        // This *has* to be called *before* validity state check, as it may make

Ambiguous "it".

>HTMLTextAreaElement::FieldSetDisabledChanged(bool aNotify)

Needs comments.

>+++ b/dom/html/nsGenericHTMLElement.cpp
> nsGenericHTMLFormElement::IsDisabled() const
>+  return State().HasState(NS_EVENT_STATE_DISABLED) ||
>          (mFieldSet && mFieldSet->IsDisabled());

Shouldn't need to look at mFieldSet, since mFieldSet is already considered when determining the state, no?

>+void nsGenericHTMLFormElement::UpdateDisabledState(bool aNotify)
>+  bool isDisabled =
>+    (HasAttr(kNameSpaceID_None, nsGkAtoms::disabled) ? true : false);

  bool isDisabled = HasAttr(kNameSpaceID_None, nsGkAtoms::disabled);

>+    AddStates(isDisabled ? NS_EVENT_STATE_DISABLED : NS_EVENT_STATE_ENABLED);

As above, this is wrong.  Please use TooggleStates.

The rest looks good to me, but please fix the above issues.

Comment 5

[Jessica Jong [:jessica] (inactive)]

(In reply to Boris Zbarsky [:bz] from comment #4)
> Comment on attachment 8886747 [details] [diff] [review]
> patch, v1.
> 
> Jessica, I'm sorry for the lag...

Thanks for the thorough review!

> 
> Some thoughts:
> 
> 1)  I think some of the bits from comment 2 about why it's safe to use the
> flag instead of calling IsDisabled() should probably be in the commit
> message.  Specifically the part about how the flag is set by
> nsGenericHTMLFormElement::IntrinsicState if and only if IsDisabled() in all
> cases when CanBeDisabled() is true, and when CanBeDisabled() is false then
> IsDisabled() is always false and the flag not set, if the element is
> nsGenericHTMLFormElement.

Will update the commit message as suggested.

> 
> For non-form-elements, <optgroup> has the flag matching IsDisabled(), but
> looks like <option> doesn't: IsDisabled() just considers the option, but the
> flag considers the optgroup.  That's arguably a bug, actually, but may not
> be relevant to us here if we're not touching the IsDisabled() callers
> outside nsGenericHTMLFormElement::IntrinsicState.

I think this bug will be gone when we change IsDisabled() to look at the NS_EVENT_STATE_DISABLED instead, since option's NS_EVENT_STATE_DISABLED will be updated whenever optgroup's disabled state changes.

> 
> 2) It looks like HTMLLabelElement::IsDisabled can just be removed, right?

Yep, it inherits from nsGenericHTMLElement, which already returns false in IsDisabled().

> 
> >+++ b/dom/html/HTMLButtonElement.cpp
> 
> Should probably document that
> nsGenericHTMLFormElementWithState::FieldSetDisabledChanged needs to be
> called before UpdateBarredFromConstraintValidation, because the latter uses
> IsDisabled().  Similar in the other FieldSetDisabledChanged methods.
> 
> >+        // This *has* to be called *before* validity state check, as it may make
> 
> It's not clear what "it" is.  How about: "because
> UpdateBarredFromConstraintValidation depends on our disabled state"?
> 
> >+++ b/dom/html/HTMLFieldSetElement.cpp
> >+    // This *has* to be called *before* validity state check, as it may make use
> 
> Which "it"?  What validity state check?  There is no validity state check
> here...
> 
> I assume this should talk about how this should happen before we call
> FieldSetDisabledChanged() on our controls.
> 
> >+++ b/dom/html/HTMLInputElement.cpp
> >+        // This *has* to be called *before* validity state check, as it may make
> 
> Again, ambiguous "it".
> 
> > HTMLInputElement::FieldSetDisabledChanged(bool aNotify)
> 
> Needs a comment like for <button>.
> 
> >+++ b/dom/html/HTMLOptGroupElement.cpp
> >+    if (aNotify) {
> >+      AddStates(aValue ? NS_EVENT_STATE_DISABLED : NS_EVENT_STATE_ENABLED);
> >+      RemoveStates(aValue ? NS_EVENT_STATE_ENABLED
> >+                          : NS_EVENT_STATE_DISABLED);
> 
> That doesn't seem right to me. What if we already had a "disabled" attr and
> we're just changing its value, so our NS_EVENT_STATE_DISABLED state is NOT
> changing?  This will notify a change for it.
> 
> It would be better if we did something where we figure out which states aer
> changing and then use ToggleStates as needed.

Will change to use ToggleStates instead.

> 
> >+++ b/dom/html/HTMLOptionElement.cpp
> >+HTMLOptionElement::UpdateDisabledState(bool aNotify)
> >+  bool isDisabled =
> >+    (HasAttr(kNameSpaceID_None, nsGkAtoms::disabled) ? true : false);
> 
> How about:
> 
>   bool isDisabled = HasAttr(kNameSpaceID_None, nsGkAtoms::disabled);

Looks better, thanks.

> 
> ?
> 
> >+  nsIContent* parent = GetParent();
> 
> Should only do this whole block if !isDisabled, right?  And then you can
> avoid doing:
> 
>     isDisabled = isDisabled || optGroupElement->IsDisabled();
> 
> and just do:
> 
>     isDisabled = optGroupElement->IsDisabled();

Ok, will do.

> 
> >+    AddStates(isDisabled ? NS_EVENT_STATE_DISABLED : NS_EVENT_STATE_ENABLED);
> 
> This has the same problem as for optgroup.  Again, we want to figure out
> what states, if any, actually changed, then ToggleStates.
> 
> >+++ b/dom/html/HTMLOptionElement.h
> 
> Please document the new methods here.
> 
> >+++ b/dom/html/HTMLSelectElement.cpp
> >+      // This *has* to be called *before* validity state check, as it may make
> 
> Ambiguous "it".
> 
> >HTMLSelectElement::FieldSetDisabledChanged(bool aNotify)
> 
> Needs comments, as above.
> 
> >+++ b/dom/html/HTMLTextAreaElement.cpp
> >+        // This *has* to be called *before* validity state check, as it may make
> 
> Ambiguous "it".
> 
> >HTMLTextAreaElement::FieldSetDisabledChanged(bool aNotify)
> 
> Needs comments.
> 
> >+++ b/dom/html/nsGenericHTMLElement.cpp
> > nsGenericHTMLFormElement::IsDisabled() const
> >+  return State().HasState(NS_EVENT_STATE_DISABLED) ||
> >          (mFieldSet && mFieldSet->IsDisabled());
> 
> Shouldn't need to look at mFieldSet, since mFieldSet is already considered
> when determining the state, no?

Right, no need to look at mFieldSet anymore.

> 
> >+void nsGenericHTMLFormElement::UpdateDisabledState(bool aNotify)
> >+  bool isDisabled =
> >+    (HasAttr(kNameSpaceID_None, nsGkAtoms::disabled) ? true : false);
> 
>   bool isDisabled = HasAttr(kNameSpaceID_None, nsGkAtoms::disabled);
> 
> >+    AddStates(isDisabled ? NS_EVENT_STATE_DISABLED : NS_EVENT_STATE_ENABLED);
> 
> As above, this is wrong.  Please use TooggleStates.
> 
> The rest looks good to me, but please fix the above issues.

Thanks again. Will also update the comments as suggested.

Comment 6

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v2.

Addressed review comment 4:
- Refine commit message
- Refine code comments
- Use ToggleStates, when needed, intead of Add/RemoveStates
- Simplify some part of the code without changing the logic

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8887846 [details] [diff] [review]
patch, v2.

>Bug 1375599 - Change IsDisabled() to look at NS_EVENT_STATE_DISABLED instead. r=bz

 ... instead of the "disabled" attribute.

>It is safe to use the NS_EVENT_STATE_DISABLED flag due to following reasons:

"flag for the following reasons"

>  form elements that can be disabled, form elements that can't be disabled

Should have ';' instead of ',' there.

>  IsDisabled(). Note that, option's IsDisabled() should also refer to

No ',' after "that".

>We also need to update the flag whenever its parent (e.g. fieldset or optgroup)

s/its/the element's/

>disable state changes and when moving into/out of a parent container.

s/disable/disabled/.

>Note that, NS_EVENT_STATE_DISABLED/ENABLED is now part of the

No ',' after "that".

>+++ b/dom/html/HTMLButtonElement.cpp
>+  // This *has* to be called *before* UpdateBarredFromConstraintValidation
>+  // because the latter depends on our disabled state.

There's no "the latter" here.  In comment 4 there was, because I was talking about two different function names and needed to specify which one I was discussing.

How about "because that function"?  Or alternately spell out what "this" is, and then "the latter" makes sense, like so:

  FieldSetDisabledChanged *has* to be called *before* UpdateBarredFromConstraintValidation,
  because the latter depends on our disabled state.

>+++ b/dom/html/HTMLOptGroupElement.cpp
> HTMLOptGroupElement::AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
>+      disabledStates &= ~NS_EVENT_STATE_ENABLED;

Why do we need this?  disabledStates started out empty, so this is a no-op.  Please take it, and the other &= bit in the "else" branch out.

>+    EventStates oldDisabledStates =
>+      State() & (NS_EVENT_STATE_DISABLED | NS_EVENT_STATE_ENABLED);

It might be nice to have a named constant in nsEventStates.h for (NS_EVENT_STATE_DISABLED | NS_EVENT_STATE_ENABLED) and use it both here and in that file...

These two comments (about the &= bit and the named constant) apply to the other places where you have this code too.

>+        if (child->IsHTMLElement(nsGkAtoms::option)) {
>+          HTMLOptionElement* optElement = HTMLOptionElement::FromContent(child);
>+          optElement->OptGroupDisabledChanged(true);

I missed this the first time around, but this is doing the "is this an option?" check twice.  You can just do:

  if (auto optElement = HTMLOptionElement::FromContent(child)) {
    optElement->OptGroupDisabledChanged(true);
  }

>+++ b/dom/html/HTMLOptionElement.cpp
>+HTMLOptionElement::UpdateDisabledState(bool aNotify)
>+    nsIContent* parent = GetParent();
>+    if (parent && parent->IsHTMLElement(nsGkAtoms::optgroup)) {
>+      HTMLOptGroupElement* optGroupElement =
>+        HTMLOptGroupElement::FromContent(parent);
>+      isDisabled = optGroupElement->IsDisabled();
>+    }

Again, this is doing the "is this an optgroup?" check twice.  More simply:

  if (auto optGroupElement = HTMLOptGroupElement::FromContentOrNull(GetParent())) {
    isDisabled = optGroupElement->IsDisabled();
  }

or with GetParent() first stored into a "parent" arg, either way.  But use FromContentOrNull instead of reimplementing it.  ;)

>+++ b/dom/html/HTMLOptionElement.h
>+   * This callback is called by a optgroup on all its option elements whenever

"by an optgroup"

>+   * Check our disabled content attribute and optgroup's (if exists) disabled

"(if it exists)"

>+++ b/dom/html/nsGenericHTMLElement.h
>+   * Check our disabled content attribute and fieldset's (if exists) disabled

"(if it exists)"

r=me.  Thank you for doing this!

Comment 8

[Jessica Jong [:jessica] (inactive)]

(In reply to Boris Zbarsky [:bz] from comment #7)
> >+++ b/dom/html/HTMLOptGroupElement.cpp
> > HTMLOptGroupElement::AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
> >+      disabledStates &= ~NS_EVENT_STATE_ENABLED;
> 
> Why do we need this?  disabledStates started out empty, so this is a no-op. 
> Please take it, and the other &= bit in the "else" branch out.

Ah, good catch! will fix it in the new patch.

> 
> >+    EventStates oldDisabledStates =
> >+      State() & (NS_EVENT_STATE_DISABLED | NS_EVENT_STATE_ENABLED);
> 
> It might be nice to have a named constant in nsEventStates.h for
> (NS_EVENT_STATE_DISABLED | NS_EVENT_STATE_ENABLED) and use it both here and
> in that file...
> 
> These two comments (about the &= bit and the named constant) apply to the
> other places where you have this code too.
> 
> >+        if (child->IsHTMLElement(nsGkAtoms::option)) {
> >+          HTMLOptionElement* optElement = HTMLOptionElement::FromContent(child);
> >+          optElement->OptGroupDisabledChanged(true);
> 
> I missed this the first time around, but this is doing the "is this an
> option?" check twice.  You can just do:
> 
>   if (auto optElement = HTMLOptionElement::FromContent(child)) {
>     optElement->OptGroupDisabledChanged(true);
>   }
> 
> >+++ b/dom/html/HTMLOptionElement.cpp
> >+HTMLOptionElement::UpdateDisabledState(bool aNotify)
> >+    nsIContent* parent = GetParent();
> >+    if (parent && parent->IsHTMLElement(nsGkAtoms::optgroup)) {
> >+      HTMLOptGroupElement* optGroupElement =
> >+        HTMLOptGroupElement::FromContent(parent);
> >+      isDisabled = optGroupElement->IsDisabled();
> >+    }
> 
> Again, this is doing the "is this an optgroup?" check twice.  More simply:
> 
>   if (auto optGroupElement =
> HTMLOptGroupElement::FromContentOrNull(GetParent())) {
>     isDisabled = optGroupElement->IsDisabled();
>   }
> 
> or with GetParent() first stored into a "parent" arg, either way.  But use
> FromContentOrNull instead of reimplementing it.  ;)

Will fix this and the one above.

> 
> r=me.  Thank you for doing this!

Thanks for the review and for fixing my endless writing errors ;)

Comment 9

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v3.

Addressed review comment 7:
- no need to clear disabled/enabled flags in the temporary EventStates since the it starts out empty
- avoid checking twice "is this an option?" and "is this an optgroup?" part
- writing errors

Carrying r+.

Comment 10

[Jessica Jong [:jessica] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e5c3a9e7944b9dc6564c85a62838006cfe2a696&group_state=expanded

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c72f79ecb0dd
Change IsDisabled() to look at NS_EVENT_STATE_DISABLED instead of the "disabled" attribute. r=bz

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/c72f79ecb0dd