Bug 137110 – [GTK] Implement is_selected method on WebKitHitTestResult

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Marcos Chavarría Teijeiro (irc: chavaone)]

We need a way to check if the user is selecting something when we get an instance of WKHitTestResult. We could do this with WK1 [1].

I have check the sources and the function exists, we only have to expose some new API functions.

[Marcos Chavarría Teijeiro (irc: chavaone)]

Created attachment 238707 [details]
Patch

[Marcos Chavarría Teijeiro (irc: chavaone)]

Created attachment 238843 [details]
Patch

Change variable names to make them similar to WK1 API.

[Carlos Garcia Campos]

Comment on attachment 238843 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238843&action=review

Thanks for the patch, it looks good to me in general, but new API patches should include a unit test (see Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestUIClient.cpp and TestContextMenu.cpp). Also this patch changes cross-platform code, so it requires the approval of a WebKit2 owner.

> Source/WebKit2/UIProcess/API/gtk/WebKitHitTestResult.cpp:366
> + */

Since: 2.8

> Source/WebKit2/UIProcess/API/gtk/WebKitHitTestResult.h:51
> + * @WEBKIT_HIT_TEST_RESULT_CONTEXT_SELECTION: a selected element.

Since: 2.8.

> Source/WebKit2/UIProcess/API/gtk/WebKitHitTestResult.h:100
> +webkit_hit_test_result_context_is_selection  (WebKitHitTestResult *hit_test_result);

I think there's an extra space between function name and parentheses.

[Marcos Chavarría Teijeiro (irc: chavaone)]

Created attachment 239089 [details]
Patch

Add tests and fix some minor details.

[Carlos Garcia Campos]

Comment on attachment 239089 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=239089&action=review

Looks good to me, apart from the minor issues I mentioned.

> Source/WebKit2/UIProcess/API/gtk/WebKitHitTestResult.cpp:367
> + * Since 2.8.

Since: 2.8

> Source/WebKit2/UIProcess/API/gtk/WebKitHitTestResult.h:55
> + * Since 2.8.

This here means the enum was introduced in 2.8, but it's only the value. You should add the Since tag after the flag.

* @WEBKIT_HIT_TEST_RESULT_CONTEXT_SELECTION: a selected element. Since 2.8

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:408
> +    // Context menu for selection.
> +    test->m_expectedMenuType = ContextMenuDefaultTest::Selection;
> +    test->showContextMenuAtPositionAndWaitUntilFinished(2, 115);

I guess this should go first because otherwise any other click would remove the selection, right? I would add a comment explaining why this should always be the first check.

[Marcos Chavarría Teijeiro (irc: chavaone)]

Created attachment 239093 [details]
Patch

Explain test and fix since related issues.

[Carlos Garcia Campos]

Comment on attachment 239093 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=239093&action=review

> Source/WebKit2/UIProcess/API/gtk/WebKitHitTestResult.cpp:367
> + * Since 2.8

Since: 2.8

This requires the ':', the regexp used by gtk-doc to parse this is:

} elsif (m%^\s*since:%i) {

[Marcos Chavarría Teijeiro (irc: chavaone)]

Created attachment 239097 [details]
Patch

Add colon to Since tag.

[Tim Horton]

Comment on attachment 239097 [details]
Patch

r+ given cgarcia's LGTM+adjustments and a review of the WebKit2 bits.

[Carlos Alberto Lopez Perez]

Seems that commit-queue forgot to update the flags or close the bug.

This was committed on r175160 <http://trac.webkit.org/r175160>


Closing