[Webkit-unassigned] [Bug 38688] Support control attribute of HTMLLabelElement

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=38688


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #55369|review?                     |review-
               Flag|                            |




--- Comment #11 from Darin Adler <darin at apple.com>  2010-05-09 19:44:06 PST ---
(From update of attachment 55369)
Looks good. But the multiple things you are doing need to be tested.

You changed the label element code so that it won't label other types of form control. So you need a test that shows that behavior change.

Your comment says that you "cleaned up" correspondingControl, but that's not what you did. You fixed a bug in it!

I'd also like to see the code refactored so the two branches of the function shared the code to check isFormControlElement rather than repeating the code twice. You could use the traverseElement code to make it even easier to share code.

> +bool HTMLFormControlElement::isControlLabelable() const

How about just "isLabelable" for this?
> +    // FIXME: Add meterTag and outputTag to the list once we support them.
> +    return hasTagName(buttonTag) || hasTagName(inputTag) || hasTagName(keygenTag)
> +#if ENABLE(PROGRESS_TAG)
> +    || hasTagName(progressTag)
> +#endif
> +    || hasTagName(selectTag) || hasTagName(textareaTag);

You should indent the continuation lines 4 spaces in.

> -HTMLElement* HTMLLabelElement::correspondingControl()
> +HTMLFormControlElement* HTMLLabelElement::correspondingControl() const

Does this really need to be a const member function? The need to const_cast in here is ugly.

> +        // the control must be "labelable form-associated element".

I see the "labelable" part here, but I don't see the "form-associated" part.

> +PassRefPtr<HTMLElement> HTMLLabelElement::control() const
> +{
> +    return correspondingControl();
> +}

There is no reason to return a PassRefPtr here. It should just be a raw pointer.

> -    HTMLElement* correspondingControl();
> +    HTMLFormControlElement* correspondingControl() const;

> +    PassRefPtr<HTMLElement> control() const;

I suggest renaming our correspondingControl function to control rather than adding a second function. And I see no reason to make this a const member function.

>      interface HTMLLabelElement : HTMLElement {
>          readonly attribute HTMLFormElement form;
> -        attribute [ConvertNullToNullString] DOMString accessKey;
> -        attribute [ConvertNullToNullString] DOMString htmlFor;
> +                 attribute [ConvertNullToNullString] DOMString accessKey;
> +                 attribute [ConvertNullToNullString] DOMString htmlFor;
> +        readonly attribute HTMLElement control;
>      };

I don't think we need to mimic the indenting of the HTML5 specification. I'd prefer to just have things indented one tab stop and not try to line things up.

Fix a few things and put it up for review again. Looks good.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.