Bug 36023 – [Chromium] Cmd-clicking submit buttons should submit in new tab

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Nico Weber]

Created attachment 50519 [details]
Patch.

Patch for the mouse part of http://crbug.com/32525 .

[David Levin]

Comment on attachment 50519 [details]
Patch.

> Index: WebKit/chromium/ChangeLog
> ===================================================================
> --- WebKit/chromium/ChangeLog	(revision 55848)
> +++ WebKit/chromium/ChangeLog	(working copy)
> @@ -1,3 +1,15 @@
> [phone]  Nicolas Weber  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).

Add bug title and bug link here. Like this:

  [Chromium] Cmd-clicking submit buttons should submit in new tab 
  https://bugs.webkit.org/show_bug.cgi?id=36023

Then a blank line followed by your description.
> +
> +        Take modifiers into account when clicking form buttons. E.g.
> +        cmd-clicking a submit button will submit in a new background tab,
> +        cmd-shift-clicking in a new foreground tab, shift-clicking in a new
> +        window. (On windows/linux, it's ctrl instead of cmd.)
> +
> +        * src/FrameLoaderClientImpl.cpp:
> +        (WebKit::FrameLoaderClientImpl::actionSpecifiesNavigationPolicy):
> +

> Index: WebKit/chromium/src/FrameLoaderClientImpl.cpp
> @@ -1477,13 +1477,25 @@ bool FrameLoaderClientImpl::actionSpecif
>      const NavigationAction& action,
>      WebNavigationPolicy* policy)
>  {
> -    if ((action.type() != NavigationTypeLinkClicked) || !action.event()->isMouseEvent())
> +    if (action.type() == NavigationTypeLinkClicked
> +        && action.event()->isMouseEvent()) {
> +        const MouseEvent* event =
> +            static_cast<const MouseEvent*>(action.event());
> +        return WebViewImpl::navigationPolicyFromMouseEvent(
> +            event->button(), event->ctrlKey(), event->shiftKey(),
> +            event->altKey(), event->metaKey(), policy);
> +    } else if (action.type() == NavigationTypeFormSubmitted
> +        && action.event()
> +        && action.event()->underlyingEvent()
> +        && action.event()->underlyingEvent()->isMouseEvent()) {
> +        const MouseEvent* event =
> +            static_cast<const MouseEvent*>(action.event()->underlyingEvent());
> +        return WebViewImpl::navigationPolicyFromMouseEvent(
> +            event->button(), event->ctrlKey(), event->shiftKey(),
> +            event->altKey(), event->metaKey(), policy);
> +    } else {
>          return false;
> +    }

There are several style issues here:
1. No need to stick to 80 columns strictly. (The code looks contorted in places to do this.)
2. When a clause ends with a "return", don't use else.
3. No braces for single line statements (the "return false;").


All that being said, it would be nice to have only one call to navigationPolicyFromMouseEvent since it looks heavy.


I'd suggest this instead:


    const MouseEvent* event = 0;
    if (action.type() == NavigationTypeLinkClicked && action.event()->isMouseEvent())
        event = static_cast<const MouseEvent*>(action.event());
    else if (action.type() == NavigationTypeFormSubmitted
             && action.event()
             && action.event()->underlyingEvent()
             && action.event()->underlyingEvent()->isMouseEvent())
        event = static_cast<const MouseEvent*>(action.event()->underlyingEvent());

    if (!event)
        return false;

    return WebViewImpl::navigationPolicyFromMouseEvent(
        event->button(), event->ctrlKey(), event->shiftKey(),
        event->altKey(), event->metaKey(), policy);

[Nico Weber]

Created attachment 50553 [details]
New Patch.

Thanks for the comments.

[Nico Weber]

Created attachment 50559 [details]
Rebase.

[David Levin]

Comment on attachment 50559 [details]
Rebase.

> Index: WebKit/chromium/src/FrameLoaderClientImpl.cpp
> +    if (action.type() == NavigationTypeLinkClicked
> +        && action.event()->isMouseEvent())

You really like wrapping this line :)

[WebKit Commit Bot]

Comment on attachment 50559 [details]
Rebase.

Clearing flags on attachment: 50559

Committed r55908: <http://trac.webkit.org/changeset/55908>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.