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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=36023


David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #50519|review?                     |review-
               Flag|                            |




--- Comment #1 from David Levin <levin at chromium.org>  2010-03-11 14:42:29 PST ---
(From update of attachment 50519)
> Index: WebKit/chromium/ChangeLog
> ===================================================================
> --- WebKit/chromium/ChangeLog	(revision 55848)
> +++ WebKit/chromium/ChangeLog	(working copy)
> @@ -1,3 +1,15 @@
> +2010-03-11  Nicolas Weber  <thakis at 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);

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.