Bug 31716

Created attachment 43571 [details] Proposed patch

I do not understand why all of the setNeedsStyleRecalc() removals are correct. None of these code paths result in style changes which need style recalc?

(In reply to comment #2) > I do not understand why all of the setNeedsStyleRecalc() removals are correct. > None of these code paths result in style changes which need style recalc? They are not removals. They are replaced with willValidateChagned() or updateValidity(). willValidateChanged() always calls setNeedsStyleRecal(), and updateValidity() calls setNeedsStyleRecal() if it is needed.

Comment on attachment 43571 [details] Proposed patch Why is caching this a good idea? It seems like this makes it easier to cause problems in the future by forgetting to call the function that sets m_valid. Is there any benefit to caching this instead of calculating it?

(In reply to comment #4) > (From update of attachment 43571 [details]) > Why is caching this a good idea? It seems like this makes it easier to cause > problems in the future by forgetting to call the function that sets m_valid. Is > there any benefit to caching this instead of calculating it? * validity()->valid(), which is called in isValidFormControl(), can be slow task. It may do regular expression matching, parsing double/ISO8601 strings, etc. * isValidFormControl() can be called multiple times even if the form control state is not changed. If we define :valid and :invalid CSS selectors, checkOneSelector() in CSSStyleSelector.cpp calls isValidFormControl() two times for a form control. I'll add another call of isValidFormControl() to hide a validation message on a value change. I agree that the caching makes it easier to cause problems. But I think it's *slightly* easier. We need to add a call to setNeedsStyleRecalc() at code which can change validity status even without the caching. So the current situation is already easy to cause problems. If the caching was introduced, we would need to add updateValidity() just instead of setNeedsStyleRecalc().

I'll replace setNeedsStyleRecalc() with updateValidity() even if we don't cache the validation result because updateValidity() will manage visibility of a validation error message. e.g. Hide a validation error message when the value become valid.

(In reply to comment #6) > I'll replace setNeedsStyleRecalc() with updateValidity() even if we don't cache > the validation result because updateValidity() will manage visibility of a > validation error message. e.g. Hide a validation error message when the value > become valid. Is updateValidity() the right name for the function? It does that, but it also triggers style recalculation, which is important in some cases even for changes that have no effect on validity. For example, changing the read-only state seems like it could affect style but not validity. It also seems that we should only call setNeedsStyleRecalc() if the validity actually changed. One benefit of caching the validity state is we can detect the case where validity did not actually change, and avoid an unnecessary style recalculation in that case.

Created attachment 44211 [details] Proposed patch (rev.2) > Is updateValidity() the right name for the function? It does that, but it also > triggers style recalculation, which is important in some cases even for changes > that have no effect on validity. I think so if we introduce the caching. Style recalculation is a result of validity status change. The reason why we added setNeedsStyleRecalc() to these code paths was because these paths could change validity status. > For example, changing the read-only state > seems like it could affect style but not validity. Right. So my patch doesn't call updateValidity() for the change of readonly attribute. > It also seems that we should only call setNeedsStyleRecalc() if the validity > actually changed. One benefit of caching the validity state is we can detect > the case where validity did not actually change, and avoid an unnecessary style > recalculation in that case. It's a good idea. The updated patch supports for it, and we need the cahcing to realize it.

style-queue ran check-webkit-style on attachment 44211 [details] without any errors.

(In reply to comment #8) > > Is updateValidity() the right name for the function? It does that, but it also > > triggers style recalculation, which is important in some cases even for changes > > that have no effect on validity. > > I think so if we introduce the caching. Style recalculation is a result of > validity status change. The reason why we added setNeedsStyleRecalc() to these > code paths was because these paths could change validity status. But aren't you also replacing some setNeedsStyleRecalc calls in code paths that previously had them for other reasons?

Created attachment 44282 [details] Proposed patch (rev.3) (In reply to comment #10) > But aren't you also replacing some setNeedsStyleRecalc calls in code paths that > previously had them for other reasons? Ah, yes. min/max attributes for type=range require setNeedsStyleRecalc() regardless validity status. Thank you for pointing out. I split and renamed the method: updateValidityAndStyle() - should be called when validity status can be changed and the appearance should be changed regardless the validity updateValidityAndStyleIfNeeded() - should be called when validity status can be changed. setNeedsStyleRecalc() is called only if the validity status is changed.

Attachment 44282 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/html/HTMLFormControlElement.cpp:115: One line control clauses should not use braces. [whitespace/braces] [4] WebCore/html/HTMLFormControlElement.cpp:323: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 2

Created attachment 44283 [details] Proposed patch (rev.4) (In reply to comment #12) > WebCore/html/HTMLFormControlElement.cpp:115: One line control clauses should > not use braces. [whitespace/braces] [4] > WebCore/html/HTMLFormControlElement.cpp:323: One line control clauses should > not use braces. [whitespace/braces] [4] Fixed.

style-queue ran check-webkit-style on attachment 44283 [details] without any errors.

This patch has been up for review for over 6 weeks. Is there a specific reviewer who has worked in this area before?

I worked on a similar patch for a while, trying to fix a performance regression (http://trac.webkit.org/changeset/53878). I eventually gave up on this approach because of the issues Darin mentioned with needing to make sure you're doing the right kind of update at the right time. With my change in r53878, we no longer call isValidFormControlElement() on all style changes. I'm not sure this caching approach saves us much now. Can you describe of a situation where we'd get some big savings from this?

(In reply to comment #16) > With my change in r53878, we no longer call isValidFormControlElement() on all > style changes. I'm not sure this caching approach saves us much now. > > Can you describe of a situation where we'd get some big savings from this? Right. This change won't improve much performance anymore. The goal is to realize: - Showing a validation error message to a user when an input element becomes invalid - Hiding the validation error message when the element becomes valid or willValidate value becomes false. - Showing/hiding a validation error message when willValidate value becomes true. We need to keep track of validity state changes to implement them. This patch is a preparation for them. It's ok to remove the caching from this patch. It makes the code less complex.

I'm removing the caching part of the patch.

Created attachment 47920 [details] Proposed patch (rev.5)

Comment on attachment 47920 [details] Proposed patch (rev.5) 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

(In reply to comment #20) > (From update of attachment 47920 [details]) > 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. Fixed all of them. > 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. Applied this to parseMappedAttribute(). > 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. I didn't apply it to insertedIntoTree() because to move willValidateChanged() to the correct place doesn't need extra condition checks. > 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. Fixed for removeFromForm() and nameAttr in parseMappedAttribute(), and formDestroyed(). > 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. I'll address them later. Caching the validity state, and adding an assertion in isValidFormControlElment() (like ASSERT(m_valid == validity()->valid())) would be helpful. > > + // 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. Updated comments. > 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". Renamed them. > > 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. Fixed. > 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. Fixed for setInputType(). > r=me, please consider my comments about the completeness and design

Landed as r54274