[webkit-reviews] review denied: [Bug 45872] Refactor HTMLInputElement : [Attachment 68337] Patch 2

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Kent Tamura <tkent at chromium.org>'s
request for review:
Bug 45872: Refactor HTMLInputElement
https://bugs.webkit.org/show_bug.cgi?id=45872

Attachment 68337: Patch 2
https://bugs.webkit.org/attachment.cgi?id=68337&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.