Bug 96481 – [Qt][WK2] Add API to find text from page

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Heikki Paajanen]

Expose functionality provided by WebPageProxy::findString and PageFindClient to QtQuick applications.

[Heikki Paajanen]

Created attachment 163549 [details]
[Qt][WK2] Add API to find text from page

Proposed implementation

[Philippe Normand]

Nice! With a test too :)
If you need a review you need to set the r? flag though.

[WebKit Review Bot]

Attachment 163549 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Heikki Paajanen]

Created attachment 163566 [details]
[Qt][WK2] Add API to find text from page

Updated ChangeLog to include bug number

[Andras Becsi]

Comment on attachment 163566 [details]
[Qt][WK2] Add API to find text from page

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

> Source/WebKit2/ChangeLog:3
> +        [Qt][WK2] Add API to find text from page

I think this has to go into QtWebKit.experimental for now.

[Heikki Paajanen]

Created attachment 163605 [details]
[Qt][WK2] Add experimental API to find text from page

New version with API moved to QtWebKit.experimental

[Andras Becsi]

Comment on attachment 163605 [details]
[Qt][WK2] Add experimental API to find text from page

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

The implementation itself looks good, also with a nice QML test. I think having an example implementation with a simple UI in MiniBrowser, too, would be even better.

Although I'm not sure this is the best API to provide.

