Derive the step base for an input element as (now) specified.

Derive the step base for an input element as (now) specified.

Determine the step base for an input element per spec,

  http://www.whatwg.org/specs/web-apps/current-work/#concept-input-min-zero

That is, consult the 'value' attribute if 'min' is not present, and
then fallback to the default step base for the input type if that
isn't present either. Previously, 'value' was not considered.

R=
BUG=257369
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162733

[sof, 2013-11-22 12:50:20]

At your own leisure, please have a look.

[sof, 2013-11-22 12:50:49]

https://codereview.chromium.org/83413002/diff/1/Source/core/html/forms/InputT...
File Source/core/html/forms/InputType.h (right):

https://codereview.chromium.org/83413002/diff/1/Source/core/html/forms/InputT...
Source/core/html/forms/InputType.h:205: virtual Decimal parseToNumber(const
String&, const String&, const Decimal& defaultValue) const;
FYI - I went back & forth a bit on whether or not to extend parseToNumber() like
this -- the alternative being to handle it in the createStepRange() methods for
the input types that have a step base. They were in the majority, hence I opted
for this, but could easily be convinced to address it differently.

[tkent, 2013-11-24 22:30:06]

Keishi, yosin, would you review this please?

[tkent, 2013-11-24 22:34:23]

https://codereview.chromium.org/83413002/diff/1/Source/core/html/forms/InputT...
File Source/core/html/forms/InputType.h (right):

https://codereview.chromium.org/83413002/diff/1/Source/core/html/forms/InputT...
Source/core/html/forms/InputType.h:205: virtual Decimal parseToNumber(const
String&, const String&, const Decimal& defaultValue) const;
On 2013/11/22 12:50:49, sof wrote:
> FYI - I went back & forth a bit on whether or not to extend parseToNumber()
like
> this -- the alternative being to handle it in the createStepRange() methods
for
> the input types that have a step base. They were in the majority, hence I
opted
> for this, but could easily be convinced to address it differently.

I don't think adding another argument to parseToNumber is reasonable.
The new behavior looks out of scope of parseToNumber.

We should handle value attribute fallback in each of createStepRange, and we may
have an additional helper function.

[yosin_UTC9, 2013-11-25 01:26:40]

Except for RangeInputType, implementations of InputType::createStepRange() are
identical. Can we share implementation of createStepRange()?

https://codereview.chromium.org/83413002/diff/1/LayoutTests/fast/forms/calend...
File
LayoutTests/fast/forms/calendar-picker/calendar-picker-datetimelocal-with-step.html
(right):

https://codereview.chromium.org/83413002/diff/1/LayoutTests/fast/forms/calend...
LayoutTests/fast/forms/calendar-picker/calendar-picker-datetimelocal-with-step.html:16:
document.getElementById('datetime-bare').value = "2012-11-20T00:00";
nit: Please use single-quote to avoid mixing single and double quote in a
statement.

https://codereview.chromium.org/83413002/diff/1/LayoutTests/fast/forms/calend...
LayoutTests/fast/forms/calendar-picker/calendar-picker-datetimelocal-with-step.html:58:
document.getElementById('datetime').max = "2012-11-20T00:00";
nit: Please use single-quote to avoid mixing single and double quote in a
statement.

https://codereview.chromium.org/83413002/diff/1/LayoutTests/fast/forms/calend...
File LayoutTests/fast/forms/calendar-picker/calendar-picker-with-step.html
(right):

https://codereview.chromium.org/83413002/diff/1/LayoutTests/fast/forms/calend...
LayoutTests/fast/forms/calendar-picker/calendar-picker-with-step.html:18:
document.getElementById('date-bare').value="2011-05-01" ;
nit: Please use single-quote to avoid mixing single and double quote in a
statement.

https://codereview.chromium.org/83413002/diff/1/LayoutTests/fast/forms/calend...
LayoutTests/fast/forms/calendar-picker/calendar-picker-with-step.html:44:
document.getElementById('date').min = "2011-05-01";
nit: Please use single-quote to avoid mixing single and double quote in a
statement.

https://codereview.chromium.org/83413002/diff/1/Source/core/html/forms/InputT...
File Source/core/html/forms/InputType.h (right):

