[webkit-reviews] review denied: [Bug 29004] [HTML5][Forms] simple implementation of date&time types of INPUT element : [Attachment 42942] Recognize date&time types as text fields (rev.7)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied TAMURA, Kent <tkent at chromium.org>'s
request for review:
Bug 29004: [HTML5][Forms] simple implementation of date&time types of INPUT
element
https://bugs.webkit.org/show_bug.cgi?id=29004

Attachment 42942: Recognize date&time types as text fields (rev.7)
https://bugs.webkit.org/attachment.cgi?id=42942&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    // get() or contains() with a null string causes a crash.
> +    if (!t.isNull() && typeMap->contains(t))
> +	   newType = typeMap->get(t);
>      else
>	   newType = TEXT;

It's not a good idiom for performance to do a contains followed by a separate
get. Those are two completely separate hash table lookups. There are two
alternatives:

    1) Just call get without first checking contains. The result will be the
default value, 0, which is TEXT.
    2) Use find instead of contains/get.

I think "causes a crash" is also not the perfect way to explain the issue.
Callers are not allowed to call get or contains with a null string. It's not
guaranteed to cause a crash, though!

I also think the comment is a little strange because there are tons of call
sites for this and I don't think we want to add a comment like this at every
one.

> +    // The values in the map must be lowercased because they will be the
> +    // return values of input.type and it must be lowercase according to DOM
Level 2.
> +    types->add(HTMLInputElement::BUTTON, "button");
> +    types->add(HTMLInputElement::CHECKBOX, "checkbox");
> +    types->add(HTMLInputElement::COLOR, "color");
> +    types->add(HTMLInputElement::DATE, "date");
> +    types->add(HTMLInputElement::DATETIME, "datetime");
> +    types->add(HTMLInputElement::DATETIMELOCAL, "datetime-local");
> +    types->add(HTMLInputElement::EMAIL, "email");
> +    types->add(HTMLInputElement::FILE, "file");
> +    types->add(HTMLInputElement::HIDDEN, "hidden");
> +    types->add(HTMLInputElement::IMAGE, "image");
> +    types->add(HTMLInputElement::ISINDEX, emptyAtom);
> +    types->add(HTMLInputElement::MONTH, "month");
> +    types->add(HTMLInputElement::NUMBER, "number");
> +    types->add(HTMLInputElement::PASSWORD, "password");
> +    types->add(HTMLInputElement::RADIO, "radio");
> +    types->add(HTMLInputElement::RANGE, "range");
> +    types->add(HTMLInputElement::RESET, "reset");
> +    types->add(HTMLInputElement::SEARCH, "search");
> +    types->add(HTMLInputElement::SUBMIT, "submit");
> +    types->add(HTMLInputElement::TELEPHONE, "tel");
> +    types->add(HTMLInputElement::TEXT, "text");
> +    types->add(HTMLInputElement::TIME, "time");
> +    types->add(HTMLInputElement::URL, "url");
> +    types->add(HTMLInputElement::WEEK, "week");

These values are enum values, consecutive integers. This should be done with an
array, not a HashMap.

> +    static const FormControlMap* formControlTypes =
createFormControlTypes();
> +    ASSERT(formControlTypes->contains(inputType()));
> +    // Don't do "return formControlTypes->get(inputType())" here.
> +    // In that case, compilers generate code like:
> +    //   AtomicString str = formControlTypes->get(inputType());
> +    //   return &str;
> +    return formControlTypes->find(inputType())->second;

I don’t really like the comment above but it will be gone if you switch to an
array anyway.

>  bool HTMLInputElement::rendererIsNeeded(RenderStyle *style)
>  {
> -    switch (inputType()) {
> -	   case BUTTON:
> -	   case CHECKBOX:
> -	   case COLOR:
> -	   case EMAIL:
> -	   case FILE:
> -	   case IMAGE:
> -	   case ISINDEX:
> -	   case NUMBER:
> -	   case PASSWORD:
> -	   case RADIO:
> -	   case RANGE:
> -	   case RESET:
> -	   case SEARCH:
> -	   case SUBMIT:
> -	   case TELEPHONE:
> -	   case TEXT:
> -	   case URL:
> -	       return HTMLFormControlElementWithState::rendererIsNeeded(style);

> -	   case HIDDEN:
> -	       return false;
> -    }
> -    ASSERT(false);
> -    return false;
> +    if (inputType() == HIDDEN)
> +	   return false;
> +    return HTMLFormControlElementWithState::rendererIsNeeded(style);

This change is fine assuming that no future type will be like "hidden". I guess
that's a reasonable assumption.

The patch otherwise seems OK.