[webkit-reviews] review denied: [Bug 56682] Opening link with unspecific hash in a new tab (except context menu) twice in a row results in hash for current window changing : [Attachment 90367] Proposed patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied ChangSeok Oh
<kevin.cs.oh at gmail.com>'s request for review:
Bug 56682: Opening link with unspecific hash in a new tab (except context menu)
twice in a row results in hash for current window changing
https://bugs.webkit.org/show_bug.cgi?id=56682

Attachment 90367: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=90367&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=90367&action=review

> Source/WebCore/loader/FrameLoader.cpp:1381
> +    if (event && event->isMouseEvent()) {
> +	   RefPtr<MouseEvent> mouseEvent =
static_pointer_cast<MouseEvent>(event);
> +#if PLATFORM(MAC)
> +	   if (mouseEvent->metaKey()) {
> +#else
> +	   if (mouseEvent->ctrlKey() || mouseEvent->button() == MiddleButton) {

> +#endif
> +	       policyChecker()->checkNewWindowPolicy(action,
FrameLoader::callContinueLoadAfterNewWindowPolicy,
> +		   request, formState.release(), frameName, this);
> +	       return;
> +	   }

This is the wrong place to do this work.  The FrameLoader shouldn't understand
anything about the meta or ctrl keys.  Even worse, it shouldn't be encoding
platform-specific behavior!