[webkit-reviews] review denied: [Bug 67192] API to set initial focus in gtk webkit : [Attachment 106852] Modified patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Antaryami Pandia
<xqb748 at motorola.com>'s request for review:
Bug 67192: API to set initial focus in gtk webkit
https://bugs.webkit.org/show_bug.cgi?id=67192

Attachment 106852: Modified patch
https://bugs.webkit.org/attachment.cgi?id=106852&action=review

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


> Source/WebKit/gtk/tests/testwebview.c:368
> +    char* uri = g_strconcat(base_uri, "bigdiv.html", NULL);

You didn't include this file...you should probably just load HTML as a string
via webkit_web_view_load_string.

> Source/WebKit/gtk/tests/testwebview.c:388
> +    g_signal_connect(window, "map-event",
> +			G_CALLBACK(map_event_cb), loop);

No need to break this line. If I'm not mistaken if you take my suggestion
below, you shouldn't need to wait for the map event.

> Source/WebKit/gtk/tests/testwebview.c:412
> +    char script[] = "if (document.activeElement.id == \"link\")   \
> +			   window.scrollBy(0, 100);";

Hrm...this approach seems a little fragile. Why not just use
webkit_dom_html_document_get_active_element from the DOM bindings to inspect
the active element?

> Source/WebKit/gtk/tests/testwebview.c:414
> +    /* Execute the script to verify the focus */

Missing a period here.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:4037
> + * Set the initial focus element for spatial(keyboard) navigation.So with
spatial
> + * navigation when a page is loaded, user should be able see the initial
position
> + * of focus without pressing tab/arrow key.
> + *
> + * If @forward is %TRUE, then focus is set in the forward direction i.e.
focuses the first
> + * focusable element.If @forward is %FALSE, then focus is set in backward
direction i.e
> + * focuses the last focusable element. 

Nice documentation. You are missing spaces after all of the periods though. I
recommned replacing both "i.e." with something like this:

If @forward is %TRUE, then focus is set in the forward direction, which focuses
the first focusable element.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:4044
> +

Extra newline.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:4049
> +    Frame* frame = page->focusController()->focusedOrMainFrame();
> +    frame->document()->setFocusedNode(0);

No need to cache the frame here.