[Webkit-unassigned] [Bug 96481] [Qt][WK2] Add API to find text from page

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=96481





--- Comment #7 from Andras Becsi <abecsi at webkit.org>  2012-09-12 08:34:44 PST ---
(From update of attachment 163605)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.