Make the setCustomValidity() error argument not nullable

Make the setCustomValidity() error argument not nullable

As per spec link: 
https://html.spec.whatwg.org/#dom-cva-setcustomvalidity,
setCustomValidity() error argument should not be nullable,
i.e [TreatUndefinedAs=NullString] bit should also be removed.
Gecko has already done these changes.

BUG=504576
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198678

[shiva.jm, 2015-06-30 05:07:31]

shiva.jm@samsung.com changed reviewers:
+ philipj@opera.com, tkent@chromium.org

[shiva.jm, 2015-06-30 05:07:31]

pls have a look.

[shiva.jm, 2015-07-01 12:04:12]

shiva.jm@samsung.com changed reviewers:
+ pdr@chromium.org

[philipj_slow, 2015-07-02 09:53:00]

https://codereview.chromium.org/1220833002/diff/1/Source/core/html/HTMLButton...
File Source/core/html/HTMLButtonElement.idl (right):

https://codereview.chromium.org/1220833002/diff/1/Source/core/html/HTMLButton...
Source/core/html/HTMLButtonElement.idl:42: void setCustomValidity(DOMString?
error);
You should also remove the "?", that's what makes the argument nullable. Gecko
has this in their IDL files:
void setCustomValidity(DOMString error);

The cases that need to be tested are then setCustomValidity(null) and
setCustomValidity(undefined), which should result in strings "null" and
"undefined" wherever it is that the error argument ends up.

[tkent, 2015-07-03 00:39:24]

https://codereview.chromium.org/1220833002/diff/1/LayoutTests/fast/forms/Vali...
File LayoutTests/fast/forms/ValidityState-customError.html (right):

https://codereview.chromium.org/1220833002/diff/1/LayoutTests/fast/forms/Vali...
LayoutTests/fast/forms/ValidityState-customError.html:115: 
Please add tests for <keygen>, <obejct>, and <output>.

https://codereview.chromium.org/1220833002/diff/1/LayoutTests/fast/forms/Vali...
LayoutTests/fast/forms/ValidityState-customError.html:153: 
Ditto.

[shiva.jm, 2015-07-06 09:51:27]

Update the tests and review comments, pls have a look.

[philipj_slow, 2015-07-06 12:36:18]

https://codereview.chromium.org/1220833002/diff/20001/LayoutTests/fast/dom/se...
File LayoutTests/fast/dom/set-custom-validity-with-too-few-arguments.html
(right):

https://codereview.chromium.org/1220833002/diff/20001/LayoutTests/fast/dom/se...
LayoutTests/fast/dom/set-custom-validity-with-too-few-arguments.html:12:
shouldBeUndefined("b.setCustomValidity(null)");
shouldNotThrow() would be a more explicit way of stating this, but the change
here isn't about the optionality of the argument, so that doesn't seem like the
right test. Rather, what has changed is the behavior when the argument is null
or undefined, so this should test that b.validationMessage is 'null' and
'undefined' respectively. A new test for the empty string doesn't seem
necessary, although it doesn't hurt.

I also don't think this is the right file for these tests, this is explicitly
about testing missing arguments, so
LayoutTests/fast/forms/ValidityState-customError.html or perhaps a new test
seems more appropriate.

[shiva.jm, 2015-07-07 10:33:42]

Update the tests and review comments, pls have a look.

https://codereview.chromium.org/1220833002/diff/20001/LayoutTests/fast/dom/se...
File LayoutTests/fast/dom/set-custom-validity-with-too-few-arguments.html
(right):

https://codereview.chromium.org/1220833002/diff/20001/LayoutTests/fast/dom/se...
LayoutTests/fast/dom/set-custom-validity-with-too-few-arguments.html:12:
shouldBeUndefined("b.setCustomValidity(null)");
On 2015/07/06 12:36:18, philipj wrote:
> shouldNotThrow() would be a more explicit way of stating this, but the change
> here isn't about the optionality of the argument, so that doesn't seem like
the
> right test. Rather, what has changed is the behavior when the argument is null
> or undefined, so this should test that b.validationMessage is 'null' and
> 'undefined' respectively. A new test for the empty string doesn't seem
> necessary, although it doesn't hurt.
> 
> I also don't think this is the right file for these tests, this is explicitly
> about testing missing arguments, so
> LayoutTests/fast/forms/ValidityState-customError.html or perhaps a new test
> seems more appropriate.

