[webkit-reviews] review denied: [Bug 38688] Support control attribute of HTMLLabelElement : [Attachment 55614] Patch for addressing review comments.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Yael <yael.aharon at nokia.com>'s request
for review:
Bug 38688: Support control attribute of HTMLLabelElement
https://bugs.webkit.org/show_bug.cgi?id=38688

Attachment 55614: Patch for addressing review comments.
https://bugs.webkit.org/attachment.cgi?id=55614&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +static HTMLFormControlElement* nodeAsLabelableFormControl(Node* node)
> +{
> +    if (!node)
> +	   return 0;
> +
> +    if (node->isHTMLElement() &&
static_cast<HTMLElement*>(node)->isFormControlElement()) {
> +	   HTMLFormControlElement* formControlElement =
static_cast<HTMLFormControlElement*>(node);
> +	   if (formControlElement->isLabelable())
> +	       return formControlElement;
> +    }
> +
> +    return 0;
> +}

I think this would read even better if it used early returns exclusively.

> +	   // Search the children of the label element for a form element.

This searches the descendants of the label element, not just the children.

> +	       if (HTMLFormControlElement* formControlElement =
nodeAsLabelableFormControl(node))
> +		   return formControlElement;

As I said before, I see how this checks for "labelable" elements, but not
"form-associated". Maybe that's the same thing as checking
isFormControlElement?

> +class HTMLFormControlElement;    
> +
>  class HTMLLabelElement : public HTMLElement {
>  public:
>      HTMLLabelElement(const QualifiedName&, Document*);
> @@ -46,7 +48,7 @@ public:
>      // Overridden to either click() or focus() the corresponding control.
>      virtual void defaultEventHandler(Event*);
>  
> -    HTMLElement* correspondingControl();
> +    HTMLElement* control();

Did you mean to change the type here to HTMLFormControlElement? If not, then
why the forward declaration?

> +	   readonly attribute HTMLElement control;

Was the type here supposed to be HTMLFormControlElement?

I don't entirely understand why the manual test you made is a manual test.
Automated tests can both activate and focus, although perhaps for hover you
need a manual test.

I'm going to say review- mainly because of the unused forward declaration in
HTMLLabelElement.h that clearly seems to be in error.