[webkit-reviews] review denied: [Bug 36023] [Chromium] Cmd-clicking submit buttons should submit in new tab : [Attachment 50519] Patch.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Mar 11 14:42:28 PST 2010


David Levin <levin at chromium.org> has denied Nico Weber <thakis at chromium.org>'s
request for review:
Bug 36023: [Chromium] Cmd-clicking submit buttons should submit in new tab
https://bugs.webkit.org/show_bug.cgi?id=36023

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

------- Additional Comments from David Levin <levin at chromium.org>
> 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);


More information about the webkit-reviews mailing list