Bug 40429

Created attachment 58451 [details] Patch

Comment on attachment 58451 [details] Patch > + Vector<RefPtr<Node> > buttons; > + form()->getNamedElements(name(), buttons); Why doesn't this function put its output in a Vector<RefPtr<Element> > or a Vector<RefPtr<HTMLFormControlElement> >? > + RefPtr<NodeList> nodeList = document()->getElementsByName(name()); > + if (nodeList) { Why is the null check needed? Can getElementsByName return null? Why use a NodeList? That is a data structure for the public DOM, but for internal use I would recommend just writing a loop. The getElementsByName function has no special algorithm and introduces unneeded overhead. for (Node* node = document(); node; node = node->traverseNextNode()) The above will do the same thing more efficiently than the NodeList code. By the way, it worries me that we have to iterate every single item in the whole document every time updateCheckedRadioButtons is called on a radio button outside a form. Can't this get pathologically slow if there are a ton of radio buttons?

Created attachment 58462 [details] Patch 2

Thank you for reviewing. I updated the patch. (In reply to comment #2) > (From update of attachment 58451 [details]) > > + Vector<RefPtr<Node> > buttons; > > + form()->getNamedElements(name(), buttons); > > Why doesn't this function put its output in a Vector<RefPtr<Element> > or a Vector<RefPtr<HTMLFormControlElement> >? It's not so easy to chenge getNamedElements() output type because it is created from HTMLCollection. I found HTMLFormElement::associatedElements() and the updated patch uses it. > > > + RefPtr<NodeList> nodeList = document()->getElementsByName(name()); > > + if (nodeList) { > Why use a NodeList? That is a data structure for the public DOM, but for internal use I would recommend just writing a loop. The getElementsByName function has no special algorithm and introduces unneeded overhead. > > for (Node* node = document(); node; node = node->traverseNextNode()) > > The above will do the same thing more efficiently than the NodeList code. Ok, I change it to traverseNextNode(). > By the way, it worries me that we have to iterate every single item in the whole document every time updateCheckedRadioButtons is called on a radio button outside a form. Can't this get pathologically slow if there are a ton of radio buttons? Of course it can be slow. I added a FIXME comment.

Comment on attachment 58462 [details] Patch 2 > + if (element->formControlType() != formControlType()) > + continue; > + if (element->formControlName() != formControlName()) > + continue; I think you can use type() and name() on the right side of these if statements. Since we're doing this work for every element in the entire document I think you need to be careful to use the check that is most likely to fail first. That's probably the formControlName() check.

(In reply to comment #5) > (From update of attachment 58462 [details]) > > + if (element->formControlType() != formControlType()) > > + continue; > > + if (element->formControlName() != formControlName()) > > + continue; > > I think you can use type() and name() on the right side of these if statements. Since we're doing this work for every element in the entire document I think you need to be careful to use the check that is most likely to fail first. That's probably the formControlName() check. Ok, I changed the order and changed to type()/name(). Committed as http://trac.webkit.org/changeset/61060

*** Bug 40591 has been marked as a duplicate of this bug. ***