Bug 137110 - [GTK] Implement is_selected method on WebKitHitTestResult
Summary: [GTK] Implement is_selected method on WebKitHitTestResult
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Gtk (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-09-25 01:45 PDT by Marcos Chavarría Teijeiro (irc: chavaone)
Modified: 2014-10-28 04:18 PDT (History)
5 users (show)

See Also:


Attachments
Patch (7.74 KB, patch)
2014-09-26 03:50 PDT, Marcos Chavarría Teijeiro (irc: chavaone)
no flags Details | Formatted Diff | Diff
Patch (9.24 KB, patch)
2014-09-29 00:39 PDT, Marcos Chavarría Teijeiro (irc: chavaone)
no flags Details | Formatted Diff | Diff
Patch (24.93 KB, patch)
2014-10-02 01:14 PDT, Marcos Chavarría Teijeiro (irc: chavaone)
no flags Details | Formatted Diff | Diff
Patch (24.99 KB, patch)
2014-10-02 02:01 PDT, Marcos Chavarría Teijeiro (irc: chavaone)
no flags Details | Formatted Diff | Diff
Patch (24.99 KB, patch)
2014-10-02 02:38 PDT, Marcos Chavarría Teijeiro (irc: chavaone)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Marcos Chavarría Teijeiro (irc: chavaone) 2014-09-25 01:45:13 PDT
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.
Comment 1 Marcos Chavarría Teijeiro (irc: chavaone) 2014-09-26 03:50:18 PDT
Created attachment 238707 [details]
Patch
Comment 2 Marcos Chavarría Teijeiro (irc: chavaone) 2014-09-29 00:39:16 PDT
Created attachment 238843 [details]
Patch

Change variable names to make them similar to WK1 API.
Comment 3 Carlos Garcia Campos 2014-09-29 03:44:29 PDT
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.
Comment 4 Marcos Chavarría Teijeiro (irc: chavaone) 2014-10-02 01:14:46 PDT
Created attachment 239089 [details]
Patch

Add tests and fix some minor details.
Comment 5 Carlos Garcia Campos 2014-10-02 01:44:57 PDT
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.
Comment 6 Marcos Chavarría Teijeiro (irc: chavaone) 2014-10-02 02:01:29 PDT
Created attachment 239093 [details]
Patch

Explain test and fix since related issues.
Comment 7 Carlos Garcia Campos 2014-10-02 02:24:14 PDT
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) {
Comment 8 Marcos Chavarría Teijeiro (irc: chavaone) 2014-10-02 02:38:48 PDT
Created attachment 239097 [details]
Patch

Add colon to Since tag.
Comment 9 Tim Horton 2014-10-23 11:15:53 PDT
Comment on attachment 239097 [details]
Patch

r+ given cgarcia's LGTM+adjustments and a review of the WebKit2 bits.
Comment 10 Carlos Alberto Lopez Perez 2014-10-28 04:18:03 PDT
Seems that commit-queue forgot to update the flags or close the bug.

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


Closing