[webkit-reviews] review denied: [Bug 49376] [GTK] Show default context menu for the currently focused element when activated with keyboard : [Attachment 73599] Patch to show context menu for focused items when using the keyboard

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 49376: [GTK] Show default context menu for the currently focused element
when activated with keyboard
https://bugs.webkit.org/show_bug.cgi?id=49376

Attachment 73599: Patch to show context menu for focused items when using the
keyboard
https://bugs.webkit.org/attachment.cgi?id=73599&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=73599&action=review

> WebKit/gtk/webkit/webkitwebview.cpp:347
> +	   || (frame->selection()->selection().isCaret() &&
!frame->selection()->selection().isContentEditable())) {
> +	   // If there's a focused elment, use its location.
> +	   bool locationSet = false;
> +	   Document* doc = frame->document();
> +	   if (doc) {

I think maybe we should break out this block out into a helper
function...perhaps everyting from the start of this block to the FIXME.
getActiveNodeLocation or something similar? This will simplify things by
allowing early returns (you can do away with the locationSet variable). You can
also simplify this further by doing something like:

if (Document* doc = frame->document()) {
    if (Node* focusedNode = doc->focusedNode()) {
	...
    }
}