Bug 36023

Summary: [Chromium] Cmd-clicking submit buttons should submit in new tab
Product: WebKit Reporter: Nico Weber <thakis>
Component: WebKit Misc.Assignee: Nico Weber <thakis>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: OS X 10.5   
Attachments:
Description Flags
Patch.
levin: review-
New Patch.
none
Rebase. none

Description Nico Weber 2010-03-11 11:26:35 PST
Created attachment 50519 [details]
Patch.

Patch for the mouse part of http://crbug.com/32525 .
Comment 1 David Levin 2010-03-11 14:42:29 PST
Comment on attachment 50519 [details]
Patch.

> Index: WebKit/chromium/ChangeLog
> ===================================================================
> --- WebKit/chromium/ChangeLog	(revision 55848)
> +++ WebKit/chromium/ChangeLog	(working copy)
> @@ -1,3 +1,15 @@
> +2010-03-11  Nicolas Weber  <thakis@chromium.org>
> +
> +        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);
Comment 2 Nico Weber 2010-03-11 15:46:30 PST
Created attachment 50553 [details]
New Patch.

Thanks for the comments.
Comment 3 Nico Weber 2010-03-11 16:36:57 PST
Created attachment 50559 [details]
Rebase.
Comment 4 David Levin 2010-03-11 17:49:08 PST
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 :)
Comment 5 WebKit Commit Bot 2010-03-12 09:03:56 PST
Comment on attachment 50559 [details]
Rebase.

Clearing flags on attachment: 50559

Committed r55908: <http://trac.webkit.org/changeset/55908>
Comment 6 WebKit Commit Bot 2010-03-12 09:04:01 PST
All reviewed patches have been landed.  Closing bug.