[webkit-reviews] review granted: [Bug 34852] Refactor parsing/serialization functions in HTMLInputElement : [Attachment 48582] Proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted TAMURA, Kent <tkent at chromium.org>'s
request for review:
Bug 34852: Refactor parsing/serialization functions in HTMLInputElement
https://bugs.webkit.org/show_bug.cgi?id=34852

Attachment 48582: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=48582&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
I would prefer that we use references rather than pointers for the out
arguments to all these functions.

> +	   Refacotr parsing/serialization functions in HTMLInputElement.

Typo: Refacotr.

> +	   value. parseToDoubleForNumer() and parseToDateComponents()

Typo: parseToDoubleForNuer.

> +	   functions are helper functions for it. serializeForCurrentType()
> +	   is the top-level function to serialize a double value to a
> +	   type-dependent string, and serializeForNumberType() and
> +	   serializeForDateTimeTypes() are helper functions for it.

I think serializeForCurrentType could be named just serialize.

> +    ASSERT_NOT_REACHED();
> +    return;
> +}

It's not good style to have return at the end of the function. I suggest just
leaving it out.

> +    default:
> +	   ASSERT_NOT_REACHED();
> +	   return String();
>      }

Including a default case defeats the GCC warning that lets us know if we forget
to add a new enumeration value to the switch statement. If possible you should
restructure so we don't have te default. In fact, I think the warning is
slightly more important than the assertion, so I'd be willing to sacrifice
that.

r=me