Done.

[philipj_slow, 2015-07-07 11:48:53]

https://codereview.chromium.org/1220833002/diff/80001/LayoutTests/fast/dom/se...
File LayoutTests/fast/dom/setCustomValidity-validationMessage.html (right):

https://codereview.chromium.org/1220833002/diff/80001/LayoutTests/fast/dom/se...
LayoutTests/fast/dom/setCustomValidity-validationMessage.html:8:
description('Test how setCustomValidity.validationMessage reacts to too few
arguments.');
There's a separate test for that, this should be primarily about null and
undefined, so maybe setCustomValidity-null-undefined.html and a description to
match. Even better if this is written using testharness.js with a single generic
bit of test code applied to all elements types, similar to
https://codereview.chromium.org/1085843002/

https://codereview.chromium.org/1220833002/diff/80001/LayoutTests/fast/dom/se...
LayoutTests/fast/dom/setCustomValidity-validationMessage.html:14:
shouldBeNull(b.validationMessage);
That seems wrong, b.validationMessage should be the string "null" unless I'm
missing something.

https://codereview.chromium.org/1220833002/diff/80001/LayoutTests/fast/dom/se...
LayoutTests/fast/dom/setCustomValidity-validationMessage.html:18:
shouldBeUndefined(b.validationMessage);
Ditto.

https://codereview.chromium.org/1220833002/diff/80001/LayoutTests/fast/forms/...
File LayoutTests/fast/forms/ValidityState-customError.html (left):

https://codereview.chromium.org/1220833002/diff/80001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/ValidityState-customError.html:107:
shouldBeFalse('customErrorFor("fieldset-many-changes")');
Do you know why <fieldset> and <button> are behaving differently than the rest
here? Is there an actual difference in behavior, or is the test doing something
strange?

[tkent, 2015-07-08 00:31:06]

https://codereview.chromium.org/1220833002/diff/80001/LayoutTests/fast/forms/...
File LayoutTests/fast/forms/ValidityState-customError.html (left):

https://codereview.chromium.org/1220833002/diff/80001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/ValidityState-customError.html:107:
shouldBeFalse('customErrorFor("fieldset-many-changes")');
On 2015/07/07 11:48:52, philipj wrote:
> Do you know why <fieldset> and <button> are behaving differently than the rest
> here? Is there an actual difference in behavior, or is the test doing
something
> strange?

<fieldset> is not a candidate for constraint validation [1], and the custom
validation message is never used by UA.  The specification is not clear enough
about validity.customError availability for <fieldset>.  IMO, this issue is
ignorable in this CL.

[1]
https://html.spec.whatwg.org/multipage/forms.html#candidate-for-constraint-va...

[philipj_slow, 2015-07-08 08:08:46]

https://codereview.chromium.org/1220833002/diff/80001/LayoutTests/fast/forms/...
File LayoutTests/fast/forms/ValidityState-customError.html (left):

https://codereview.chromium.org/1220833002/diff/80001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/ValidityState-customError.html:107:
shouldBeFalse('customErrorFor("fieldset-many-changes")');
On 2015/07/08 00:31:06, tkent wrote:
> On 2015/07/07 11:48:52, philipj wrote:
> > Do you know why <fieldset> and <button> are behaving differently than the
rest
> > here? Is there an actual difference in behavior, or is the test doing
> something
> > strange?
> 
> <fieldset> is not a candidate for constraint validation [1], and the custom
> validation message is never used by UA.  The specification is not clear enough
> about validity.customError availability for <fieldset>.  IMO, this issue is
> ignorable in this CL.
> 
> [1]
>
https://html.spec.whatwg.org/multipage/forms.html#candidate-for-constraint-va...

Interesting, looks like HTMLFieldset.setCustomValidity() doesn't actually do
anything. Out of scope for the CL, I agree.

[shiva.jm, 2015-07-10 04:35:03]

Update the tests and review comments, pls have a look.

[shiva.jm, 2015-07-10 05:55:45]

Fixed the tests failed in last patch, pls have a look.

[philipj_slow, 2015-07-10 08:45:27]

The CQ bit was checked by philipj@opera.com

[philipj_slow, 2015-07-10 08:45:27]

lgtm

[commit-bot: I haz the power, 2015-07-10 08:45:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1220833002/120001

[philipj_slow, 2015-07-10 08:45:55]

The CQ bit was unchecked by philipj@opera.com

[philipj_slow, 2015-07-10 08:48:26]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2015-07-10 08:48:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1220833002/120001

[philipj_slow, 2015-07-10 08:49:45]

Some test nits, but this is already going through CQ so never mind. If you like
you can fix them in a followup.

https://codereview.chromium.org/1220833002/diff/120001/LayoutTests/fast/dom/s...
File LayoutTests/fast/dom/setCustomValidity-null-undefined.html (right):

https://codereview.chromium.org/1220833002/diff/120001/LayoutTests/fast/dom/s...
LayoutTests/fast/dom/setCustomValidity-null-undefined.html:2: <html>
You can omit <html>:
https://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-...

https://codereview.chromium.org/1220833002/diff/120001/LayoutTests/fast/dom/s...
LayoutTests/fast/dom/setCustomValidity-null-undefined.html:8: test(function() {
Should be indented.

[commit-bot: I haz the power, 2015-07-10 08:51:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-10 08:51:50]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)

[shiva.jm, 2015-07-10 08:58:03]

On 2015/07/10 08:49:45, philipj wrote:
> Some test nits, but this is already going through CQ so never mind. If you
like
> you can fix them in a followup.
> 
>
https://codereview.chromium.org/1220833002/diff/120001/LayoutTests/fast/dom/s...
> File LayoutTests/fast/dom/setCustomValidity-null-undefined.html (right):
> 
>
https://codereview.chromium.org/1220833002/diff/120001/LayoutTests/fast/dom/s...
> LayoutTests/fast/dom/setCustomValidity-null-undefined.html:2: <html>
> You can omit <html>:
>
https://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-...
> 
>
https://codereview.chromium.org/1220833002/diff/120001/LayoutTests/fast/dom/s...
> LayoutTests/fast/dom/setCustomValidity-null-undefined.html:8: test(function()
{
> Should be indented.

Ok, will fix the nits and also will remove the expected.txt as presubmit is
failing.

[shiva.jm, 2015-07-10 09:09:28]

Updated nits and presubmit check error, pls have a look.

https://codereview.chromium.org/1220833002/diff/120001/LayoutTests/fast/dom/s...
File LayoutTests/fast/dom/setCustomValidity-null-undefined.html (right):

https://codereview.chromium.org/1220833002/diff/120001/LayoutTests/fast/dom/s...
LayoutTests/fast/dom/setCustomValidity-null-undefined.html:2: <html>
On 2015/07/10 08:49:45, philipj wrote:
> You can omit <html>:
>
https://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-...

Done.

https://codereview.chromium.org/1220833002/diff/120001/LayoutTests/fast/dom/s...
LayoutTests/fast/dom/setCustomValidity-null-undefined.html:8: test(function() {
On 2015/07/10 08:49:45, philipj wrote:
> Should be indented.

Done.

[shiva.jm, 2015-07-10 09:09:45]

The CQ bit was checked by shiva.jm@samsung.com to run a CQ dry run

[shiva.jm, 2015-07-10 09:09:45]

The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com
Link to the patchset: https://codereview.chromium.org/1220833002/#ps140001
(title: " ")

[commit-bot: I haz the power, 2015-07-10 09:10:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1220833002/140001

[philipj_slow, 2015-07-10 09:20:01]

lgtm

[commit-bot: I haz the power, 2015-07-10 10:33:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-10 10:33:13]

Dry run: This issue passed the CQ dry run.

[shiva.jm, 2015-07-10 10:35:22]

The CQ bit was checked by shiva.jm@samsung.com

[commit-bot: I haz the power, 2015-07-10 10:35:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1220833002/140001

[commit-bot: I haz the power, 2015-07-10 10:39:27]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198678