[webkit-reviews] review granted: [Bug 45872] Refactor HTMLInputElement : [Attachment 69035] Patch 4

[bugzilla-daemon at webkit.org]

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

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

------- Additional Comments from Darin Adler <darin at apple.com>
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 (knoll at kde.org)
> + *		(C) 1999 Antti Koivisto (koivisto at kde.org)
> + *		(C) 2001 Dirk Mueller (mueller at kde.org)
> + * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010 Apple Inc. All
rights reserved.
> + *		(C) 2006 Alexey Proskuryakov (ap at nypop.com)
> + * Copyright (C) 2007 Samuel Weinig (sam at webkit.org)
> + * 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.