Maybe others also have an opinion on how it should look like.

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:289
> +    enum FindOptions {

Probably not all of these options make sense for us. Although it's convenient to cast this to WebKit::FindOptions it might be worth to make the options similar to what QWebPage::FindFlags provides.
The name FindOptions is fine but I think the enum values do not need the "FindOptions" prefix.

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:290
> +        FindOptionsCaseInsensitive = 1 << 0,

Since case insensitive is the default, the flag should be something like FindCaseSensitive.

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:292
> +        FindOptionsAtWordStarts = 1 << 1,
> +        FindOptionsTreatMedialCapitalAsWordStart = 1 << 2,

Not sure having these options makes sense.

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:293
> +        FindOptionsBackwards = 1 << 3,

FindBackwards, etc.

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:296
> +        FindOptionsShowFindIndicator = 1 << 6,

If this is the setting for the small indicators on the scrollbar then we certainly do not support it since scrollbars are created by the embedder in QML.

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:369
> +    void findString(const QString& string, FindOptions options = FindOptionsCaseInsensitive, unsigned maxMatchCount = 1000);

I think it would be better if it would resemble the Qt4 API which has:

bool findText ( const QString & subString, FindFlags options = 0 )

I'm not sure we should expose the maxMatchCount parameter though, I do not really see a usecase for it and passing 1000 by default seems pretty arbitrary.

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:370
> +    void hideFindString();

This could be omitted by making the findString clear the selection in case an empty string is passed.

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:396
> +    void stringFound(unsigned matchCount);
> +    void stringFindFailed();

Maybe these could also be merged into one signal where matchCount would determine if the search was successful or not.

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

This seems to be the notification for countStringMatches and seems a bit redundant to stringFound, considering we do not implement countStringMatches.

[Heikki Paajanen]

Created attachment 164054 [details]
[Qt][WK2] Add experimental API to find text from page

Thanks for the review & comments!

Here's an updated version where I tried to fix all of the issues.

[Andras Becsi]

Comment on attachment 164054 [details]
[Qt][WK2] Add experimental API to find text from page

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

I think the API looks OK now. Simon, what do you think?
Also Tor Arne might have comments on the MiniBrowser QML part.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1343
> +    d->webPageProxy->findString(string, wkOptions, std::numeric_limits<unsigned>::max()-1);

Missing space around the "-" after max().

> Source/WebKit2/UIProcess/qt/QtWebPageFindClient.h:24
> +#include <QtGlobal>

This include seems to be unneeded in the header.

> Source/WebKit2/UIProcess/qt/QtWebPageFindClient.h:26
> +#include <WebFrameProxy.h>

Ditto.

> Tools/MiniBrowser/qt/qml/BrowserWindow.qml:356
> +    Rectangle {
> +        id: findBar

I think the findBar might be covered by the page when scrolling to the next occurence so you probably need to adjust z as we do for the navigationBar.

> Tools/MiniBrowser/qt/qml/BrowserWindow.qml:424
> +                    font {
> +                        pointSize: 11
> +                        family: "Sans"
> +                        }

The indentation is off for the bracket.

[Simon Hausmann]

(In reply to comment #9)
> (From update of attachment 164054 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164054&action=review
> 
> I think the API looks OK now. Simon, what do you think?
> Also Tor Arne might have comments on the MiniBrowser QML part.

I'm finding it difficult right now to prioritize time for API reviews, I'm instead spending my time on trying to get things stable and shippable. So maybe somebody else is up for reviewing it :), otherwise I'll take a look when I find time.

[Simon Hausmann]

Comment on attachment 164054 [details]
[Qt][WK2] Add experimental API to find text from page

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

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:295
> +    enum FindFlags {
> +        FindCaseSensitively = 1 << 0,
> +        FindBackward = 1 << 1,
> +        FindWrapsAroundDocument = 1 << 2,
> +        FindHighlightAllOccurrences = 1 << 3
> +    };

This should probably become a real QFlag, i.e. this type is called FindFlag and then you use Q_DECLARE_FLAGS and Q_DECLARE_OPERATORS_FOR_FLAGS to turn FindFlag into FindFlags. Then you can combine them with logical and and or operators without the need for casting, registered in the class with Q_FLAGS it makes them properly accessible through introspection and you'll get a default constructor that avoids the options = static_cast<FindFlags>(0) :)

[Heikki Paajanen]

Created attachment 164346 [details]
[Qt][WK2] Add experimental API to find text from page

Thanks again for review!

This version addresses the issues listed in comment #9 and comment #11

Btw, hiding find bar when page is scrolled down causes page to jump to the beginning. Is that a known issue?
I think it's caused by change in the size of WebView.

[Philippe Normand]

Would you find some time to review this patch Simon? Thanks! :)

[Jocelyn Turcotte]

Comment on attachment 164346 [details]
[Qt][WK2] Add experimental API to find text from page

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.

[Heikki Paajanen]

Created attachment 175200 [details]
[Qt][WK2] Add experimental API to find text from page

Thanks for the review! Here's a new version.

I think ToolbarButton refactoring would indeed be better in it's own patch

[Jocelyn Turcotte]

Comment on attachment 175200 [details]
[Qt][WK2] Add experimental API to find text from page

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

Sorry for taking so long, LGTM now.

> Tools/MiniBrowser/qt/BrowserWindow.cpp:106
>  }
> +void BrowserWindow::toggleFind()

Missing newline, please fix before landing or re-upload a patch if you want to use the commit queue.

[Heikki Paajanen]

Created attachment 179068 [details]
[Qt][WK2] Add experimental API to find text from page

Fixed missing newline

[Heikki Paajanen]

Is there something I can do to help get this patch forward?

[Jocelyn Turcotte]

(In reply to comment #18)
> Is there something I can do to help get this patch forward?

I r+ed it, so the patch is approved. You just need to set the cq=? flag by clicking "Details" or land it yourself if you have commit rights.

[Simon Hausmann]

Comment on attachment 179068 [details]
[Qt][WK2] Add experimental API to find text from page

Since this is the first change of the API after the initial release, there are two things that need to be done I think:

    (1) The API unit test needs to be adapted to not fail. (It fails when new API is added, to prevent accidental publishing of properties)

    (2) The new functionality needs to be versioned properly. If you do "import QtWebKit 3.0" none of the new API must be visible, if you do "import QtWebKit 3.1" only then it should be accessible.

This can be done in a separate patch, but I don't think it's a good idea to at least land a change that we know is going to cause the API test to fail.

[Simon Hausmann]

Comment on attachment 179068 [details]
[Qt][WK2] Add experimental API to find text from page

Putting on hold for commit queue for a minute

[Simon Hausmann]

Comment on attachment 179068 [details]
[Qt][WK2] Add experimental API to find text from page

Ooops, sorry, it's experimental :)

[Simon Hausmann]

Comment on attachment 179068 [details]
[Qt][WK2] Add experimental API to find text from page

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

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

Some feedback on the API. Perhaps the find match count should also be a _property_, so that it's not only accessible using the imperative signal handling API but also declarative.

Maybe "findMatchCount"

[WebKit Review Bot]

Comment on attachment 179068 [details]
[Qt][WK2] Add experimental API to find text from page

Clearing flags on attachment: 179068

Committed r138618: <http://trac.webkit.org/changeset/138618>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.