[webkit-reviews] review denied: [Bug 137597] Support activation behavior of link element : [Attachment 239605] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Dhi Aurrahman
<diorahman at rockybars.com>'s request for review:
Bug 137597: Support activation behavior of link element
https://bugs.webkit.org/show_bug.cgi?id=137597

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=239605&action=review


The patch is great. The changelogs are great. The tests are really well done.

r- because of the duplicated isLinkClick() in HTMLLinkElement and
HTMLAnchorElement. Otherwise this is nearly perfect.

> Source/WebCore/html/HTMLLinkElement.cpp:413
> +bool HTMLLinkElement::isLinkClick(Event* event)
> +{
> +    return event->type() == eventNames().clickEvent &&
(!is<MouseEvent>(*event) || downcast<MouseEvent>(*event).button() !=
RightButton);
> +}

Since this code is duplicated in HTMLAnchorElement, we should put it in a
common place.

I suggest adding a static function "isRightClick" to the class MouseEvent. Then
modify HTMLAnchorElement and HTMLLinkElement to use that function.

> Source/WebCore/html/HTMLLinkElement.cpp:420
> +    if (url.isNull())
> +	   return;

I believe this is an interesting case, there should be a test for it if
possible.

Maybe:
1) Create a click event with document.createEvent.
2) Send it to the <link> element with dispatchEvent()
3) Verify that the event has its default handled.
4) Set a time to 100ms. If the document has not navigated anywhere after the
100ms, finish the test with success.

This might even work when opened on Firefox...