Bug 48722

Created attachment 72447 [details] proposed patch...

Created attachment 72448 [details] proposed patch (Update I)... Fixed code style issue (trailing whitespaces) in the previous patch.

Attachment 72447 [details] did not build on qt: Build output: http://queues.webkit.org/results/4816140

Created attachment 72449 [details] proposed patch (Update II)... More trailing whitespace removal...

Created attachment 72451 [details] proposed patch (Update III) Fixed the test case code. It was meant to use QCOMPARE not QVERIFY...

This API addition looks fine to me. CC'ing more people for ack/nak. (Sidenote: Your patch is missing \since tags and a ChangeLog entry.)

Why is this a lot faster than selectedText().isEmpty()? I mean, couldn't some of this logic be put into selectedText() or is it the construction of the empty QString that is expensive?

(In reply to comment #7) > Why is this a lot faster than selectedText().isEmpty()? I mean, couldn't some of this logic be put into selectedText() or is it the construction of the empty QString that is expensive? I think you are missing the case of there being a selection when you do the check, i.e. selectedText().isEmpty()... In that case, you end up with the code trying to convert the current selection to a normalized range and then attempting to get a string represtation of that... Obviously that is more expensive than simply asking the selection object for its type, which is a pre set flag/enum.

(In reply to comment #8) > (In reply to comment #7) > > Why is this a lot faster than selectedText().isEmpty()? I mean, couldn't some of this logic be put into selectedText() or is it the construction of the empty QString that is expensive? > > I think you are missing the case of there being a selection when you do the check, i.e. selectedText().isEmpty()... In that case, you end up with the code trying to convert the current selection to a normalized range and then attempting to get a string represtation of that... Obviously that is more expensive than simply asking the selection object for its type, which is a pre set flag/enum. Maybe I didn't get you, but why couldn't selectedText() internally not just call hasSelection and return an empty string in the case it has not? The reason is that I do not like adding such apis [optimization apis] as our users might not even use them or they won't understand why selectedText is expensive if there is no selection.

(In reply to comment #9) > Maybe I didn't get you, but why couldn't selectedText() internally not just call hasSelection and return an empty string in the case it has not? Good point, we should do this regardless.

(In reply to comment #10) > (In reply to comment #9) > > Maybe I didn't get you, but why couldn't selectedText() internally not just call hasSelection and return an empty string in the case it has not? > > Good point, we should do this regardless. Opened bug 48736 for this.

(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > Why is this a lot faster than selectedText().isEmpty()? I mean, couldn't some of this logic be put into selectedText() or is it the construction of the empty QString that is expensive? > > > > I think you are missing the case of there being a selection when you do the check, i.e. selectedText().isEmpty()... In that case, you end up with the code trying to convert the current selection to a normalized range and then attempting to get a string represtation of that... Obviously that is more expensive than simply asking the selection object for its type, which is a pre set flag/enum. > > Maybe I didn't get you, but why couldn't selectedText() internally not just call hasSelection and return an empty string in the case it has not? > > The reason is that I do not like adding such apis [optimization apis] as our users might not even use them or they won't understand why selectedText is expensive if there is no selection. We are looking at this from completely different angles. Yes, I agree what Andreas ultimately did, short-circuting selectedText() when there is no selection, in bug 48736 is a good optimization. However, that was not what I was trying to address with this patch. My intention here was to optimize the case where there is a selection, but the act of checking for the presence of a selection and actually accessing the selected content are two separate actions for the client. In that case calling selectedText().isEmpty() in one place and later on down the line calling selectedText() again would be expensive. For example when showing a menu item you might want to find out whether there is a selection to enable disable an action, but not retrieve the selected content yet. That would have to wait for the user's desired action. Anyhow, this is no different than what is in other Qt APIs. For example, QLineEdit contains both hasSelectedText() and selectedText()...

Comment on attachment 72451 [details] proposed patch (Update III) Thanks for explaning the use-case, I find this API fine, but it will need review before cherry-picking it into 2.1

I'm unsure what's left to be done here?

Created attachment 77303 [details] Proposed patch (Update IV) Updated the patch to make sure it applies against current trunk. Can someone please review and land this before it bit rots and gets forgotton ? No one seems to have any objection to this API thus far. Thanks...

Comment on attachment 77303 [details] Proposed patch (Update IV) View in context: https://bugs.webkit.org/attachment.cgi?id=77303&action=review > WebKit/qt/ChangeLog:7 > + Reviewed by NOBODY (OOPS!). > + > + [Qt] Add a 'hasSelection' function to QWebView and QWebPage... > + https://bugs.webkit.org/show_bug.cgi?id=48722 > + You need to explain here why that API is needed and why you cannot accomplish it with the current API. That is useful for git blame etc > WebKit/qt/Api/qwebpage.cpp:2630 > + By default, this property is false. Im not sure this 'by default' is needed. By default there is no selection, which means that the property is false... not the opposite.

Created attachment 77332 [details] Proposed patch (Update V) Addressed the issues in review comment #16.

Comment on attachment 72451 [details] proposed patch (Update III) Removed the r+ flag from the old patch...

Comment on attachment 77332 [details] Proposed patch (Update V) Clearing flags on attachment: 77332 Committed r74558: <http://trac.webkit.org/changeset/74558>

All reviewed patches have been landed. Closing bug.

http://trac.webkit.org/changeset/74558 might have broken GTK Linux 64-bit Debug The following tests are not passing: media/adopt-node-crash.html