https://codereview.chromium.org/83413002/diff/1/Source/core/html/forms/InputT...
Source/core/html/forms/InputType.h:205: virtual Decimal parseToNumber(const
String&, const String&, const Decimal& defaultValue) const;
On 2013/11/24 22:34:23, tkent wrote:
> On 2013/11/22 12:50:49, sof wrote:
> > FYI - I went back & forth a bit on whether or not to extend parseToNumber()
> like
> > this -- the alternative being to handle it in the createStepRange() methods
> for
> > the input types that have a step base. They were in the majority, hence I
> opted
> > for this, but could easily be convinced to address it differently.
> 
> I don't think adding another argument to parseToNumber is reasonable.
> The new behavior looks out of scope of parseToNumber.
> 
> We should handle value attribute fallback in each of createStepRange, and we
may
> have an additional helper function.
Agree with tkent@. It is confusing having alternative and default.
nit: Please put parameter name for const String&.

https://codereview.chromium.org/83413002/diff/1/Source/core/html/forms/RangeI...
File Source/core/html/forms/RangeInputType.cpp (right):

https://codereview.chromium.org/83413002/diff/1/Source/core/html/forms/RangeI...
Source/core/html/forms/RangeInputType.cpp:132: const Decimal step =
equalIgnoringCase(precisionValue, "float") ? Decimal::nan() : 1;
BTW, Can we stop supporting precsion="float"? If so, implementations of
InputType::createStepRange() are identical.

[tkent, 2013-11-25 01:36:32]

https://codereview.chromium.org/83413002/diff/1/Source/core/html/forms/RangeI...
File Source/core/html/forms/RangeInputType.cpp (right):

https://codereview.chromium.org/83413002/diff/1/Source/core/html/forms/RangeI...
Source/core/html/forms/RangeInputType.cpp:132: const Decimal step =
equalIgnoringCase(precisionValue, "float") ? Decimal::nan() : 1;
On 2013/11/25 01:26:41, Yoshi wrote:
> BTW, Can we stop supporting precsion="float"? If so, implementations of
> InputType::createStepRange() are identical.

We have UseCounter for precision attribute.  We can make a decision of the
removal after M32 stable release.

[keishi, 2013-11-25 02:56:49]

Tests look good.
I too would prefer the fallback to be handled in createStepRange.

[sof, 2013-11-25 08:12:26]

Thanks very much for the great feedback. I'm travelling today, but will address
as soon as possible.

[sof, 2013-11-26 17:59:05]

https://codereview.chromium.org/83413002/diff/1/LayoutTests/fast/forms/calend...
File
LayoutTests/fast/forms/calendar-picker/calendar-picker-datetimelocal-with-step.html
(right):

https://codereview.chromium.org/83413002/diff/1/LayoutTests/fast/forms/calend...
LayoutTests/fast/forms/calendar-picker/calendar-picker-datetimelocal-with-step.html:16:
document.getElementById('datetime-bare').value = "2012-11-20T00:00";
On 2013/11/25 01:26:41, Yoshi wrote:
> nit: Please use single-quote to avoid mixing single and double quote in a
> statement.

Done.

https://codereview.chromium.org/83413002/diff/1/LayoutTests/fast/forms/calend...
LayoutTests/fast/forms/calendar-picker/calendar-picker-datetimelocal-with-step.html:58:
document.getElementById('datetime').max = "2012-11-20T00:00";
On 2013/11/25 01:26:41, Yoshi wrote:
> nit: Please use single-quote to avoid mixing single and double quote in a
> statement.

Done.

https://codereview.chromium.org/83413002/diff/1/LayoutTests/fast/forms/calend...
File LayoutTests/fast/forms/calendar-picker/calendar-picker-with-step.html
(right):

https://codereview.chromium.org/83413002/diff/1/LayoutTests/fast/forms/calend...
LayoutTests/fast/forms/calendar-picker/calendar-picker-with-step.html:18:
document.getElementById('date-bare').value="2011-05-01" ;
On 2013/11/25 01:26:41, Yoshi wrote:
> nit: Please use single-quote to avoid mixing single and double quote in a
> statement.

Done.

https://codereview.chromium.org/83413002/diff/1/LayoutTests/fast/forms/calend...
LayoutTests/fast/forms/calendar-picker/calendar-picker-with-step.html:44:
document.getElementById('date').min = "2011-05-01";
On 2013/11/25 01:26:41, Yoshi wrote:
> nit: Please use single-quote to avoid mixing single and double quote in a
> statement.

Done.

https://codereview.chromium.org/83413002/diff/1/Source/core/html/forms/InputT...
File Source/core/html/forms/InputType.h (right):

