Bug 45872 – Refactor HTMLInputElement

• Component: Forms
• Status: RESOLVED FIXED

[Kent Tamura]

Refactor HTMLInputElement

[Kent Tamura]

Created attachment 67768 [details]
Patch

[Kent Tamura]

- We will introduce TypeHandler subclasses for each input types because of HTMLInputElement::formControlName().

- We can move type-specific data members of HTMLInputElement such as m_xPos m_yPos m_maxResults m_imageLoader m_fileList to TypeHandler classes.

[Adam Barth]

I like the approach.  I'd have to look at it when I'm more awake to review it properly.

[Dimitri Glazkov (Google)]

Comment on attachment 67768 [details]
Patch

Did you forget to include new files?

[Dimitri Glazkov (Google)]

(In reply to comment #4)
> (From update of attachment 67768 [details])
> Did you forget to include new files?

Oh, nevermind :) Sorry, need to wake up first.

[Dimitri Glazkov (Google)]

Comment on attachment 67768 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=67768&action=prettypatch

Yay! This is exactly what we need. However, I think you're not handling a case of changing input type by setting the value of the "input" attribute. Even though the input may be born as a text input, it could change to any other type during its lifecycle.

> WebCore/html/HTMLInputElement.cpp:2957
> +// Type handlers ---------------------------------------------------------------

This should live either in separate files (preferred) or with TypeHandle (InputType).

