[webkit-reviews] review denied: [Bug 33253] Want access to <label> element associated with <input> : [Attachment 45962] Patch making labelForElement a public static function in HTMLLabelElement

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied scroggo at google.com's request for
review:
Bug 33253: Want access to <label> element associated with <input>
https://bugs.webkit.org/show_bug.cgi?id=33253

Attachment 45962: Patch making labelForElement a public static function in
HTMLLabelElement
https://bugs.webkit.org/attachment.cgi?id=45962&action=review

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

> +    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