Bug 47834 - Refactor HTMLInputElement: Move createRender(), appendFormData(), saveFormControlState() and restoreFormControlState() to InputTypes.
Summary: Refactor HTMLInputElement: Move createRender(), appendFormData(), saveFormCon...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Minor
Assignee: Kent Tamura
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-18 10:19 PDT by Kent Tamura
Modified: 2010-10-25 19:28 PDT (History)
3 users (show)

See Also:


Attachments
Patch (57.94 KB, patch)
2010-10-18 10:26 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 2 (54.94 KB, patch)
2010-10-22 01:38 PDT, Kent Tamura
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2010-10-18 10:19:35 PDT
Refactor HTMLInputElement: Move createRender(), appendFormData(), saveFormControlState() and restoreFormControlState() to InputTypes.
Comment 1 Kent Tamura 2010-10-18 10:26:52 PDT
Created attachment 71055 [details]
Patch
Comment 2 Early Warning System Bot 2010-10-18 10:47:57 PDT
Attachment 71055 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4482030
Comment 3 Darin Adler 2010-10-18 10:59:16 PDT
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?
Comment 4 Kent Tamura 2010-10-22 01:38:43 PDT
Created attachment 71536 [details]
Patch 2
Comment 5 Kent Tamura 2010-10-22 01:45:49 PDT
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.
Comment 6 Darin Adler 2010-10-25 11:47:38 PDT
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”.
Comment 7 Kent Tamura 2010-10-25 19:27:39 PDT
Committed r70511: <http://trac.webkit.org/changeset/70511>
Comment 8 Kent Tamura 2010-10-25 19:28:53 PDT
(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!.