Expose functionality provided by WebPageProxy::findString and PageFindClient to QtQuick applications.
Created attachment 163549 [details] [Qt][WK2] Add API to find text from page Proposed implementation
Nice! With a test too :) If you need a review you need to set the r? flag though.
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.
Created attachment 163566 [details] [Qt][WK2] Add API to find text from page Updated ChangeLog to include bug number
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.
Created attachment 163605 [details] [Qt][WK2] Add experimental API to find text from page New version with API moved to QtWebKit.experimental
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.
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.
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.
(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.
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) :)
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.
Would you find some time to review this patch Simon? Thanks! :)
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.
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
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.
Created attachment 179068 [details] [Qt][WK2] Add experimental API to find text from page Fixed missing newline
Is there something I can do to help get this patch forward?
(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.
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.
Comment on attachment 179068 [details] [Qt][WK2] Add experimental API to find text from page Putting on hold for commit queue for a minute
Comment on attachment 179068 [details] [Qt][WK2] Add experimental API to find text from page Ooops, sorry, it's experimental :)
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"
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>
All reviewed patches have been landed. Closing bug.