https://codereview.chromium.org/83413002/diff/1/Source/core/html/forms/InputT...
Source/core/html/forms/InputType.h:205: virtual Decimal parseToNumber(const
String&, const String&, const Decimal& defaultValue) const;
On 2013/11/25 01:26:41, Yoshi wrote:
> On 2013/11/24 22:34:23, tkent wrote:
> > On 2013/11/22 12:50:49, sof wrote:
> > > FYI - I went back & forth a bit on whether or not to extend
parseToNumber()
> > like
> > > this -- the alternative being to handle it in the createStepRange()
methods
> > for
> > > the input types that have a step base. They were in the majority, hence I
> > opted
> > > for this, but could easily be convinced to address it differently.
> > 
> > I don't think adding another argument to parseToNumber is reasonable.
> > The new behavior looks out of scope of parseToNumber.
> > 
> > We should handle value attribute fallback in each of createStepRange, and
we
> may
> > have an additional helper function.
> Agree with tkent@. It is confusing having alternative and default.
> nit: Please put parameter name for const String&.

Thanks, added a helper InputType::findStepBase(const Decimal&) instead &
reverted changes to parseToNumber().

(I did not give parseToNumber()'s first argument a name, however. Not sure there
is one that conveys considerable meaning.)

[sof, 2013-11-26 18:01:44]

On 2013/11/25 01:26:40, Yoshi wrote:
> Except for RangeInputType, implementations of InputType::createStepRange() are
> identical. Can we share implementation of createStepRange()?
> 

Looks plausible, but I've not addressed this.

[yosin_UTC9, 2013-11-27 01:27:56]

On 2013/11/26 18:01:44, sof wrote:
> On 2013/11/25 01:26:40, Yoshi wrote:
> > Except for RangeInputType, implementations of InputType::createStepRange()
are
> > identical. Can we share implementation of createStepRange()?
> > 
> 
> Looks plausible, but I've not addressed this.

Rather than introducing simple helper, I think it is time to re-factor this.
Each implementation of createStepRange() looks very similar.

[yosin_UTC9, 2013-11-27 01:28:05]

https://codereview.chromium.org/83413002/diff/130001/Source/core/html/forms/I...
File Source/core/html/forms/InputType.cpp (right):

https://codereview.chromium.org/83413002/diff/130001/Source/core/html/forms/I...
Source/core/html/forms/InputType.cpp:988: Decimal InputType::findStepBase(const
Decimal& defaultValue) const
The word "find" leads me the function has loop. How about "calculateStepBase()"
or better?

https://codereview.chromium.org/83413002/diff/130001/Source/core/html/forms/I...
Source/core/html/forms/InputType.cpp:990: Decimal stepBase =
parseToNumber(element().fastGetAttribute(minAttr), Decimal::nan());
nit: const Decimal stepBase = ...;

https://codereview.chromium.org/83413002/diff/130001/Source/core/html/forms/I...
Source/core/html/forms/InputType.cpp:992: stepBase =
parseToNumber(element().fastGetAttribute(valueAttr), defaultValue);
nit: rather than copying, just |return parseToNumber(...);|

[tkent, 2013-11-27 02:04:30]

On 2013/11/27 01:27:56, Yoshi wrote:
> On 2013/11/26 18:01:44, sof wrote:
> > On 2013/11/25 01:26:40, Yoshi wrote:
> > > Except for RangeInputType, implementations of InputType::createStepRange()
> are
> > > identical. Can we share implementation of createStepRange()?
> > > 
> > 
> > Looks plausible, but I've not addressed this.
> 
> Rather than introducing simple helper, I think it is time to re-factor this.
> Each implementation of createStepRange() looks very similar.

I don't recommend doing a behavior change and refactoring at once.  Smaller
patch is better.

[tkent, 2013-11-27 02:05:32]

lgtm.

yosin, keishi, thank you for non-owner review!

[commit-bot: I haz the power, 2013-11-27 02:05:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/83413002/130001

[commit-bot: I haz the power, 2013-11-27 03:12:59]

Change committed as 162733

[sof, 2013-11-27 09:21:02]

On 2013/11/27 02:04:30, tkent wrote:
> On 2013/11/27 01:27:56, Yoshi wrote:
> > On 2013/11/26 18:01:44, sof wrote:
> > > On 2013/11/25 01:26:40, Yoshi wrote:
> > > > Except for RangeInputType, implementations of
InputType::createStepRange()
> > are
> > > > identical. Can we share implementation of createStepRange()?
> > > > 
> > > 
> > > Looks plausible, but I've not addressed this.
> > 
> > Rather than introducing simple helper, I think it is time to re-factor this.
> > Each implementation of createStepRange() looks very similar.
> 
> I don't recommend doing a behavior change and refactoring at once.  Smaller
> patch is better.

Thanks everyone, appreciated :) I can follow up on the refactoring.