[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