[webkit-reviews] review denied: [Bug 96481] [Qt][WK2] Add API to find text from page : [Attachment 164346] [Qt][WK2] Add experimental API to find text from page

[bugzilla-daemon at webkit.org]

Jocelyn Turcotte <jocelyn.turcotte at digia.com> has denied Heikki Paajanen
<heikki.paajanen at palm.com>'s request for review:
Bug 96481: [Qt][WK2] Add API to find text from page
https://bugs.webkit.org/show_bug.cgi?id=96481

Attachment 164346: [Qt][WK2] Add experimental API to find text from page
https://bugs.webkit.org/attachment.cgi?id=164346&action=review

------- Additional Comments from Jocelyn Turcotte <jocelyn.turcotte at digia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=164346&action=review


It's a pretty good patch overall at this point, here are some minor issues.

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:392
> +    void textFound(unsigned matchCount);

I don't think we use unsigned so much in APIs, I'd rather have it exposed as a
signed int for consistency.

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:85
> +    virtual void didFindString(unsigned matchCount);

No need to make it virtual if it won't be overridden by
QQuickWebView(Legacy|Flickable)Private.

> Tools/MiniBrowser/qt/qml/BrowserWindow.qml:73
> +	   function toggle() {

Could you also trigger it with the ctrl+f shortcut in
MiniBrowserApplication::notify?
Having the TextInput get/lose focus when toggle() is called would be awesome
too.

> Tools/MiniBrowser/qt/qml/BrowserWindow.qml:94
> +	   MouseArea {

Please add a comment about its purpose.

> Tools/MiniBrowser/qt/qml/BrowserWindow.qml:151
> +	       Rectangle {
> +		   id: prevButton

We really have to pull this common code out in a ToolbarButton.qml as this code
is getting quite duplicated, but this can be done in a different patch if you
don't want to take care of it.