Bug 34924 - Add checks if setNeedsWillValidateCheck() and setNeedsValidityCheck() are called correctly
Summary: Add checks if setNeedsWillValidateCheck() and setNeedsValidityCheck() are cal...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 31716
Blocks: 31718
  Show dependency treegraph
 
Reported: 2010-02-14 05:11 PST by Kent Tamura
Modified: 2010-03-23 04:47 PDT (History)
1 user (show)

See Also:


Attachments
Proposed patch (13.66 KB, patch)
2010-02-14 05:40 PST, Kent Tamura
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2010-02-14 05:11:09 PST
https://bugs.webkit.org/show_bug.cgi?id=31716#c20
> 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.
Comment 1 Kent Tamura 2010-02-14 05:40:27 PST
Created attachment 48721 [details]
Proposed patch
Comment 2 Kent Tamura 2010-02-14 05:47:36 PST
(In reply to comment #0)
> > 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 couldn't apply the single three-state concept.
According to the HTML5, the validity state should work even if willValidate is false. So the patch introduces two boolean members.
Comment 3 Adam Barth 2010-03-22 08:17:54 PDT
Comment on attachment 48721 [details]
Proposed patch

in HTMLInputElement::setValue, it looks like

+ setNeedsValidityCheck();

can be lifted above the "inputType() == FILE" if-clause because its called on both branches.

This looks reasonable.  I'm not an expert on validity checks, but this patch has been sitting around for a month and seems to prove the codebase.
Comment 4 Kent Tamura 2010-03-23 04:04:41 PDT
Thank you for reviewing.

(In reply to comment #3)
> + setNeedsValidityCheck();
> 
> can be lifted above the "inputType() == FILE" if-clause because its called on
> both branches.

setNeedsValidityCehck() should be called
  after updating the value && before style recalc.
So this code is the best.

I'm adding a comment about it, and landing.
Comment 5 Kent Tamura 2010-03-23 04:47:32 PDT
Landed as r56385.