Created attachment 67422 [details] test case (may crash) See attached test case.
Created attachment 67493 [details] Patch
Comment on attachment 67493 [details] Patch Why do we need to call setNeedsValidityCheck() in both functions? One of these always calls another. If we do need that, it may be worth a comment in code, or at the very least, in ChangeLog.
Thank you for the feedback. (In reply to comment #2) > (From update of attachment 67493 [details]) > Why do we need to call setNeedsValidityCheck() in both functions? One of these always calls another. > > If we do need that, it may be worth a comment in code, or at the very least, in ChangeLog. setNonDirtyValue() is called by other functions. We need to call setNeedsValidityCheck() in that case. I'll improve the code to reduce redundant calls of setNeedsValidityCheck().
Created attachment 67754 [details] Patch 2 (resolve redundant setNeedsValidityCheck())
> setNonDirtyValue() is called by other functions. OK, but why wouldn't it be sufficient to only have setNeedsValidityCheck() in setNonDirtyValue()? Since setValue() calls it, it doesn't seem to also call the same function. Talking about the new patch - this is probably a related question, but why can't setNeedsValidityCheck() be in setValueCommon()? Does its behavior depend on m_isDirty value? It doesn't use it explicitly, and if an indirect use is the reason not to put this call in setValueCommon(), then a comment should explain that (and setNeedsValidityCheck() probably needs a better name, since it does something different than setting a "needsValidityCheck" boolean). Both these patches confuse me, but it seems that a larger refactoring may be needed. If so, the first one might actually be better as a targeted fix, as it's simpler and doesn't add a new method.
(In reply to comment #5) > > setNonDirtyValue() is called by other functions. > > OK, but why wouldn't it be sufficient to only have setNeedsValidityCheck() in setNonDirtyValue()? Since setValue() calls it, it doesn't seem to also call the same function. > > but why can't setNeedsValidityCheck() be in setValueCommon()? Does its behavior depend on m_isDirty value? Right. setNeedsValidityCheck() result depends on m_isDirty value. The name "setNeedsValidityCheck" doesn't represent the current behavior. The role of setNeedsValdityCheck was to set "needs to recalculate element validity", and the name followed "setNeedsStyleRecalc". But we need to calculate validity in setNeedsValidityCheck for some reasons. I don't think we'll have a large refactor for needsValidityCheck, but we should rename it.
Comment on attachment 67754 [details] Patch 2 (resolve redundant setNeedsValidityCheck()) OK. So, do you prefer the old or the new patch yourself? Given that you kept the new one for review, I guess that's it, so I'll mark it r+. > ta.checkValidity(); // This made an assertion failure. "This made an assertion fail".
(In reply to comment #7) > (From update of attachment 67754 [details]) > OK. So, do you prefer the old or the new patch yourself? Given that you kept the new one for review, I guess that's it, so I'll mark it r+. Yeah, I prefer the second patch. Thanks for r+. > > ta.checkValidity(); // This made an assertion failure. > > "This made an assertion fail". I'll fix it before landing.
Landed as r68494.