Bug 29796

Summary: .maxLength should return -1 if maxlength attribute is not specified
Product: WebKit Reporter: Kent Tamura <tkent>
Component: FormsAssignee: Kent Tamura <tkent>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://html5.org/tools/web-apps-tracker?from=3933&to=3934
Bug Depends on:    
Bug Blocks: 19264, 27454    
Attachments:
Description Flags
Proposed patch none

Description Kent Tamura 2009-09-27 23:21:11 PDT
HTML5 draft was updated so that .maxLength IDL attributes are signed, and shoud return -1 if the corresponding HTML attribute is missing.
Comment 1 Kent Tamura 2009-09-27 23:24:19 PDT
In Firefox 3.5 and Opera 10, input.maxLength returns -1 in that case.  In IE8, input.maxLength returns 2147483647.  They don't support for textarea.maxLength yet.

In WebKit, input.maxLength returns 524288 for now.
Comment 2 Kent Tamura 2009-09-28 22:39:37 PDT
HTML5 draft says:
> If, on the other hand, it fails or returns an out of range value, or if the attribute
> is absent, the default value must be returned instead, or −1 if there is no default
> value. On setting, if the value is negative, the user agent must fire an
> INDEX_SIZE_ERR exception. Otherwise, the given value must be converted to the
> shortest possible string representing the number as a valid non-negative integer
> and then that string must be used as the new content attribute value.

We may return "the default value".  So it's ok that HTMLInputElement.maxLength returns 524288 in WebKit, 2147483647 in IE8.
Comment 3 Kent Tamura 2009-09-28 22:42:40 PDT
Created attachment 40283 [details]
Proposed patch

The patch addresses
 - HTMLTextAreaElement.maxLength value without maxlength= attribute
 - INDEX_SIZE_ERR
Comment 4 Darin Adler 2009-09-29 11:29:36 PDT
Comment on attachment 40283 [details]
Proposed patch

> -void HTMLInputElement::setMaxLength(int _maxLength)
> +void HTMLInputElement::setMaxLength(int _maxLength, ExceptionCode& exceptionCode)
>  {
> -    setAttribute(maxlengthAttr, String::number(_maxLength));
> +    if (_maxLength < 0)
> +        exceptionCode = INDEX_SIZE_ERR;
> +    else
> +        setAttribute(maxlengthAttr, String::number(_maxLength));
>  }

Underscore in the argument name is usually something we'd want to avoid. One of the few abbreviations we use consistently in WebCore code is "ec" for the exception code. I think using exceptionCode instead is nicer, but I'd prefer to stay consistent.

I was under the impression that INDEX_SIZE_ERR was usually generated by having an attribute of type unsigned long in IDL and handled automatically by the bindings. Could you find another example and make sure you are doing this consistently?

r=me as-is, but please consider the comments above too.
Comment 5 WebKit Commit Bot 2009-09-29 14:26:01 PDT
Comment on attachment 40283 [details]
Proposed patch

Clearing flags on attachment: 40283

Committed r48903: <http://trac.webkit.org/changeset/48903>
Comment 6 WebKit Commit Bot 2009-09-29 14:26:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Kent Tamura 2009-09-29 21:10:49 PDT
(In reply to comment #4)
> (From update of attachment 40283 [details])
> > -void HTMLInputElement::setMaxLength(int _maxLength)
> > +void HTMLInputElement::setMaxLength(int _maxLength, ExceptionCode& exceptionCode)
> >  {
> > -    setAttribute(maxlengthAttr, String::number(_maxLength));
> > +    if (_maxLength < 0)
> > +        exceptionCode = INDEX_SIZE_ERR;
> > +    else
> > +        setAttribute(maxlengthAttr, String::number(_maxLength));
> >  }

> I was under the impression that INDEX_SIZE_ERR was usually generated by having
> an attribute of type unsigned long in IDL and handled automatically by the
> bindings. Could you find another example and make sure you are doing this
> consistently?

For example, HTMLTableElement::insertRow(long) and deleteRow(long)  have similar code.  I couldn't find examples of properties, not functions.
We need to use signed long for maxLength IDL definition because it can return -1.  I think we have no choices other than this code.