Bug 19562 - ValidityState object stub
Summary: ValidityState object stub
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Michelangelo De Simone
URL: http://www.w3.org/TR/web-forms-2/#val...
Keywords:
Depends on:
Blocks: HTML5Forms H5F-RequiredAttr H5F-PatternAttr
  Show dependency treegraph
 
Reported: 2008-06-15 11:36 PDT by Michelangelo De Simone
Modified: 2009-07-15 11:46 PDT (History)
7 users (show)

See Also:


Attachments
ValidityState stub class and related stuff (21.05 KB, patch)
2008-06-15 11:54 PDT, Michelangelo De Simone
eric: review-
Details | Formatted Diff | Diff
ValidityState stub class and related stuff, second revision (31.45 KB, patch)
2008-08-05 12:38 PDT, Michelangelo De Simone
eric: review-
Details | Formatted Diff | Diff
ValidityState stub class and related stuff, third revision (23.05 KB, patch)
2008-08-14 13:16 PDT, Michelangelo De Simone
adele: review-
Details | Formatted Diff | Diff
Clean and functioning patch. (15.57 KB, patch)
2009-06-18 06:39 PDT, Michelangelo De Simone
no flags Details | Formatted Diff | Diff
Clean and functioning patch (corrected). (22.54 KB, patch)
2009-06-18 06:43 PDT, Michelangelo De Simone
no flags Details | Formatted Diff | Diff
ValidityState stuff, basic support. (22.64 KB, patch)
2009-06-18 14:02 PDT, Michelangelo De Simone
no flags Details | Formatted Diff | Diff
ValidityState stuff, basic support (22.64 KB, patch)
2009-06-18 14:11 PDT, Michelangelo De Simone
adele: review+
Details | Formatted Diff | Diff
Fixes to build systems for ValidityState (1.42 MB, patch)
2009-07-09 13:23 PDT, Michelangelo De Simone
no flags Details | Formatted Diff | Diff
ValidityState full patch (28.89 KB, patch)
2009-07-10 06:59 PDT, Michelangelo De Simone
adele: review+
Details | Formatted Diff | Diff
Patch for checkin (31.56 KB, patch)
2009-07-10 14:15 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
patch as checked in (r45739) (32.07 KB, patch)
2009-07-10 17:48 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
ValidityState and related build stuff (28.93 KB, patch)
2009-07-14 16:07 PDT, Michelangelo De Simone
adele: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michelangelo De Simone 2008-06-15 11:36:02 PDT
A ValidityState object stub shall be added to the trunk in order to permits further development of WS2 spec.
Comment 1 Michelangelo De Simone 2008-06-15 11:54:35 PDT
Created attachment 21716 [details]
ValidityState stub class and related stuff
Comment 2 Eric Seidel (no email) 2008-07-04 11:40:28 PDT
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.
Comment 3 Eric Seidel (no email) 2008-07-04 11:41:37 PDT
(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.
Comment 4 Michelangelo De Simone 2008-08-05 12:38:49 PDT
Created attachment 22658 [details]
ValidityState stub class and related stuff, second revision
Comment 5 Eric Seidel (no email) 2008-08-06 02:38:23 PDT
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
Comment 6 Michelangelo De Simone 2008-08-14 13:16:05 PDT
Created attachment 22797 [details]
ValidityState stub class and related stuff, third revision

VS is now allocated on request.
Comment 7 Darin Adler 2008-10-12 16:03:30 PDT
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 8 Adele Peterson 2008-10-14 11:16:58 PDT
Comment on attachment 22797 [details]
ValidityState stub class and related stuff, third revision

We need to fix this up. r- for now.
Comment 9 Michelangelo De Simone 2009-06-12 15:03:37 PDT
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?
Comment 10 Peter Kasting 2009-06-12 15:14:08 PDT
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.
Comment 11 Michelangelo De Simone 2009-06-12 15:27:26 PDT
(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
Comment 12 Michelangelo De Simone 2009-06-18 06:39:10 PDT
Created attachment 31493 [details]
Clean and functioning patch.

Here it is. Waiting for comments before asking r?.
Comment 13 Michelangelo De Simone 2009-06-18 06:43:40 PDT
Created attachment 31494 [details]
Clean and functioning patch (corrected).
Comment 14 Peter Kasting 2009-06-18 10:49:03 PDT
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.
Comment 15 Michelangelo De Simone 2009-06-18 13:45:50 PDT
(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.
Comment 16 Peter Kasting 2009-06-18 13:48:42 PDT
(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.
Comment 17 Michelangelo De Simone 2009-06-18 14:02:38 PDT
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!:)
Comment 18 Michelangelo De Simone 2009-06-18 14:11:11 PDT
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!:)
Comment 19 Kent Tamura 2009-07-03 02:35:23 PDT
Michelangelo,
we need to update WebCore/WebCore.vcproj/WebCore.vcproj too.
Comment 20 Peter Kasting 2009-07-03 10:29:24 PDT
(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.
Comment 21 Michelangelo De Simone 2009-07-03 10:31:35 PDT
(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 22 Adele Peterson 2009-07-07 20:34:31 PDT
Comment on attachment 31508 [details]
ValidityState stuff, basic support

looks good!
Comment 23 Adele Peterson 2009-07-07 20:38:18 PDT
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!
Comment 24 Peter Kasting 2009-07-09 10:59:01 PDT
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.
Comment 25 Michelangelo De Simone 2009-07-09 13:23:54 PDT
Created attachment 32529 [details]
Fixes to build systems for ValidityState

Waiting for feedbacks.
Comment 26 Peter Kasting 2009-07-09 13:26:51 PDT
(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.
Comment 27 Michelangelo De Simone 2009-07-10 06:59:34 PDT
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.
Comment 28 Peter Kasting 2009-07-10 12:46:31 PDT
(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.
Comment 29 Peter Kasting 2009-07-10 14:15:40 PDT
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.
Comment 30 Peter Kasting 2009-07-10 15:15:03 PDT
(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)
Comment 31 Peter Kasting 2009-07-10 15:19:12 PDT
(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...
Comment 32 Peter Kasting 2009-07-10 16:04:06 PDT
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?
Comment 33 Peter Kasting 2009-07-10 17:48:59 PDT
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.
Comment 34 Peter Kasting 2009-07-10 17:58:04 PDT
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.
Comment 35 Peter Kasting 2009-07-10 18:34:29 PDT
(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.
Comment 36 Peter Kasting 2009-07-10 18:40:04 PDT
(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.
Comment 37 Adam Roben (:aroben) 2009-07-13 07:52:41 PDT
(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.
Comment 38 Dimitri Glazkov (Google) 2009-07-13 09:36:47 PDT
(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.
Comment 39 Michelangelo De Simone 2009-07-13 11:19:16 PDT
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.
Comment 40 Peter Kasting 2009-07-13 11:31:18 PDT
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.
Comment 41 Michelangelo De Simone 2009-07-13 15:51:25 PDT
(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.
Comment 42 Peter Kasting 2009-07-13 15:58:28 PDT
(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 :(
Comment 43 Michelangelo De Simone 2009-07-14 12:15:49 PDT
(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...
Comment 44 Michelangelo De Simone 2009-07-14 14:25:56 PDT
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.
Comment 45 Peter Kasting 2009-07-14 14:49:04 PDT
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.
Comment 46 Michelangelo De Simone 2009-07-14 14:51:22 PDT
(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...
Comment 47 Michelangelo De Simone 2009-07-14 16:07:02 PDT
Created attachment 32744 [details]
ValidityState and related build stuff
Comment 48 Peter Kasting 2009-07-14 16:29:07 PDT
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.
Comment 49 Peter Kasting 2009-07-14 19:04:29 PDT
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.
Comment 50 Dimitri Glazkov (Google) 2009-07-15 09:18:26 PDT
V8 bindings fix landed as http://trac.webkit.org/changeset/45888.
Comment 51 Peter Kasting 2009-07-15 11:46:51 PDT
(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.