[webkit-reviews] review granted: [Bug 23808] Add SelectElement abstraction : [Attachment 30619] Updated patch v2

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 23808: Add SelectElement abstraction
https://bugs.webkit.org/show_bug.cgi?id=23808

Attachment 30619: Updated patch v2
https://bugs.webkit.org/attachment.cgi?id=30619&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Didn't we discover a way to do this without introducing multiple inheritance? I
thought I saw patches where we were undoing some virtualization we did before
based on feedback from Dave Hyatt.

> -	   AccessibilityObject* listOption =
listBoxOptionAccessibilityObject(listItems[i]);
> +	   AccessibilityObject* listOption =
listBoxOptionAccessibilityObject(static_cast<HTMLElement*>(listItems[i]));

This cast needs a comment explaining why it is safe. Casts like these are
dangerous and we've had many bugs in the past where we did an illegal downcast.
I believe the answer is that this is guaranteed to be an HTML element unless
you are compiling with WML enabled, and that the code in this file is not
compatible with WML.

> -	       return listBoxOptionAccessibilityObject(listItems[i]);
> +	       return
listBoxOptionAccessibilityObject(static_cast<HTMLElement*>(listItems[i]));

Same thought here.

>      if (m_renderer->isMenuList())
> -	   return static_cast<RenderMenuList*>(m_renderer)->selectElement();
> -    
> +	   return static_cast<Element*>(m_renderer->node());

Same comment again. Why is this cast to Element safe? We need to at least have
a comment saying why, and ideally come up with idioms that don't require
unchecked type casts, unless the check is just before the cast. You'll note
that in the old code, the cast to RenderMenuList was checked by the code in the
if statement.

> +    virtual void dispatchFormControlChangeEvent() { }

Do we really need this in the Element class? Our goal is to go in the opposite
direction, and cut down the size of our massive base classes.

This is really just a helper function to call dispatchEvent with the
changeEvent event name. I don't think it's really form-control-specific.
Perhaps it would be cleaner move this into a separate helper function and have
it not be a member function at all. Having this be a virtual function isn't
helpful unless we actually plan to call this in cases where we do not want a
change event dispatched. We could also have this just be in the EventTarget
class.

> +	   if (element->hasTagName(HTMLNames::selectTag))
> +	       return static_cast<HTMLSelectElement*>(element);
> +	   else if (element->hasTagName(HTMLNames::keygenTag))
> +	       return static_cast<HTMLKeygenElement*>(element);

We normally do not do else after return.

> +    virtual bool multiple() const = 0;

Not a great name for a function -- you probably didn't create it. Typically
getters should have a noun or adjective that applies to the object. But "a
select element's multiple" makes no sense. 

> +protected:
> +    SelectElement() { }

This explicit declaration and definition of the constructor is unneeded. If you
don't declare or define it, we'll get the same thing automatically. So the only
effect this has is to make the constructor protected. Since the class has pure
virtual functions, objects of this class already can't be constructed, so it's
best to just leave this out.

>	   if (current->hasTagName(optionTag)) {
> -	       m_listItems.append(static_cast<HTMLElement*>(current));
> +	       m_listItems.append(static_cast<Element*>(current));

I think this should be a cast to HTMLOptionElement*, not to Element*, since
that matches the test that was done. I know that m_listItems can hold any
Element*, but that need not be the deciding factor.

> -	   // Save the selection so it can be compared to the new selection
when we call onChange during dispatchBlurEvent.
> +	   // Save the selection so it can be compared to the new selection
when we call dispatchFormControlChangeEvent during dispatchBlurEvent.

I think this comment is probably referring to the change event, not the
onChange function. If so, we should call it "send a change event" or "call
onchange", and not use the function name.

> -    // We only need to fire onChange here for menu lists, because we fire
onChange for list boxes whenever the selection change is actually made.
> +    // We only need to fire dispatchFormControlChangeEvent here for menu
lists, because we fire onChange for list boxes whenever the selection change is
actually made.

Same thought here, but even more so. When you say "fire onchange" I think
you're referring to the event, not the function.

In general, I don't think the renames inside the comments are all improvements,
especially since the new function name is so long.

> +void WMLFormControlElement::dispatchFormControlChangeEvent()
> +{
> +    // no-op
> +}

Why have this since it's the same as the base class?

I'm going to say r=me. This seems a little bit messy, but no significant
problems.