Bug 29069

I'm working on this.

Created attachment 39319 [details] Proposed patch

Created attachment 39858 [details] Proposed patch (rev.2) resources/ -> script-tests/

Comment on attachment 39858 [details] Proposed patch (rev.2) Reversed, no? 53 double maximum; // maximum must be <= minimum. Tab, will cause commit to fail: 13 testPassed(resultText); I would have probably written a little static inline function to help grab the numbers, but I think my "make a function" threshold is lower than most others: double min; 202 double doubleValue; 203 if (hasAttribute(minAttr) && formStringToDouble(getAttribute(minAttr).string(), &min) && formStringToDouble(value(), &doubleValue)) 204 return doubleValue < min; This comment mostly confuses me: 249 max = min; // The spec allows min == max. I'm not surprised that min is allowed to equal max. Why not write that whole if as: max = max(min, max(100, max)) obviously you'd have to use std:: or change the name of max. r- for the nits. Really only the backwards comment, the rest doesn't need to be changed. If you were a committer, I would r+ this and you could commit it with mods yourself.

Created attachment 40033 [details] Proposed patch (rev.3) > Reversed, no? > 53 double maximum; // maximum must be <= minimum. Right! Fixed. > Tab, will cause commit to fail: > 13 testPassed(resultText); Fixed 8 instances in 4 files. > I would have probably written a little static inline function to help grab the > numbers, but I think my "make a function" threshold is lower than most others: > double min; > 202 double doubleValue; > 203 if (hasAttribute(minAttr) && formStringToDouble(getAttribute(minAttr).string(), &min) && formStringToDouble(value(), &doubleValue)) > 204 return doubleValue < min; I didn't make a such function, and simplified this code by removing hasAttribute() and .string() instead. formStringToDouble() returns false if the input string is null. > This comment mostly confuses me: > 249 max = min; // The spec allows min == max. > I'm not surprised that min is allowed to equal max. I improved the code and comments here.

Created attachment 40601 [details] Proposed patch (rev.4) Resolved conflicts with today's WebKit.

Comment on attachment 40601 [details] Proposed patch (rev.4) > + // Returns the minimum value for type=range. Don't call this for other types. > + double minRange() const; I think this should be named rangeMinimum(), not minRange(), which sounds like "minimum range" to me. > + // Returns the maximum value for type=range. Don't call this for other types. > + // This always returns a value which is <= minRange(). > + double maxRange() const; And rangeMaximum() for this. Otherwise seems fine to me, r=me

Created attachment 40602 [details] Proposed patch (rev.5) Renamed minRange() to rangeMinimum(), maxRange() to rangeMaximum().

Comment on attachment 40602 [details] Proposed patch (rev.5) > + (WebCore::HTMLInputElement::minRange): > + (WebCore::HTMLInputElement::maxRange): Needs new name here. > + // This always returns a value which is <= minRange(). And here. Still fine as-is, but better to get it right.

Created attachment 40608 [details] Proposed patch (rev.6) Oops, updated a comment and ChangeLog for rangeMinimum()/rangeMaximum().

Comment on attachment 40608 [details] Proposed patch (rev.6) Clearing flags on attachment: 40608 Committed r49104: <http://trac.webkit.org/changeset/49104>

All reviewed patches have been landed. Closing bug.

This broke the windows bots: http://build.webkit.org/builders/Windows%20Debug%20%28Build%29/builds/5913 so I've asked the commit-bot to roll out this patch. Should be rolled out in 10 minutes or so.

Looks like the build failure was due to MSVC not being smart enough about uninitialized variables and code flow. Should be easy to work around.

Brian got past that error by initializing doubleValue to 0.0, but then hit 17 more and gave up. Hence the rollout.

Rolled out in http://trac.webkit.org/changeset/49107.

Created attachment 40679 [details] Proposed patch (rev.7) Fixed the build problems on Windows. - Initialize local variables with 0.0 - Not define max/min for COM to avoid conflict in WebKit.h I think to add "#define NOMINMAX" to WebKit.h is the right fix. But I couldn't find who generates WebKit.h.

I found WebKit.h was generated by MIDL. It's hard to insert "#define NO_MINMAX" to it. I think WebKit.h is a public header, and we should not add "#define NO_MINMAX" to it and should not request "define NO_MINMAX before including WebKit.h" because it would break source-code compatibility. So, I think the patch (rev.7) is the best approach.

Comment on attachment 40679 [details] Proposed patch (rev.7) Clearing flags on attachment: 40679 Committed r49199: <http://trac.webkit.org/changeset/49199>

All reviewed patches have been landed. Closing bug.