Bug 68369

Summary: [Qt][WK2] Add support for hover API in Qt WebKit2
Product: WebKit Reporter: Igor Trindade Oliveira <igor.oliveira>
Component: WebKit2Assignee: Caio Marcelo de Oliveira Filho <cmarcelo>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, cmarcelo, jesus, kling, menard, sam
Priority: P3 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 68426    
Bug Blocks:    
Attachments:
Description Flags
Patch.
none
Patch
none
Patch kling: review+

Description Igor Trindade Oliveira 2011-09-19 10:13:11 PDT
Add support for hover API in Qt WebKit2
Comment 1 Igor Trindade Oliveira 2011-09-20 08:00:11 PDT
Created attachment 108002 [details]
Patch.

Proposed implementation.
Comment 2 Andreas Kling 2011-09-21 04:39:56 PDT
CC'ing some WK2 gentlemen since you're adding C API.
Comment 3 Alexis Menard (darktears) 2011-09-21 04:54:49 PDT
Comment on attachment 108002 [details]
Patch.

Don't you miss some build systems here? GTK and the cmake stuff?
Comment 4 Igor Trindade Oliveira 2011-09-21 06:13:30 PDT
CMake code is in the beginning of the patch and GTK is handled by GNUmakefile.am.

(In reply to comment #3)
> (From update of attachment 108002 [details])
> Don't you miss some build systems here? GTK and the cmake stuff?
Comment 5 Igor Trindade Oliveira 2011-09-21 06:14:28 PDT
Created attachment 108140 [details]
Patch

Updated patch.
Comment 6 Anders Carlsson 2011-09-21 10:36:55 PDT
Comment on attachment 108140 [details]
Patch

The API added doesn't follow the naming conventions we use for returning copies. Also, https://bugs.webkit.org/show_bug.cgi?id=68426 also adds the same API.
Comment 7 Anders Carlsson 2011-09-21 12:16:50 PDT
Also, this adds LGPL API headers. WebKit2 API headers should be BSD licensed.
Comment 8 Caio Marcelo de Oliveira Filho 2011-09-26 17:51:56 PDT
Created attachment 108767 [details]
Patch
Comment 9 Caio Marcelo de Oliveira Filho 2011-09-26 17:55:40 PDT
Igor went for vacations, so I'm picking up this. WKHitTestResult landed already for another bug.

Besides removing the WKHitTestResult, this patch also
- change MiniBrowser to use the new feature;
- add tests for the signal;
- remove the third parameter textContent, since I'm not convinced it is necessary.
Comment 10 Alexis Menard (darktears) 2011-09-26 18:40:41 PDT
Comment on attachment 108767 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:91
> +    void linkHovered(const QUrl& url, const QString &title);

Style issue. const QString&.
Comment 11 Andreas Kling 2011-09-27 06:27:27 PDT
Comment on attachment 108767 [details]
Patch

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

r=me

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:436
> +    if (linkURL != lastHoveredURL || linkTitle != lastHoveredTitle) {

I would write this as:
if (linkURL == lastHoveredURL && linkTitle == lastHoveredTitle)
    return;

> Source/WebKit2/UIProcess/qt/ClientImpl.cpp:167
> +void qt_wk_mouseDidMoveOverElement(WKPageRef page, WKHitTestResultRef hitTestResult, WKEventModifiers modifiers, WKTypeRef userData, const void *clientInfo)

Style, const void* clientInfo.

> Source/WebKit2/UIProcess/qt/ClientImpl.h:44
> +void qt_wk_mouseDidMoveOverElement(WKPageRef, WKHitTestResultRef, WKEventModifiers, WKTypeRef, const void *clientInfo);

Ditto.
Comment 12 Caio Marcelo de Oliveira Filho 2011-09-27 07:09:22 PDT
Committed r96105: <http://trac.webkit.org/changeset/96105>