Bug 19562

Created attachment 21716 [details] ValidityState stub class and related stuff

Comment on attachment 21716 [details] ValidityState stub class and related stuff In general, this looks fine. I'm sad that %baseTypeHash still exists (but that's not your fault). That information needs to move into .in or .idl files. This needs your copyright instead of Apple's :) + // ValidityState objects are always bound to their controls at creation time isn't really needed. The ASSERT says the same thing. So is there any way a ValidityState could outlive its associated form control? If so, its going to end up with a bad pointer to a deleted form control. This would be clearer: + return (!(typeMismatch() || stepMismatch() || rangeUnderflow() || rangeOverflow() || + tooLong() || patternMismatch() || valueMissing() || customError())); Written in two parts: + bool formHasError = typeMismatch() || stepMismatch() || rangeUnderflow() || rangeOverflow() || + tooLong() || patternMismatch() || valueMissing() || customError(); + return !formHasError; The use of the extra variable name makes the long boolean expression much clearer to read. + static PassRefPtr<ValidityState> create(HTMLFormControlElement* owner = 0) Should not have =0 if the form control is required. That makes it a compile time error to call HTMLFormControlElement::create() w/o arguments instead of a runtime error. r- for the possible crasher (and for the =0 in the create call). How about adding a minimal js test case which tests for the existence of this interface? Hum...I guess there is no way to get at it yet (you haven't added a ValidityState accessor onto form controls.

(In reply to comment #2) > I'm sad that %baseTypeHash still exists (but that's not your fault). That > information needs to move into .in or .idl files. You can ignore that comment. It is a general comment about mis-design in our bindings system, not about your current patch.

Created attachment 22658 [details] ValidityState stub class and related stuff, second revision

Comment on attachment 22658 [details] ValidityState stub class and related stuff, second revision There are several instances of tabs instead of spaces. I also don't think we should allocate a ValidityState object for every form control. They should be created on demand. Your test case would be better if it printed the type of form control being tested: +Form Control #1 ValidityState exists - SUCCESS +Form Control #1 customError +Form Control #1 stepMismatch +Form Control #1 valid +Form Control #1 rangeUnderflow And you only actually need to print on PASS or fail for each type of form control. Not one line for each validity state atribute. also, all tests like this should use the fast/js .js template system. See examples in fast/js and other places where the entire test case is in javascript and the wrapper html file is autogenerated. Also, your test case is missing a newline at the end of the file: +<ol id="console"></ol> +</body> +</html> \ No newline at end of file diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog r- for the memory issue. These should be allocated on demand. This is one of numerous examples of the fast/js javascript test template system: http://trac.webkit.org/browser/trunk/LayoutTests/fast/dom/DOMException/resources/EventException.js http://trac.webkit.org/browser/trunk/LayoutTests/fast/dom/Document/resources/createElementNS-namespace-err.js http://trac.webkit.org/browser/trunk/LayoutTests/fast/dom/Document/resources/TEMPLATE.html

Created attachment 22797 [details] ValidityState stub class and related stuff, third revision VS is now allocated on request.

Adele, what should we do with this patch? Do we want a non-working validity object in the tree? To land this we'd need to make a version of the patch that adds the new source files into all the various project files. Should we review- this patch, or fix it and land it, or what?

Comment on attachment 22797 [details] ValidityState stub class and related stuff, third revision We need to fix this up. r- for now.

We're working on required and pattern attribute, which should be quite a piece of cake to get up and running once we have VS checked into the tree. Each validation check should be made in ValidityState code in my vision, so - given that VS blocks both of them - we have to focus to get this code landed asap. What is your opinion Peter? Should we abort r? on #25551?

Are you saying you cannot land the patch on bug 25551 without landing a patch on here first? Or is the patch on bug 25551 independent? If this bug blocks your outstanding patch, you need to make that clear on that patch, and I would suggest canceling r? until something lands here. If not, go ahead and maintain the review request there. For this bug, if you have a clear understanding of what Darin/Adele would like to see based on their comments above, then go ahead and address them. It doesn't sound like anyone has raised major architectural concerns so far. If you don't understand what the fixup desired above is, or if other patches elsewhere don't provide it, then you should probably contact Darin/Adele on IRC or by email and get a plan worked out.

(In reply to comment #10) > Are you saying you cannot land the patch on bug 25551 without landing a patch > on here first? Or is the patch on bug 25551 independent? If we land 25551 and 25552 before this one we'd have to add some code at a later time to add "concrete" validation behaviour and make both of them work with ValidityState (at least another patch to "merge" requiredAttr/patternAttr code with ValidityState and get everything to function the way it should); 4 patches expected. On the other hand, if we land this one as first, we could manage to get patternAttr and requiredAttr work in one shot. 3 patches expected. I need to figure out how the "sweet" build system for the DOM bindings works...:-D

Created attachment 31493 [details] Clean and functioning patch. Here it is. Waiting for comments before asking r?.

Created attachment 31494 [details] Clean and functioning patch (corrected).

In WebCore/ChangeLog, the bug link is usually just above the description (see other comments there). In the comments you might want a link to the relevant HTML5 spec section. In ValidityState.cpp:valid(), I'd just do "return !typeMismatch() && !stepMismatch() && ...". In ValidityState.idl, any reason you listed the attributes in a different order than the spec? (It doesn't matter, I don't think, but it'd be easier to cross-check if they matched.) Otherwise things look OK to me but I'm clearly not the right person to review.

(In reply to comment #14) > In WebCore/ChangeLog, the bug link is usually just above the description (see > other comments there). In the comments you might want a link to the relevant > HTML5 spec section. Correcting.. > In ValidityState.cpp:valid(), I'd just do "return !typeMismatch() && > !stepMismatch() && ...". So did I at the beginning but opted to get it more readable this way; it's easier to read and to understand. > In ValidityState.idl, any reason you listed the attributes in a different order > than the spec? (It doesn't matter, I don't think, but it'd be easier to > cross-check if they matched.) It was the WF2's idl: just a matter of order; anyway np, reordering as you've suggested. > Otherwise things look OK to me but I'm clearly not the right person to review. Noted, gonna ask r? in a manner of minutes.

(In reply to comment #15) > > In ValidityState.cpp:valid(), I'd just do "return !typeMismatch() && > > !stepMismatch() && ...". > > So did I at the beginning but opted to get it more readable this way; it's > easier to read and to understand. I think it's less easy to read due to being longer and having an unnecessary temp, but it doesn't matter. No one will care.

Created attachment 31506 [details] ValidityState stuff, basic support. This patch adds support for the ValidityState class as per HTML5 Forms spec: http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#validitystate. All the remaining validation behaviour is waiting to be asked for r? due to this; valueMissing/requiredAttr (#25551) and patternMismatch/patternAttr (#25552) shall be ready in one day or two (others are expected to be available in a couple of weeks) so ValidityState is definitely needed in the tree.:) Have a nice review!:)

Created attachment 31508 [details] ValidityState stuff, basic support (Repost due to a mistake in the previous diff) This patch adds support for the ValidityState class as per HTML5 Forms spec: http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#validitystate. All the remaining validation behaviour is waiting to be asked for r? due to this; valueMissing/requiredAttr (#25551) and patternMismatch/patternAttr (#25552) shall be ready in one day or two (others are expected to be available in a couple of weeks) so ValidityState is definitely needed in the tree.:) Have a nice review!:)

Michelangelo, we need to update WebCore/WebCore.vcproj/WebCore.vcproj too.

(In reply to comment #19) > Michelangelo, > we need to update WebCore/WebCore.vcproj/WebCore.vcproj too. If there are build system changes needed, note that various ports use different ones: .vcproj files, .xcodeproj files, .bkl files (hope I got that right), and at least one or two other types. Best way to find what needs to be touched is to pick a filename that already exists in the same location/subproject as where you're adding, and then grep the source tree for that filename and note all files that contain it.

(In reply to comment #20) > > we need to update WebCore/WebCore.vcproj/WebCore.vcproj too. ..and... > you're adding, and then grep the source tree for that filename and note all > files that contain it. ...noted.:) Gonna work on those build files...

Comment on attachment 31508 [details] ValidityState stuff, basic support looks good!

r+ is contingent on making sure those other build project files are updated before checking in. Peter's on board with this - so no one else commit without making sure that's settled!

Michelangelo, are you having trouble determining how to touch the build files here? It should be the work of a few minutes to get your latest patch ready for checkin.

Created attachment 32529 [details] Fixes to build systems for ValidityState Waiting for feedbacks.

(In reply to comment #25) > Created an attachment (id=32529) [details] > Fixes to build systems for ValidityState This can't be reviewed; your .vcproj diff is the entire file. Did you get CR vs. CR-LF switched from how the file is checked in? Also, your ChangeLog should reference this bug. Once you get everything fixed with a smaller patch, go ahead and obsolete the r+d patch.

Created attachment 32557 [details] ValidityState full patch This patch includes previous code plus build fixes for various platform. It includes tweaks to add ValidityState into the recently added gyp build system.

(In reply to comment #27) > Created an attachment (id=32557) [details] > ValidityState full patch > > This patch includes previous code plus build fixes for various platform. It > includes tweaks to add ValidityState into the recently added gyp build system. Looks OK to me at a glance (without testing). Tiny tweak: I suggest putting the bug link in your ChangeLog descriptions above the summary rather than below. This can be done by whoever checks in if they agree, though. Since you've asked for r? from Adele I will wait for her opinion rather than just landing.

Created attachment 32578 [details] Patch for checkin The existing patch didn't apply cleanly due to some out-of-date sources, especially the .pbxproj. Here's the patch as I've tried to hack it locally to do the right thing. I've ordered the sections for comparison against the original patch in case anyone wants to. Not carrying over Adele's r+ from the other patch just so it doesn't look like I've stamped it r+ myself, but I do intend to check this in if things work. I'm a bit nervous about this since I don't have a Mac to test my .pbxproj changes on. Right now I'm building locally for Windows, maybe I'll ask another Chromium developer to test-build this on Mac. I am also not 100% sure that DOMValidityState.* should go in the DerivedSources/HTML group (where nearly all the other files are DOMHTML*), but I don't know if there's a better place.

(In reply to comment #29) > Created an attachment (id=32578) [details] > Patch for checkin > > I'm a bit nervous about this since I don't have a Mac to test my .pbxproj > changes on. Right now I'm building locally for Windows, maybe I'll ask another > Chromium developer to test-build this on Mac. dglazkov tells me this builds and looks OK in XCode, and passes the layout test. Hooray. However, on Windows, even a clean build gets IDL errors that I don't understand (see below). I'm not wiling to commit this until I figure out why. 9>e:\pkasting\webkit\WebKit\WebKitBuild\obj\WebKit\DerivedSources\IGEN_DOMHTMLButtonElement.idl(59) : error MIDL2011 : unresolved type declaration : IGEN_DOMValidityState [ Parameter of Procedure 'validity' ( Interface 'IGEN_DOMHTMLButtonElement' ) ] 9>e:\pkasting\webkit\WebKit\WebKitBuild\obj\WebKit\DerivedSources\IGEN_DOMHTMLFieldSetElement.idl(59) : error MIDL2011 : unresolved type declaration : IGEN_DOMValidityState [ Parameter of Procedure 'validity' ( Interface 'IGEN_DOMHTMLFieldSetElement' ) ] 9>e:\pkasting\webkit\WebKit\WebKitBuild\obj\WebKit\DerivedSources\IGEN_DOMHTMLInputElement.idl(65) : error MIDL2011 : unresolved type declaration : IGEN_DOMValidityState [ Parameter of Procedure 'validity' ( Interface 'IGEN_DOMHTMLInputElement' ) ] 9>e:\pkasting\webkit\WebKit\WebKitBuild\obj\WebKit\DerivedSources\IGEN_DOMHTMLSelectElement.idl(79) : error MIDL2011 : unresolved type declaration : IGEN_DOMValidityState [ Parameter of Procedure 'validity' ( Interface 'IGEN_DOMHTMLSelectElement' ) ] 9>e:\pkasting\webkit\WebKit\WebKitBuild\obj\WebKit\DerivedSources\IGEN_DOMHTMLTextAreaElement.idl(62) : error MIDL2011 : unresolved type declaration : IGEN_DOMValidityState [ Parameter of Procedure 'validity' ( Interface 'IGEN_DOMHTMLTextAreaElement' ) ] 9>Project : warning PRJ0018 : The following environment variables were not found: 9>$(PRODUCTION) 9>Build log was saved at "file://e:\pkasting\webkit\WebKit\WebKitBuild\obj\Interfaces\Release\BuildLog.htm" 9>Interfaces - 5 error(s), 0 warning(s)

(In reply to comment #30) > However, on Windows, even a clean build gets IDL errors that I don't understand > (see below). Looks like WebKitBuild/obj/WebKit/DerivedSources/IGEN_DOMValidityState.idl is being included in these files, but not generated first. Maybe a vcproj or script is just missing an entry...

More questions: What about: WebCore/bindings/objc/DOMHTML.h WebCore/bindings/objc/PublicDOMInterfaces.h WebCore/WebCore.base.exp WebKit/win/Interfaces/WebKit.idl WebKit/win/WebKit.vcproj/WebKit.vcproj Do these need to have something about DOMValidityState added, regardless of the Windows build failure?

Created attachment 32595 [details] patch as checked in (r45739) On IRC, weinig suggested commenting out the "validity" portions of the modified IDL files in the case of COM. I still don't fully understand the justification he gave, but doing this makes things compile for now. Now that things build and the layout test passes on Windows and Mac, I'm going to check in. I am not sure that all the bindings everywhere are in the right shape so I won't close this bug yet.

weinig/aroben/andersca: You guys were mentioned on IRC as knowing about IDL files and/or bindings. Can you take a look at what got checked in (see attachment above), especially w.r.t. comment 32 and comment 33, and either sketch out or (preferably, since I'm clueless) write a demonstration patch/link to some past patch that covers what, if any, changes should be made for COM and ObjC? dglazkov: Same question, just for V8 bindings, which I'm sure did not get covered by the checked-in patch.

(In reply to comment #33) > Created an attachment (id=32595) [details] > patch as checked in This may have caused the following tests to fail: * fast/dom/domListEnumeration.html - excerpt: "[object HTMLSelectElement] FAIL resultArray.length should be 135. Was 136." I don't know any JS, but if this is testing the number of attributes on the HTMLSelectElement, it probably increased by 1 ("validity") so the test should be updated. * http/tests/workers/worker-importScripts.html - excerpt: "FAIL: Threw Error: NETWORK_ERR: XMLHttpRequest Exception 101 when attempting cross origin load" At a glance I'm not sure why this would be related to this patch. * transitions/change-values-during-transition.html - Doesn't fail for me on Windows but failed on the Linux bot. May be a red herring.

(In reply to comment #35) > This may have caused the following tests to fail: > > * fast/dom/domListEnumeration.html - excerpt: > "[object HTMLSelectElement] > FAIL resultArray.length should be 135. Was 136." > > I don't know any JS, but if this is testing the number of attributes on the > HTMLSelectElement, it probably increased by 1 ("validity") so the test should > be updated. Since I have to run right now, I disabled this layout test (r45742) until I have time to make sure this is the right fix.

(In reply to comment #34) > weinig/aroben/andersca: You guys were mentioned on IRC as knowing about IDL > files and/or bindings. Can you take a look at what got checked in (see > attachment above), especially w.r.t. comment 32 and comment 33, and either > sketch out or (preferably, since I'm clueless) write a demonstration patch/link > to some past patch that covers what, if any, changes should be made for COM and > ObjC? Sam is the expert, but I don't think you need to make any changes specifically for COM (beyond what you've done already). The generated COM bindings will need a final push before they become usable, anyway.

(In reply to comment #34) > dglazkov: Same question, just for V8 bindings, which I'm sure did not get > covered by the checked-in patch. These bindings are quite simple, should "Just Work" (tm) for V8 bindings.

Working to get the patch conditional... If there's no objection I'll be using the ENABLE_FORM_VALIDATION flag as per Darin's suggestion.

Backed everything out in r45830. Here's what needs to happen to re-land: * The "patch as checked in" should be merged onto a clean, up-to-date tree, and any conflicts resolved. * A flag, ENABLE_FORM_VALIDATION, should be added, and everything from this patch and any other validation-related attributes/functions that a web author can detect (willValidate, checkValidity(), etc.) should be placed under it until the point where we have the complete feature ready for web authors to use. * The layout test in this bug, and any tests for already-existing features, should be moved to "foo.html-disabled" (for appropriate values of foo.html). We can un-disable them in the change that turns the flag on. * The correct change to fast/dom/domListEnumeration.html when the flag is on should be determined so we can commit that change at the point we turn the flag on. See comment 35. * The ObjC bindings should be finished. I don't know what the right thing is here. See comment 32. Things not to worry about: * V8 bindings -- the .idl changes in this patch are sufficient to cover them. * COM bindings -- according to comment 37 I guess we just disable the generation here and leave this alone? * Unbreaking downstream Chromium build -- dglazkov plans to land the change to use the upstreamed .gypi files today so no downstream patches should be necessary.

(In reply to comment #32) > WebCore/bindings/objc/DOMHTML.h I don't think it's wise to include DOMValidityState.h in this file right now due to ENABLE flag. > WebCore/bindings/objc/PublicDOMInterfaces.h This should be done but, at this time (with the ENABLE flag), we should rely on private header files. > WebCore/WebCore.base.exp I'm really doubtful about this: I need to investigate further. > WebKit/win/Interfaces/WebKit.idl As far as I can understand this should be related to COM bindings, if so we should avoid touching it as per Adam's reply.

(In reply to comment #41) > (In reply to comment #32) > > WebCore/bindings/objc/DOMHTML.h > > I don't think it's wise to include DOMValidityState.h in this file right now > due to ENABLE flag. Is this file not generated at all with the flag off, or is it generated and just doesn't have any non-#ifdef'd contents? > > WebCore/WebCore.base.exp > > I'm really doubtful about this: I need to investigate further. I believe you have to export symbols for anything that someone can bind to from outside WebCore. > > WebKit/win/Interfaces/WebKit.idl > > As far as I can understand this should be related to COM bindings, if so we > should avoid touching it as per Adam's reply. We should try and get someone to verify this. I really, really wish there was documentation of this stuff somewhere :(

(In reply to comment #42) > > I don't think it's wise to include DOMValidityState.h in this file right now > > due to ENABLE flag. > Is this file not generated at all with the flag off, or is it generated and > just doesn't have any non-#ifdef'd contents? It's generated anyway, even with the "Conditional=FORM_VALIDATION" interface tweak; I've added another condition inside the IDL in order to minimize troubles for now. On the other hand, speaking about interfaces into PublicDOMInterfaces.h: *for now* I will cut "willValidate" and "validity" attributes out because we couldn't manage to exclude them with the ENABLE flag and that could cause troubles. [WebCore.base.exp] > I believe you have to export symbols for anything that someone can bind to from > outside WebCore. As far as I can see there's no such use with other stuff (like VoidCallback, ImageData, etc...). You suggest to modify this one too, anyway? > I really, really wish there was documentation of this stuff somewhere :( So do I... This patch is gonna drive me insane.:) I'm crosschecking the build for now...

After chatting with the good Sam, here's a quick recap about Obj-C DOM bindings. DOMHTML.h and PublicDOMInterfaces.h MUST NOT be touched at all, they're meant as (wow!) public DOM interfaces for the Mac. Just a few elected ones could touch them.:) WebCore.base.exp is not required to be modified: if the build couldn't link, we would. Anyway, this "patch as checked in" is incomplete due to lack of reference to new Obj-C bindings in MigrateHeaders.make. I'm waiting for comments about the ENABLE_FORM_VALIDATION flag to get the right code in.

Further update: We should just strip out ObjC changes from here entirely, which means we should remove DOMValidityState* references from the patch. We won't worry about adding ObjC bindings later, the Apple folks will do that as needed. Also, we're not going to worry about sticking things behind an ENABLE_ flag since hopefully the form validation stuff can be completed by a month or so from now. So the only changes needed to Friday's patch are: remove DOMValidityState*, and fix the layout test that broke.

(In reply to comment #45) > So the only changes needed to Friday's patch are: remove DOMValidityState*, and > fix the layout test that broke. Acknowledged. Working...

Created attachment 32744 [details] ValidityState and related build stuff

Comment on attachment 32744 [details] ValidityState and related build stuff Removing specific reviewer request. The only thing that needs review here is that adding DOMValidityState.{h,mm} (which we needed to compile) but not adding to Copy-Generated Headers or adding DOMValidityStateInternal* is kosher. Everything else is the same as the patch on Friday other than a layout test fix.

Landed patch as r45888. Since Michelangelo and I don't plan to make further ObjC or COM bindings changes ourselves for this, this bug is hopefully now dealt with and we can continue on the dependent bugs.

V8 bindings fix landed as http://trac.webkit.org/changeset/45888.

(In reply to comment #50) > V8 bindings fix landed as http://trac.webkit.org/changeset/45888. I'm pretty sure dglazkov meant http://trac.webkit.org/changeset/45915 . Thanks for the note, I'll double-check future form validation patches to see that they have similar changes.