[webkit-reviews] review granted: [Bug 31716] Fix a bug that changes for some constraint attributes doesn't update validation CSS selectors. : [Attachment 47920] Proposed patch (rev.5)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted TAMURA, Kent <tkent at chromium.org>'s
request for review:
Bug 31716: Fix a bug that changes for some constraint attributes doesn't update
validation CSS selectors.
https://bugs.webkit.org/show_bug.cgi?id=31716

Attachment 47920: Proposed patch (rev.5)
https://bugs.webkit.org/attachment.cgi?id=47920&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
First, some comments about the HTMLFormControlElement::willValidate function,
not changed in this patch. I notice it calls isReadOnlyFormControl. When we
already have an HTMLFormControlElement pointer, this is unnecessarily
inefficient because it's a virtual function call. The efficient way to check is
to check m_readOnly directly. The value added by isReadOnlyFormControl is a
type check. I think it would also be better to use m_disabled than the disabled
function. Perhaps the disabled function should also be made inline.

> -	       setNeedsStyleRecalc();
> +	       willValidateChanged();

Probably would be better to only do style recalculation if it's needed.

One way to accomplish that would be to use a different idiom. We could call
willValidate both before and after making changes and only call
willValidateChanged if the value of willValidate actually changed. I'm assuming
that an otherwise-unnecessary style recalculation is expensive and worth
avoiding.

The insertedIntoTree function now calls willValidateChanged even if m_form was
zero and remains zero, and for disabled or read-only controls. That could all
be avoided with the suggestion above.

As far as the task of adding calls to willValidateChanged in
HTMLFormControlElement is concerned, I see code that handles changes to
"m_disabled" and "m_readOnly" and where "m_form" becomes non-zero, but I do not
see code that handles the case where "m_form" becomes zero in removeFromForm or
that handles changes to nameAttr in case the name has become empty/non-empty.

It almost seems that despite our bad experiences, caching the old value would
be helpful, not for optimizing the result of the functions to fetch the
validity state, but for getting the state changes correct. It's quite fragile
to both have functions to answer "is this valid" and then scattered code to
invalidate when those constraints change. How would someone changing the
functions know to go update the call sites where those states could change.

How would notice if we forgot to call one of these state change functions?
Maybe we need to devise a debugging feature to catch mistakes.

If we do cache a value I think it should be a single three-state concept: "will
not validate", "valid", "invalid". Treating these as independent makes the code
more complicated in places where they overlap. I know the DOM API for this
treats them separately, but the style code would be much simpler if it used a
single three-state concept.

> +    // This should be called when a validation constraint or control value
is
> +    // changed. This requests style reculculation.

Typo here: "reculculation". I think you could leave out the second sentence.
And I would say "must" instead of "should".

> +    // This should be called if willValidate() result can be changed.
> +    void willValidateChanged();

I think the comment needs to be more specific. The phrase "can be changed" here
is unclear. You probably mean something like "may have changed".

A comment more like this: "This must be called any time the result of
willValidate() has changed."

But that still is not helpful for someone making code changes on this class.
Another sentence could explain, "Code that changes values that are inspected in
the wilLValidate function has to call this." I'm not sure my comments are
great, but they might be a bit better.

I think the name "validity changed" is not ideal, because the function is
currently called in lots of cases when the validity has not changed. The code
must call it when something has changed that affects the validity computation,
but is allowed to call it extra times. That fuzziness is annoying.
Possibly-better name for this type of function are "setNeedsValidityCheck" and
"setNeedsWillValidateCheck".

It's worth thinking out how to explain the concept in plain language and then
converting that to a function name rather than using a function name that comes
from other function names.

>	   dispatchFormControlChangeEvent();
>  
>      InputElement::notifyFormStateChanged(this);
> -    updateValidity();
> +    validityChanged();

To me it seems this is a little too late to call validityChanged. Some day in
the future we might want to make sure this is called before anyone checks
validity, so it would be best to call it before dispatching an event. In
general, I think these calls should be as close to the change as possible
before other sorts of notifications, even though at this time the only thing
these functions do is call setNeedsStyleRecalc.

In theory, there should be a willValidateChanged call as well as a
validityChanged call in HTMLInputElement::setInputType. If we are really going
to call it any time the state of "will validate" may have changed. That's one
of the disadvantages of treating these two concepts separately instead of
having a single three-state concept.

r=me, please consider my comments about the completeness and design