[webkit-reviews] review denied: [Bug 22382] Middle click fires onclick event : [Attachment 67124] patch, cleanup portion, v1

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Sep 9 17:46:05 PDT 2010


Darin Adler <darin at apple.com> has denied Peter Kasting <pkasting at google.com>'s
request for review:
Bug 22382: Middle click fires onclick event
https://bugs.webkit.org/show_bug.cgi?id=22382

Attachment 67124: patch, cleanup portion, v1
https://bugs.webkit.org/attachment.cgi?id=67124&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Thanks for doing this. I really appreciate you picking up the ball on this.

>  void DeleteButton::defaultEventHandler(Event* event)
>  {
> -    // FIXME: Is it really import to check the type of the event?
> -    // Seems OK to respond to any event named click even if it does not have
the correct type.
> -    if (event->isMouseEvent()) {
> -	   if (event->type() == eventNames().clickEvent) {
> -	      
document()->frame()->editor()->deleteButtonController()->deleteTarget();
> -	       event->setDefaultHandled();
> -	       // FIXME: Shouldn't we return here instead of falling through?
> -	   }
> +    if (event->type() == eventNames().clickEvent) {
> +	  
document()->frame()->editor()->deleteButtonController()->deleteTarget();
> +	   event->setDefaultHandled();
> +    } else {
> +	   HTMLImageElement::defaultEventHandler(event);
>      }
> -
> -    HTMLImageElement::defaultEventHandler(event);
>  }

In the WebKit project we use early return, not else, for cases like this.

A single line else does not get braces around it in the WebKit coding style.

> +HTMLAnchorElement::EventType HTMLAnchorElement::eventTypeFromEvent(Event*
event)
> +{
> +    if (!event->isMouseEvent())
> +	   return NonMouseEvent;
> +    return static_cast<MouseEvent*>(event)->shiftKey() ?
MouseEventWithShiftKey : MouseEventWithoutShiftKey;
> +}

This seems unnecessarily complicated. Why do we need to introduce this enum.
Can't we just pass in an event?

> +bool HTMLAnchorElement::isLiveLinkImpl(EventType eventType) const

I am really confused by the function name "isLiveLinkImpl". When I translate it
to English "is this a live link impl" it remains unclear.

What is this for? Can we give it a sensible name?

> +    case EditableLinkLiveWhenNotFocused:
> +	   return (eventType == MouseEventWithShiftKey) || ((eventType ==
MouseEventWithoutShiftKey) && (m_rootEditableElementForSelectionOnMouseDown !=
rootEditableElement()));

This has too many parentheses in it. In the WebKit project we normally don't
include parentheses when mixing "==" and "!=" with logical operators.

> +    return (event->type() == eventNames().keydownEvent) &&
event->isKeyboardEvent() &&
(static_cast<KeyboardEvent*>(event)->keyIdentifier() == "Enter");

Same here.

> +    return event->isMouseEvent() &&
(static_cast<MouseEvent*>(event)->button() == MiddleButton);

Here too.

> +    return (event->type() == eventNames().clickEvent) &&
(!event->isMouseEvent() || (static_cast<MouseEvent*>(event)->button() !=
RightButton));

And here.

> +void handleLinkClick(Event* event, Document* document, const String& url,
const String& target, bool hideReferrer)
> +{
> +    event->setDefaultHandled();
> +    Frame* frame = document->frame();
> +    if (frame)
> +	   frame->loader()->urlSelected(document->completeURL(url), target,
event, false, false, true, hideReferrer ? NoReferrer : SendReferrer);
>  }

In WebKit, we normally use early return rather than nesting things inside an
if.

> +    static EventType eventTypeFromEvent(Event* event);
> +    bool isLiveLinkImpl(EventType eventType) const;

We omit argument names when they are clear from the argument type as in these
cases.

review- mainly because of isLiveLinkImpl, but also because you made many style
changes that do not match the usual WebKit style


More information about the webkit-reviews mailing list