[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