Bug 34852 – Refactor parsing/serialization functions in HTMLInputElement

• Component: New Bugs
• Status: RESOLVED FIXED

[Kent Tamura]

Refacotr parsing/serialization functions in HTMLInputElement

[Kent Tamura]

Created attachment 48582 [details]
Proposed patch

[Darin Adler]

Comment on attachment 48582 [details]
Proposed patch

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

[Kent Tamura]

(In reply to comment #2)
> (From update of attachment 48582 [details])
> I would prefer that we use references rather than pointers for the out
> arguments to all these functions.

As for the pointer arguments of parseToDoubleForNumberType() and parseToDateComponents(), they are used for just check syntax of input stings, and resultant double and DateCompnents are not used in that case.  So pointers are reasonable.

> > +        Refacotr parsing/serialization functions in HTMLInputElement.
> Typo: Refacotr.

Fixed.

> > +        value. parseToDoubleForNumer() and parseToDateComponents()
> Typo: parseToDoubleForNuer.

Fixed.
I made too many typos...

> > +        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.

ok, renamed to 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.

Removed.

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

Removed default:


Landed as r54754.