[webkit-reviews] review denied: [Bug 47834] Refactor HTMLInputElement: Move createRender(), appendFormData(), saveFormControlState() and restoreFormControlState() to InputTypes. : [Attachment 71055] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Kent Tamura <tkent at chromium.org>'s
request for review:
Bug 47834: Refactor HTMLInputElement: Move createRender(), appendFormData(),
saveFormControlState() and restoreFormControlState() to InputTypes.
https://bugs.webkit.org/show_bug.cgi?id=47834

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

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