Bug 34852 - Refactor parsing/serialization functions in HTMLInputElement
Summary: Refactor parsing/serialization functions in HTMLInputElement
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-11 10:22 PST by Kent Tamura
Modified: 2010-02-13 12:02 PST (History)
1 user (show)

See Also:


Attachments
Proposed patch (16.42 KB, patch)
2010-02-11 10:36 PST, Kent Tamura
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2010-02-11 10:22:56 PST
Refacotr parsing/serialization functions in HTMLInputElement
Comment 1 Kent Tamura 2010-02-11 10:36:12 PST
Created attachment 48582 [details]
Proposed patch
Comment 2 Darin Adler 2010-02-12 09:51:50 PST
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
Comment 3 Kent Tamura 2010-02-13 12:02:36 PST
(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.