[webkit-reviews] review granted: [Bug 120059] Add support for maction at toggle : [Attachment 218844] Patch V6

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted 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 218844: Patch V6
https://bugs.webkit.org/attachment.cgi?id=218844&action=review

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


r=me; OK to land as is, but please fix as many of the comments as possible
before landing. Other improvements can be done later.

> Source/WebCore/mathml/MathMLSelectElement.cpp:122
> +    if (event->type() == eventNames().clickEvent && event->isMouseEvent()) {


There’s no reason to check isMouseEvent here. Please remove that check.

>> Source/WebCore/mathml/MathMLSelectElement.cpp:123
>> +	    if (getAttribute(MathMLNames::actiontypeAttr) == "toggle") {
> 
> can this be fastGetAttribute()

Yes, this should be fastGetAttribute.

>> Source/WebCore/mathml/MathMLSelectElement.cpp:124
>> +		toggle();
> 
> I think you also want to set
> event->setDefaultHandled();
> here

Calling setDefaultHandled should *not* break bubbling. I am quite surprised
that it does. We should look into it more.

> Source/WebCore/mathml/MathMLSelectElement.cpp:150
> +    int selection = 1;
> +    if (m_selectedChild) {
> +	   Element* child = firstElementChild();
> +	   while (child && child != m_selectedChild->nextElementSibling()) {
> +	       child = child->nextElementSibling();
> +	       selection++;
> +	   }
> +	   // If we reach the end of the child list, we go back to the first
child.
> +	   if (!child)
> +	       selection = 1;
> +    }

This way of writing the code makes a simple operation look too confusing. We
need to write this in terms of helper functions so we don’t need to write out
the while loop. I suggest adding a childElementIndex helper function and using
that.

> Source/WebCore/mathml/MathMLSelectElement.cpp:154
> +    setAttribute(MathMLNames::selectionAttr, String::number(selection));

Should be AtomicString::number rather than String::number. Same for any other
function using String::number but then passing it to setAttribute.

> Source/WebCore/mathml/MathMLSelectElement.h:30
> +#include "Event.h"

Not correct to include a header just so we can compile an Event*. And also
unneeded. Base class header was already able to compile an Event* so no include
is needed.

>> Source/WebCore/mathml/MathMLSelectElement.h:38
>> +	void toggle();
> 
> doesn't look like toggle() needs to be public

I agree. It should be private.