[webkit-reviews] review denied: [Bug 120059] Add support for maction at toggle : [Attachment 218321] Patch V5

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Frédéric Wang <fred.wang at free.fr>'s
request for review:
Bug 120059: Add support for maction at toggle
https://bugs.webkit.org/show_bug.cgi?id=120059

Attachment 218321: Patch V5
https://bugs.webkit.org/attachment.cgi?id=218321&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=218321&action=review


> Source/WebCore/mathml/MathMLSelectElement.cpp:40
> +class MActionEventListener : public EventListener {

I don’t think we need an event listener for this. This should just be done in
the default event handler for the MathMLSelectElement.

> Source/WebCore/mathml/MathMLSelectElement.cpp:42
> +    static PassRefPtr<MActionEventListener> create(MathMLSelectElement*
maction) { return adoptRef(new MActionEventListener(maction)); }

Should return a PassRef rather than PassRefPtr. Should take
MathMLSelectElement& instead of MathMLSelectElement*.

> Source/WebCore/mathml/MathMLSelectElement.cpp:43
> +    static const MActionEventListener* cast(const EventListener* listener)

Do we really need this function? The one call site below would be clearer
without it.

> Source/WebCore/mathml/MathMLSelectElement.cpp:47
> +	       : 0;

nullptr

> Source/WebCore/mathml/MathMLSelectElement.cpp:49
> +    virtual bool operator==(const EventListener& other);

Should be marked OVERRIDE.

> Source/WebCore/mathml/MathMLSelectElement.cpp:52
> +    MActionEventListener(MathMLSelectElement* maction)

Should mark this explicit. Should take a reference instead of a pointer.

> Source/WebCore/mathml/MathMLSelectElement.cpp:58
> +    virtual void handleEvent(ScriptExecutionContext*, Event*);

Needs OVERRIDE.

> Source/WebCore/mathml/MathMLSelectElement.cpp:59
> +    MathMLSelectElement* m_maction;

Should be a reference instead of pointer.

> Source/WebCore/mathml/MathMLSelectElement.cpp:68
> +	   RefPtr<EventListener> listener = MActionEventListener::create(this);

> +	   addEventListener("click", listener.release(), false);

Would read better as a one-liner. Also should use the clickEvent event name
rather than “click”. Also not clear this should be done with addEventListener.
Why can’t this just be in the default event handler for MathMLSelectElement?

> Source/WebCore/mathml/MathMLSelectElement.cpp:75
> +    if (event->defaultPrevented())
> +	   return;

Surprised this is needed and not handled by the caller.

> Source/WebCore/mathml/MathMLSelectElement.cpp:190
> +    // We get the current value of the selection attribute.
> +    int selection = fastGetAttribute(MathMLNames::selectionAttr).toInt();
> +    if (selection < 1)
> +	   selection = 1;
> +
> +    // We find the successor of the current selection.
> +    selection++;
> +    Element* child = firstElementChild();
> +    if (child) {
> +	   for (int i = 1; i < selection; i++) {
> +	       child = child->nextElementSibling();
> +	       if (!child)
> +		   break;
> +	   }
> +    }
> +
> +    // If we reach the end of the child list, we go back to the first child.

> +    if (!child)
> +	   selection = 1;
> +
> +    // We update the attribute value of the selection attribute.
> +    // This will also call MathMLSelectElement::attributeChanged to update
the selected child.
> +    setAttribute(MathMLNames::selectionAttr, String::number(selection));

Can write this much more concisely:

    int selection = std::max(1,
fastGetAttribute(MathMLNames::selectionAttr).toInt()) + 1;
    if (selection > childElementCount())
	selection = 1;
    setAttribute(MathMLNames::selectionAttr, String::number(selection));

The extra cost of counting all the children seems quite affordable; does not
make the worst case any worse.