Bug 38203 - Move number parsing code out of HTMLInputElement.
Summary: Move number parsing code out of HTMLInputElement.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 38140
  Show dependency treegraph
 
Reported: 2010-04-27 10:27 PDT by Yael
Modified: 2010-05-01 05:12 PDT (History)
2 users (show)

See Also:


Attachments
Patch. (6.24 KB, patch)
2010-04-27 11:00 PDT, Yael
darin: review-
darin: commit-queue-
Details | Formatted Diff | Diff
Patch (9.47 KB, patch)
2010-04-27 18:29 PDT, Yael
no flags Details | Formatted Diff | Diff
Update the JavaScriptCore.def for the new function. (10.19 KB, patch)
2010-04-28 14:23 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch. Based on comment #9. (9.49 KB, patch)
2010-04-29 16:08 PDT, Yael
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yael 2010-04-27 10:27:36 PDT
Move HTMLInputElement::parseToDoubleForNumberType and HTMLInputElement::serializeForNumberType to HTMLElement, so that other elements can reuse this code.
This was suggested in https://bugs.webkit.org/show_bug.cgi?id=38140#c12.
A patch is coming soon.
Comment 1 Yael 2010-04-27 11:00:17 PDT
Created attachment 54432 [details]
Patch.

This patch is simply moving the number handling methods from HTMLInputElement to HTMLElement.
Comment 2 Darin Adler 2010-04-27 12:42:02 PDT
Comment on attachment 54432 [details]
Patch.

If we're going to move these, I suggest we make them non-member functions. I see no reason for these to be members of any DOM class.
Comment 3 Yael 2010-04-27 18:29:44 PDT
Created attachment 54493 [details]
Patch

Moved HTMLInputElement::parseToDoubleForNumberType to String::toDoubleStrict .
Comment 4 Adam Roben (:aroben) 2010-04-28 13:47:17 PDT
Here are the errors on Windows:

13>WebCore.lib(HTMLElementsAllInOne.obj) : error LNK2019: unresolved external symbol "public: double __thiscall WebCore::String::toDoubleStrict(bool *)const " (?toDoubleStrict@String@WebCore@@QBENPA_N@Z) referenced in function "public: bool __thiscall WebCore::HTMLInputElement::stepMismatch(void)const " (?stepMismatch@HTMLInputElement@WebCore@@QBE_NXZ)
13>WebCore.lib(ValidityState.obj) : error LNK2001: unresolved external symbol "public: double __thiscall WebCore::String::toDoubleStrict(bool *)const " (?toDoubleStrict@String@WebCore@@QBENPA_N@Z)
13>WebCore.lib(StepRange.obj) : error LNK2001: unresolved external symbol "public: double __thiscall WebCore::String::toDoubleStrict(bool *)const " (?toDoubleStrict@String@WebCore@@QBENPA_N@Z)
Comment 5 Yael 2010-04-28 14:22:22 PDT
(In reply to comment #4)
> Here are the errors on Windows:
> 
> 13>WebCore.lib(HTMLElementsAllInOne.obj) : error LNK2019: unresolved external
> symbol "public: double __thiscall WebCore::String::toDoubleStrict(bool *)const
> " (?toDoubleStrict@String@WebCore@@QBENPA_N@Z) referenced in function "public:
> bool __thiscall WebCore::HTMLInputElement::stepMismatch(void)const "
> (?stepMismatch@HTMLInputElement@WebCore@@QBE_NXZ)
> 13>WebCore.lib(ValidityState.obj) : error LNK2001: unresolved external symbol
> "public: double __thiscall WebCore::String::toDoubleStrict(bool *)const "
> (?toDoubleStrict@String@WebCore@@QBENPA_N@Z)
> 13>WebCore.lib(StepRange.obj) : error LNK2001: unresolved external symbol
> "public: double __thiscall WebCore::String::toDoubleStrict(bool *)const "
> (?toDoubleStrict@String@WebCore@@QBENPA_N@Z)

Thank you for your help, Adam!
Comment 6 Yael 2010-04-28 14:23:49 PDT
Created attachment 54620 [details]
Update the JavaScriptCore.def for the new function.
Comment 7 Darin Adler 2010-04-28 16:14:49 PDT
Comment on attachment 54620 [details]
Update the JavaScriptCore.def for the new function.

I don't think that parsing a number in a way that is right for HTML is something that necessarily belongs in WTF. The particular strictness here is tied to HTML semantics. I think that having this be somewhere inside the WebCore project makes sense. I just don't think it should be a member function of a class. Not of any class, including String.
Comment 8 Yael 2010-04-28 17:23:28 PDT
(In reply to comment #7)
> (From update of attachment 54620 [details])
> I don't think that parsing a number in a way that is right for HTML is
> something that necessarily belongs in WTF. The particular strictness here is
> tied to HTML semantics. I think that having this be somewhere inside the
> WebCore project makes sense. I just don't think it should be a member function
> of a class. Not of any class, including String.

Thank you for the review.
I am thinking that the best place would probably be in HTMLElement, but not as a member.
Would you agree?
Comment 9 Kent Tamura 2010-04-29 07:16:36 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 54620 [details] [details])
> > I don't think that parsing a number in a way that is right for HTML is
> > something that necessarily belongs in WTF. The particular strictness here is
> > tied to HTML semantics. I think that having this be somewhere inside the
> > WebCore project makes sense. I just don't think it should be a member function
> > of a class. Not of any class, including String.
> 
> Thank you for the review.
> I am thinking that the best place would probably be in HTMLElement, but not as
> a member.
> Would you agree?

Moving to HTMLParser might be another choice, but it would be curious because we should move serializeForNumberType(double) too.
Comment 10 Yael 2010-04-29 14:35:34 PDT
(In reply to comment #9)
> Moving to HTMLParser might be another choice, but it would be curious because
> we should move serializeForNumberType(double) too.
Thanks Kent, I should move both methods to the HTMLParser.
Comment 11 Yael 2010-04-29 16:08:13 PDT
Created attachment 54750 [details]
Patch. Based on comment #9.

Moved the code to HTMLparser as suggested in comment #9.
Comment 12 Yael 2010-05-01 05:11:51 PDT
Committed r58635: <http://trac.webkit.org/changeset/58635>