Bug 47834 – Refactor HTMLInputElement: Move createRender(), appendFormData(), saveFormControlState() and restoreFormControlState() to InputTypes.

• Component: Forms
• Status: RESOLVED FIXED

[Kent Tamura]

Refactor HTMLInputElement: Move createRender(), appendFormData(), saveFormControlState() and restoreFormControlState() to InputTypes.

[Kent Tamura]

Created attachment 71055 [details]
Patch

[Early Warning System Bot]

Attachment 71055 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4482030

[Darin Adler]

Comment on attachment 71055 [details]
Patch

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

I think we should move the checks for empty name() into the individual appendFormData functions.

review- because I had enough comments plus the Qt build seems to have failed.

> WebCore/html/BaseButtonInputType.cpp:3
> + * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010 Apple Inc. All rights reserved.
> + * Copyright (C) 2010 Google Inc. All rights reserved.

Too much copyright for the contents of this file. I don’t think you need any of those Apple copyrights for the one or two lines of code here.

> WebCore/html/BaseCheckableInputType.cpp:55
> +RenderObject* BaseCheckableInputType::createRenderer(RenderArena*, RenderStyle* style) const
> +{
> +    return RenderObject::createObject(element(), style);
> +}

Can this default implementation go into InputType instead of BaseCheckableInputType?

> WebCore/html/BaseCheckableInputType.h:45
> +    virtual bool saveFormControlState(String&) const;
> +    virtual void restoreFormControlState(const String&) const;
> +    virtual bool appendFormData(FormDataList&, bool) const;
> +    virtual RenderObject* createRenderer(RenderArena*, RenderStyle*) const;

I think these can all be private rather than protected.

> WebCore/html/HTMLFormControlElement.h:218
> +    friend class TextFieldInputType;

Adding this friendship seems like the wrong approach. If we need one protected function to be public, then please do that instead.

> WebCore/html/HTMLInputElement.cpp:812
> +    // The image type generates its own names, but for other types there is no
> +    // form data unless there's a name.
> +    if (name().isEmpty() && !isImageButton())
>          return false;

It’s annoying to be left with an explicit isImageButton check here. The point of the InputType refactoring is to eliminate such things. Maybe we need another virtual function for this?

> WebCore/html/HTMLInputElement.h:370
> +    friend class ImageInputType;
> +    friend class SubmitInputType;

I understand if we do need these friends, but please also do whatever you can to avoid them. Having one or two public functions that could otherwise be private is almost always preferable to having to friend another class, even in cases like this.

> WebCore/html/ImageInputType.cpp:45
> +    if (!element()->m_activeSubmit)
> +        return false;

I’d like to see this accessed through a function, not a direct access to a data member.

> WebCore/html/InputType.h:58
> +    // -------- Type query functions

I don’t think this "--------" formatting adds much. Can’t we just use normal comments for these sections?

[Kent Tamura]

Created attachment 71536 [details]
Patch 2

[Kent Tamura]

Comment on attachment 71055 [details]
Patch

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

Thank you for reviewing.

>> WebCore/html/BaseButtonInputType.cpp:3
>> + * Copyright (C) 2010 Google Inc. All rights reserved.
> 
> Too much copyright for the contents of this file. I don’t think you need any of those Apple copyrights for the one or two lines of code here.

I see.  I changed copyright headers with trivial existing code to "2010 Google Inc." only.

>> WebCore/html/BaseCheckableInputType.cpp:55
>> +}
> 
> Can this default implementation go into InputType instead of BaseCheckableInputType?

Done.
Yes, RenderObject::createObject() should be the default.

>> WebCore/html/BaseCheckableInputType.h:45
>> +    virtual RenderObject* createRenderer(RenderArena*, RenderStyle*) const;
> 
> I think these can all be private rather than protected.

Done.

>> WebCore/html/HTMLFormControlElement.h:218
>> +    friend class TextFieldInputType;
> 
> Adding this friendship seems like the wrong approach. If we need one protected function to be public, then please do that instead.

Done.
I introduced placeholderShouldBeVisible() public and remove the friend declaration.

>> WebCore/html/HTMLInputElement.cpp:812
>>          return false;
> 
> It’s annoying to be left with an explicit isImageButton check here. The point of the InputType refactoring is to eliminate such things. Maybe we need another virtual function for this?

Done.
I introduced isFormDataAppendable().

>> WebCore/html/HTMLInputElement.h:370
>> +    friend class SubmitInputType;
> 
> I understand if we do need these friends, but please also do whatever you can to avoid them. Having one or two public functions that could otherwise be private is almost always preferable to having to friend another class, even in cases like this.

Done.
I introduced new functions and removed the friends.

>> WebCore/html/InputType.h:58
>> +    // -------- Type query functions
> 
> I don’t think this "--------" formatting adds much. Can’t we just use normal comments for these sections?

Done.

[Darin Adler]

Comment on attachment 71536 [details]
Patch 2

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

> WebCore/html/HTMLInputElement.h:206
> +    bool activeSubmit() const { return m_activeSubmit; }

This data member and function don’t have a great name. In fact I have no idea what the boolean means.

> WebCore/html/InputType.h:49
> +// One of InputType classes represents a type of HTMLInputElement.
> +// Do not expose insntances of InputType and its subclasses to classes other
> +// than HTMLInputElement.

The phrase “one of InputType classes” is not quite right here. I would word it more like this, “An InputType object represents the type-specific part of an HTMLInputElement.”

Typo: "insntances".

The term “subclasses” is not a C++ term. Instead we should say “InputType and classes derived from it”.

[Kent Tamura]

Committed r70511: <http://trac.webkit.org/changeset/70511>

[Kent Tamura]

(In reply to comment #6)
> (From update of attachment 71536 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71536&action=review
> 
> > WebCore/html/HTMLInputElement.h:206
> > +    bool activeSubmit() const { return m_activeSubmit; }
> 
> This data member and function don’t have a great name. In fact I have no idea what the boolean means.

I found we already had HTMLInputELement::isActivatedSubmit(), and removed activeSubmit().

> > WebCore/html/InputType.h:49
> > +// One of InputType classes represents a type of HTMLInputElement.
> > +// Do not expose insntances of InputType and its subclasses to classes other
> > +// than HTMLInputElement.
> 
> The phrase “one of InputType classes” is not quite right here. I would word it more like this, “An InputType object represents the type-specific part of an HTMLInputElement.”
> 
> Typo: "insntances".
> 
> The term “subclasses” is not a C++ term. Instead we should say “InputType and classes derived from it”.

Fixed.  Thank you!.