[webkit-reviews] review denied: [Bug 25551] Support for HTML5 Forms "required" attribute : [Attachment 32950] Proposed patch, rev. 4b

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Michelangelo De Simone
<micdesim at gmail.com>'s request for review:
Bug 25551: Support for HTML5 Forms "required" attribute
https://bugs.webkit.org/show_bug.cgi?id=25551

Attachment 32950: Proposed patch, rev. 4b
https://bugs.webkit.org/attachment.cgi?id=32950&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +	       case CSSSelector::PseudoOptional: {
> +		   if (!e || !e->isFormControlElement())
> +		       return false;
> +
> +		   return e->isOptionalFormControl();
> +	       }

No braces needed here.

Since you put isOptionalFormControl into Element, you don't need so much code.
Just:

    case CSSSelector::PseudoOptional:
	return e && e->isOptionalFormControl();

Same for PseudoRequired.

> +    virtual bool isOptionalFormControl() const { return true; }

I prefer to make such virtual function overrides in the private: section of a
class. If anyone calls this on a button element it means they made a
programming mistake and I think it's best if the code fails to compile in such
cases that can easily be caught. Making the virtual function override private
accomplishes that.

> +bool HTMLInputElement::isRequiredFormControl() const
> +{
> +    if (!required())
> +	   return false;
> +
> +    switch (inputType()) {
> +	   case HTMLInputElement::HIDDEN:
> +	   case HTMLInputElement::RANGE:
> +	   case HTMLInputElement::SUBMIT:
> +	   case HTMLInputElement::IMAGE:
> +	   case HTMLInputElement::RESET:
> +	   case HTMLInputElement::BUTTON:
> +	       return false;
> +	   default:
> +	       return true;
> +    }
> +
> +    return false;
> +}

There is no need to have that "return false" statement there if there is no
exit from the switch statement, and in fact I believe this will fail to compile
with some compilers due to the unreachable code.

The enum values don't need to be qualified by the HTMLInputElement class name,
because this is already a member function. For maintenance I prefer to have all
the values of the enum listed. If you do that and omit the default from the
switch, then the gcc compiler will catch you if you add a new enum value
without modifying this function.

If you remove the default, then you will need the return after the switch,
though, although you can do an ASSERT_NOT_REACHED. The saveForMControlState
function uses the preferred style.

> +    virtual bool isOptionalFormControl() const { return
!isRequiredFormControl(); }
> +    virtual bool isRequiredFormControl() const;

Same thought about making these private.

> +    virtual bool isOptionalFormControl() const { return true; }

Ditto.

> +    virtual bool isOptionalFormControl() const { return
!isRequiredFormControl(); }
> +    virtual bool isRequiredFormControl() const { return required(); }

Ditto.

> +	   switch (i->inputType()) {
> +	       case HTMLInputElement::TEXT:
> +	       case HTMLInputElement::SEARCH:
> +	       case HTMLInputElement::URL:
> +	       case HTMLInputElement::TELEPHONE:
> +	       case HTMLInputElement::EMAIL:
> +	       case HTMLInputElement::PASSWORD:
> +	       case HTMLInputElement::NUMBER:
> +		   return i->value().isEmpty();
> +	       case HTMLInputElement::FILE:
> +		   return i->files()->isEmpty();
> +	       case HTMLInputElement::CHECKBOX:
> +		   return !i->checked();
> +	       case HTMLInputElement::RADIO:
> +		   return
!i->document()->checkedRadioButtons().checkedButtonForGroup(i->name());
> +	       default:
> +		   return false;

Same thought about omitting the default case from this.

Again, would this be done better with code in the various element classes and a
virtual function rather than all this code specific per-type here in the
ValidityState class?

I think those are enough comments that I'll say review- and give you a chance
to consider the comments. I think all the comments I made are optional, so
please do use your own best judgement.