> WebCore/html/HTMLInputElement.h:41
> +class TypeHandler {

This needs to be moved to its own file. Also, can we name it something more specific, like InputType? Then, the subclasses could be TextInputType, CheckboxInputType, etc.

[Dimitri Glazkov (Google)]

> This needs to be moved to its own file. Also, can we name it something more specific, like InputType? Then, the subclasses could be TextInputType, CheckboxInputType, etc.

Ok, InputType is already taken. How about InputElementType?

[Darin Adler]

Comment on attachment 67768 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=67768&action=prettypatch

The setInputType function will need to create a new instance of one of these objects and destroy the old one.

I agree with Dmitri that the objects should be called InputType even though that conflicts with the existing enum. These objects are going to replace the enum. At first, you could name the base class InputTypeBase, and then later give it the name InputType once we eliminate the enum.

There are two competing design patterns here. In one pattern, each function of the new class takes an HTMLInputElement*. In another, the new class contains an HTMLInputElement*. I suppose having the new class contain an HTMLInputElement* member is OK.

I suggest refactoring clients of HTMLInputElement so they no longer use the inputType() function. Outside of the HTMLInputElement class itself I see only 15 call sites in WebCore and all would be easy to fix and just a few in WebKit that check for password.

Oh, wait, Chromium seems to expose the input type enum as public API!? That should be fixed. The Chromium WebKit code seems to use the inputType() function a lot! Well, regardless, I think we can eliminate all clients except for WebInputElement::inputType quite easily, and maybe we can do something about that one.

> WebCore/html/HTMLInputElement.h:52
> +    HTMLInputElement* input() const { return m_input; }

I would name this function element() rather than input().

> WebCore/html/HTMLInputElement.h:56
> +    HTMLInputElement* m_input;

I would name this data member element() rather than input().

[Dimitri Glazkov (Google)]

(In reply to comment #8)
> (From update of attachment 67768 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67768&action=prettypatch
> 
> The setInputType function will need to create a new instance of one of these objects and destroy the old one.
> 
> I agree with Dmitri that the objects should be called InputType even though that conflicts with the existing enum. These objects are going to replace the enum. At first, you could name the base class InputTypeBase, and then later give it the name InputType once we eliminate the enum.
> 
> There are two competing design patterns here. In one pattern, each function of the new class takes an HTMLInputElement*. In another, the new class contains an HTMLInputElement*. I suppose having the new class contain an HTMLInputElement* member is OK.

I've been thinking about this and from the ownership/clarity perspective, it might be good to just always pass input (as InputElement*) when we need to query value etc.

> 
> I suggest refactoring clients of HTMLInputElement so they no longer use the inputType() function. Outside of the HTMLInputElement class itself I see only 15 call sites in WebCore and all would be easy to fix and just a few in WebKit that check for password.
> 
> Oh, wait, Chromium seems to expose the input type enum as public API!? That should be fixed. The Chromium WebKit code seems to use the inputType() function a lot! Well, regardless, I think we can eliminate all clients except for WebInputElement::inputType quite easily, and maybe we can do something about that one.

Yes, that's bad stuff. We need to get rid of that.

> 
> > WebCore/html/HTMLInputElement.h:52
> > +    HTMLInputElement* input() const { return m_input; }
> 
> I would name this function element() rather than input().
> 
> > WebCore/html/HTMLInputElement.h:56
> > +    HTMLInputElement* m_input;
> 
> I would name this data member element() rather than input().

[Darin Adler]

I started work on removing callers of inputType. See bug 45903.

[Kent Tamura]

Thank you for the comments, and thank you for fixing InputType usage, Darin.

TODO summary:

In short term (next patch):
- Move new classes to a separated file
  though introducing a new source file is the most painful task in WebKit development ;-(

- Rename TypeHandler to InputTypeBase

- Should HTMLInputElement* be a data member or a parameter?
-- data member:
  Extra memory is needed to store the member.
-- parameter:
  Extra runtime cost and code size are needed to pass and receive the parameter.
  We can't use singletons for InputTypeBase objects even in this case because we'll move m_xPos m_yPos ... m_fileList to InputType.

I like data member because it would be simple.


In long term:
- Remove InputType enum dependency from Chromium port
- Rename InputTypeBase class to InputType

[Darin Adler]

I suggest we rename InputType and inputType() and m_type to DeprecatedInputType, deprecatedInputType() and m_numericType as soon as possible, to free up the InputType class name, type() function name and m_type member name for private use within the HTMLInputElement class.

I think one of the major issues may end up being access from the InputType classes to private things in HTMLInputElement that we would not want to expose to other classes. Because of this, we may need to make some InputType classes be friends of HTMLInputElement.

> - Move new classes to a separated file
>   though introducing a new source file is the most painful task in WebKit development ;-(

I would like to see each InputType class in a separate source file. Since adding source files can be a chore, I suggest we do this work up front.

> - Rename TypeHandler to InputTypeBase

Lets name it InputType.

> - Should HTMLInputElement* be a data member or a parameter?
> -- data member:
>   Extra memory is needed to store the member.
> -- parameter:
>   Extra runtime cost and code size are needed to pass and receive the parameter.
>   We can't use singletons for InputTypeBase objects even in this case because we'll move m_xPos m_yPos ... m_fileList to InputType.

I’m not sure it’s OK to move that data to the InputType object. It’s only OK if that data can be destroyed when you change the type of the input element. If the data needs to survive when you change the type and back, then the data needs to be stored outside the InputType object. We should construct some tests and see what our browser and other browsers do in these respects.

> In long term:
> - Remove InputType enum dependency from Chromium port
> - Rename InputTypeBase class to InputType

The first of these can be long term, but the second one I think we should do right at the start.

[Darin Adler]

Bug 46023 contains a patch that changes the old input type to add a “deprecated” prefix to leave the shorter cleaner names free for use for the refactoring.

[Kent Tamura]

Created attachment 68243 [details]
Work-in-progress patch

[Ryosuke Niwa]

Comment on attachment 68243 [details]
Work-in-progress patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68243&action=review

> WebCore/html/InputType.cpp:422
> +namespace InputTypeNames {
> +
> +const AtomicString& button()
> +{
> +    DEFINE_STATIC_LOCAL(AtomicString, name, ("button"));
> +    return name;
> +}

Is there a reason why these names are wrapped in a function?  Can't we declare all of them as constant static variables?

[Darin Adler]

(In reply to comment #15)
> Is there a reason why these names are wrapped in a function?  Can't we declare all of them as constant static variables?

We don’t allow global constructors in the WebKit code for multiple reasons. One is the lack of control over order of initialization, and another is that initializing such objects slows down loading the WebKit library on platforms such as Mac OS X.

If we add a global constructor, the Mac build will fail because it contains a build step that checks for such things.

[Kent Tamura]

Created attachment 68337 [details]
Patch 2

[Darin Adler]

Comment on attachment 68337 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=68337&action=review

Great start! Please take another crack at it.

> WebCore/html/HTMLInputElement.cpp:165
> +    m_inputType = InputType::create(this, InputTypeNames::text());

Can we use constructor syntax instead?

Also, maybe it’s slightly more elegant to have a InputType::create function that does not take a type name or an InputType::createText, rather than passing the name text explicitly?

> WebCore/html/HTMLInputElement.h:29
> +#include "InputType.h"

This header should just forward-declare the InputType class, not include the whole header.

> WebCore/html/InputType.cpp:46
> +// Textfield-looking types including NUMBER.

I don’t think Textfield-looking is a good way to say this.

> WebCore/html/InputType.cpp:53
> +// Base of email, passoword, search, tel, text, and URL types.

Typo: passoword.

> WebCore/html/InputType.cpp:61
> +    {
> +        const AtomicString& pattern = element()->getAttribute(patternAttr);

This function body is too long to have inlined in the class definition. I suggest having the function definitions outside the class definitions.

> WebCore/html/InputType.cpp:75
> +class ButtonInputType : public InputType {

I have mixed feelings about having all this in a single file. We should really consider one file per input type.

> WebCore/html/InputType.cpp:365
> +typedef HashMap<AtomicString, PassOwnPtr<InputType> (*)(HTMLInputElement*), CaseFoldingHash> InputTypeFactoryMap;

I’m not sure the key should be AtomicString if it’s CaseFoldingHash. We can just have the key be String.

> WebCore/html/InputType.cpp:400
> +    PassOwnPtr<InputType> (*factory)(HTMLInputElement*);
> +    factory = factoryMap->get(typeName);

This definition and initialization should be done on a single line, not two.

> WebCore/html/InputType.cpp:414
> +InputType::InputType(HTMLInputElement* inputElement) : m_element(inputElement) {}
> +
> +InputType::~InputType() {}
> +
> +bool InputType::isTextField() const { return false; }
> +
> +bool InputType::isTextType() const { return false; }
> +
> +bool InputType::patternMismatch(const String&) const { return false; }

We don’t format functions on single lines like this except inside class definitions.

Also, we put spaces in between the braces when it’s like this { }.

> WebCore/html/InputType.h:34
> +#include <wtf/PassOwnPtr.h>

I think you can just include Forward.h instead of including PassOwnPtr.h.

> WebCore/html/InputType.h:36
> +#include <wtf/text/WTFString.h>

Not need to include WTFString if you’re already including AtomicString.

But I think you don’t need to include AtomicString.h. You can just forward-declare AtomicString for what’s needed here.

> WebCore/html/InputType.h:42
> +class InputType {

This should inherit from Noncopyable.

> WebCore/html/InputType.h:50
> +    virtual bool isTextField() const;
> +    virtual bool isTextType() const;
> +    virtual const AtomicString& typeName() const = 0;
> +    virtual bool patternMismatch(const String&) const;

What’s the ordering here? I’d like to see a more logical grouping and ordering if possible.

[Kent Tamura]

Thank you for the comments!

(In reply to comment #18)
> > WebCore/html/InputType.cpp:75
> > +class ButtonInputType : public InputType {
> 
> I have mixed feelings about having all this in a single file. We should really consider one file per input type.

I think it's good to have one file per one input type.

Do you think we should have separated files for common base classes, TextFieldInputType and BaseTextInputType? I think we need to introduce more common classes; radio/checkbox, date/time, button/reset/submit.


-------------------------------------------------------
> > WebCore/html/HTMLInputElement.cpp:165
> > +    m_inputType = InputType::create(this, InputTypeNames::text());
> 
> Can we use constructor syntax instead?

done.

> Also, maybe it’s slightly more elegant to have a InputType::create function that does not take a type name or an InputType::createText, rather than passing the name text explicitly?

done.  I made InputType::createText().

> > WebCore/html/HTMLInputElement.h:29
> > +#include "InputType.h"
> 
> This header should just forward-declare the InputType class, not include the whole header.

done.

> > WebCore/html/InputType.cpp:46
> > +// Textfield-looking types including NUMBER.
> 
> I don’t think Textfield-looking is a good way to say this.

I update the comment:
// The class represents types of which UI contain text fields.
// It supports not only the types for BaseTextInputType but also type=number.


> > WebCore/html/InputType.cpp:53
> > +// Base of email, passoword, search, tel, text, and URL types.
> 
> Typo: passoword.

done.

> > WebCore/html/InputType.cpp:61
> > +    {
> > +        const AtomicString& pattern = element()->getAttribute(patternAttr);
> 
> This function body is too long to have inlined in the class definition. I suggest having the function definitions outside the class definitions.

done.

> > WebCore/html/InputType.cpp:365
> > +typedef HashMap<AtomicString, PassOwnPtr<InputType> (*)(HTMLInputElement*), CaseFoldingHash> InputTypeFactoryMap;
> 
> I’m not sure the key should be AtomicString if it’s CaseFoldingHash. We can just have the key be String.

done. Changed it to String.

> > WebCore/html/InputType.cpp:400
> > +    PassOwnPtr<InputType> (*factory)(HTMLInputElement*);
> > +    factory = factoryMap->get(typeName);
> 
> This definition and initialization should be done on a single line, not two.

done.

> > WebCore/html/InputType.cpp:414
> > +InputType::InputType(HTMLInputElement* inputElement) : m_element(inputElement) {}
> > +
> > +InputType::~InputType() {}
> > +
> > +bool InputType::isTextField() const { return false; }
> > +
> > +bool InputType::isTextType() const { return false; }
> > +
> > +bool InputType::patternMismatch(const String&) const { return false; }
> 
> We don’t format functions on single lines like this except inside class definitions.

done.  I haven't known this rule.

> > WebCore/html/InputType.h:34
> > +#include <wtf/PassOwnPtr.h>
> 
> I think you can just include Forward.h instead of including PassOwnPtr.h.

done.  I haven't known about Forward.h.  Oops.

> > WebCore/html/InputType.h:36
> > +#include <wtf/text/WTFString.h>
> 
> Not need to include WTFString if you’re already including AtomicString.
> But I think you don’t need to include AtomicString.h. You can just forward-declare AtomicString for what’s needed here.

We don't need this line anymore because of Forward.h.

> > WebCore/html/InputType.h:42
> > +class InputType {
> 
> This should inherit from Noncopyable.

done.

> > WebCore/html/InputType.h:50
> > +    virtual bool isTextField() const;
> > +    virtual bool isTextType() const;
> > +    virtual const AtomicString& typeName() const = 0;
> > +    virtual bool patternMismatch(const String&) const;
> 
> What’s the ordering here? I’d like to see a more logical grouping and ordering if possible.

The order is:
 - type checking functions, and
 - others :-)
I inserted a blank line before patternMismatch().

[Kent Tamura]

Created attachment 68651 [details]
Patch 3 (not separated yet)

[Darin Adler]

(In reply to comment #19)
> Do you think we should have separated files for common base classes, TextFieldInputType and BaseTextInputType? I think we need to introduce more common classes; radio/checkbox, date/time, button/reset/submit.

Sure, I’d suggest a file per class, even if the classes are pretty small. We might regret it, but we’ve done it elsewhere and I think the results are usually good.

[Kent Tamura]

Created attachment 69035 [details]
Patch 4

[Darin Adler]

Comment on attachment 69035 [details]
Patch 4

View in context: https://bugs.webkit.org/attachment.cgi?id=69035&action=review

Good work. I like where this is going.

> WebCore/html/BaseTextInputType.cpp:8
> + * Copyright (C) 1999 Lars Knoll ([e-mail])
> + *           (C) 1999 Antti Koivisto ([e-mail])
> + *           (C) 2001 Dirk Mueller ([e-mail])
> + * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010 Apple Inc. All rights reserved.
> + *           (C) 2006 Alexey Proskuryakov ([e-mail])
> + * Copyright (C) 2007 Samuel Weinig ([e-mail])
> + * Copyright (C) 2010 Google Inc. All rights reserved.

While it’s good to not remove copyrights, I think that putting this entire list on every file is probably not right; the code here doesn’t necessarily have contributions from all these people and dates. You should consider if you can be more precise.

> WebCore/html/BaseTextInputType.cpp:39
> +BaseTextInputType::BaseTextInputType(HTMLInputElement* inputElement)
> +    : TextFieldInputType(inputElement)

I’d just name this element rather than inputElement.

> WebCore/html/BaseTextInputType.cpp:50
> +    const AtomicString& pattern = element()->getAttribute(patternAttr);

Just a side note: Could use fastGetAttribute here.

> WebCore/html/BaseTextInputType.cpp:54
> +    RegularExpression patternRegExp(pattern, TextCaseSensitive);

If it was me, I wouldn’t put this into a local variable. I’d just write:

    int matchOffset = RegularExpression(pattern, TextCaseSensitive).match(value, 0, &matchLength);

> WebCore/html/BaseTextInputType.cpp:56
> +    int valueLength = value.length();

I don’t see any reason to put value.length() into a local variable? Is it to avoid a typecast?

> WebCore/html/BaseTextInputType.h:42
> +    BaseTextInputType(HTMLInputElement*);

This should be protected, not public.

> WebCore/html/BaseTextInputType.h:44
> +    virtual bool isTextType() const;
> +    virtual bool patternMismatch(const String&) const;

These should be private, not public.

> WebCore/html/ButtonInputType.cpp:34
> +PassOwnPtr<InputType> ButtonInputType::create(HTMLInputElement* inputElement)

Again, element instead of inputElement would be cleaner. I won’t repeat this every time!

> WebCore/html/ButtonInputType.cpp:42
> +ButtonInputType::ButtonInputType(HTMLInputElement* inputElement)
> +    : InputType(inputElement)
> +{
> +}

I suggest marking this inline and defining it before the create function. We’d like it to be inlined into its single caller.

> WebCore/html/ImageInputType.h:44
> +    virtual const AtomicString& formControlType() const { return InputTypeNames::image(); }

I’m not sure it’s good to define these functions in the header files. They are virtual functions so they won’t get called inline. Probably better to use a function definition in the .cpp file instead for these.

> WebCore/html/InputType.cpp:95
> +    PassOwnPtr<InputType> (*factory)(HTMLInputElement*) = factoryMap->get(typeName);

Are we guaranteed that the type name is not the null string? I believe we’ll get a crash at runtime if we call get with a null string.

[Kent Tamura]

Thank you for reviewing.  I landed it with the following changes as http://trac.webkit.org/changeset/68629


(In reply to comment #23)
> > WebCore/html/BaseTextInputType.cpp:8
> > + * Copyright (C) 1999 Lars Knoll ([e-mail])
> > + *           (C) 1999 Antti Koivisto ([e-mail])
> > + *           (C) 2001 Dirk Mueller ([e-mail])
> > + * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010 Apple Inc. All rights reserved.
> > + *           (C) 2006 Alexey Proskuryakov ([e-mail])
> > + * Copyright (C) 2007 Samuel Weinig ([e-mail])
> > + * Copyright (C) 2010 Google Inc. All rights reserved.
> 
> While it’s good to not remove copyrights, I think that putting this entire list on every file is probably not right; the code here doesn’t necessarily have contributions from all these people and dates. You should consider if you can be more precise.

Ok, all code except BaseTextInputType::patternMismatch() was written by me.  So I added Michelangelo's copyright header to BaseTextInputType.cpp, and Google header to others.

> > WebCore/html/BaseTextInputType.cpp:39
> > +BaseTextInputType::BaseTextInputType(HTMLInputElement* inputElement)
> > +    : TextFieldInputType(inputElement)
> 
> I’d just name this element rather than inputElement.

Changed all of  "inputElement" in this change to "element".

> > WebCore/html/BaseTextInputType.cpp:50
> > +    const AtomicString& pattern = element()->getAttribute(patternAttr);
> 
> Just a side note: Could use fastGetAttribute here.

Done.

> > WebCore/html/BaseTextInputType.cpp:54
> > +    RegularExpression patternRegExp(pattern, TextCaseSensitive);
> 
> If it was me, I wouldn’t put this into a local variable. I’d just write:
> 
>     int matchOffset = RegularExpression(pattern, TextCaseSensitive).match(value, 0, &matchLength);

Done.

> > WebCore/html/BaseTextInputType.cpp:56
> > +    int valueLength = value.length();
> 
> I don’t see any reason to put value.length() into a local variable? Is it to avoid a typecast?

This is not my code.
valueLength is used twice.  So it would be better than calling value.length() twice.

> > WebCore/html/BaseTextInputType.h:42
> > +    BaseTextInputType(HTMLInputElement*);
> 
> This should be protected, not public.

Done.

> > WebCore/html/BaseTextInputType.h:44
> > +    virtual bool isTextType() const;
> > +    virtual bool patternMismatch(const String&) const;
> 
> These should be private, not public.

Done.

> > WebCore/html/ButtonInputType.cpp:42
> > +ButtonInputType::ButtonInputType(HTMLInputElement* inputElement)
> > +    : InputType(inputElement)
> > +{
> > +}
> 
> I suggest marking this inline and defining it before the create function. We’d like it to be inlined into its single caller.

Ok, I made all constructors inline definitions, and move all virtual function definitions to .cpp.

> > WebCore/html/InputType.cpp:95
> > +    PassOwnPtr<InputType> (*factory)(HTMLInputElement*) = factoryMap->get(typeName);
> 
> Are we guaranteed that the type name is not the null string? I believe we’ll get a crash at runtime if we call get with a null string.

I added null checking.