[Webkit-unassigned] [Bug 33253] Want access to <label> element associated with <input>

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jan 6 09:58:38 PST 2010


Darin Adler <darin at apple.com> changed:

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

--- Comment #3 from Darin Adler <darin at apple.com>  2010-01-06 09:58:38 PST ---
(From update of attachment 45962)
I'm not entirely sure this is a good change.

> +HTMLLabelElement* HTMLLabelElement::labelForElement(Element* element)
> +{
> +    RefPtr<NodeList> list = element->document()->getElementsByTagName("label");

I know you're just moving the above code, but I'll note that using the NodeList
is a quite-inefficient way to iterate all the label elements -- it's the public
DOM API for JavaScript use, but inside the engine it's not a great idea.

It would be straightforward to just iterate all the nodes in the document
looking for label elements instead. It's also really strange that this function
uses a node list created by getElementsByTagName but then still checks the tag
name of each element in the list. That makes no sense.

You just moved the code, but it's not great code.

The only real value in having this function in the header would be knowing that
the algorithm used is exactly the same one used by HTMLLabelElement itself. And
yet I see no evidence that this is true. The function was used only in the
accessibility code. It's inefficient to iterate an entire document, so using
this function would typically not be encouraged.

And it's straightforward to implement this in terms of already existing public

> +    unsigned len = list->length();

As long as you're moving the code, it would be good to use "length" instead of
"len". We prefer words to abbreviations.

> Index: WebCore/html/HTMLLabelElement.h
> ===================================================================
> --- WebCore/html/HTMLLabelElement.h	(revision 52806)
> +++ WebCore/html/HTMLLabelElement.h	(working copy)
> @@ -56,6 +56,7 @@ public:
>      void focus(bool restorePreviousSelection = true);
> +    static HTMLLabelElement* labelForElement(Element* element);
>   private:
>      String m_formElementID;

There should be a blank line after it so this function isn't paragraphed with
the private members below.

The argument name "element" should be omitted.

I assume you want to make this function public for some kind of Chromium use.
Could you be more clear about the specifics? Typically, if there's a function
that is not used anywhere in the WebKit source tree people will feel free to
refactor so it's private again. So I think it would be best to add the use of
the function at the same time you make it public.

review- for now

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

More information about the webkit-unassigned mailing list