WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
48722
[Qt] Add a 'hasSelection' function to QWebView and QWebPage...
https://bugs.webkit.org/show_bug.cgi?id=48722
Summary
[Qt] Add a 'hasSelection' function to QWebView and QWebPage...
Dawit A.
Reported
2010-10-30 19:18:21 PDT
The attached patch adds a hasSelection function for quickly determining if there is selected content on the page. This is faster than doing selectedText().isEmpty()...
Attachments
proposed patch...
(5.31 KB, patch)
2010-10-30 21:27 PDT
,
Dawit A.
no flags
Details
Formatted Diff
Diff
proposed patch (Update I)...
(5.02 KB, patch)
2010-10-30 21:38 PDT
,
Dawit A.
no flags
Details
Formatted Diff
Diff
proposed patch (Update II)...
(5.02 KB, patch)
2010-10-30 21:44 PDT
,
Dawit A.
no flags
Details
Formatted Diff
Diff
proposed patch (Update III)
(5.02 KB, patch)
2010-10-30 22:50 PDT
,
Dawit A.
no flags
Details
Formatted Diff
Diff
Proposed patch (Update IV)
(5.04 KB, patch)
2010-12-22 21:09 PST
,
Dawit A.
no flags
Details
Formatted Diff
Diff
Proposed patch (Update V)
(5.26 KB, patch)
2010-12-23 07:08 PST
,
Dawit A.
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Dawit A.
Comment 1
2010-10-30 21:27:51 PDT
Created
attachment 72447
[details]
proposed patch...
Dawit A.
Comment 2
2010-10-30 21:38:31 PDT
Created
attachment 72448
[details]
proposed patch (Update I)... Fixed code style issue (trailing whitespaces) in the previous patch.
Early Warning System Bot
Comment 3
2010-10-30 21:39:35 PDT
Attachment 72447
[details]
did not build on qt: Build output:
http://queues.webkit.org/results/4816140
Dawit A.
Comment 4
2010-10-30 21:44:33 PDT
Created
attachment 72449
[details]
proposed patch (Update II)... More trailing whitespace removal...
Dawit A.
Comment 5
2010-10-30 22:50:12 PDT
Created
attachment 72451
[details]
proposed patch (Update III) Fixed the test case code. It was meant to use QCOMPARE not QVERIFY...
Andreas Kling
Comment 6
2010-10-31 05:18:00 PDT
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.)
Kenneth Rohde Christiansen
Comment 7
2010-10-31 06:00:37 PDT
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?
Dawit A.
Comment 8
2010-10-31 10:03:07 PDT
(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.
Kenneth Rohde Christiansen
Comment 9
2010-10-31 12:31:21 PDT
(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.
Andreas Kling
Comment 10
2010-10-31 12:37:57 PDT
(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.
Andreas Kling
Comment 11
2010-10-31 12:52:24 PDT
(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.
Dawit A.
Comment 12
2010-10-31 15:24:00 PDT
(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()...
Kenneth Rohde Christiansen
Comment 13
2010-10-31 16:00:28 PDT
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
Eric Seidel (no email)
Comment 14
2010-12-14 01:35:20 PST
I'm unsure what's left to be done here?
Dawit A.
Comment 15
2010-12-22 21:09:20 PST
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...
Kenneth Rohde Christiansen
Comment 16
2010-12-23 02:05:07 PST
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.
Dawit A.
Comment 17
2010-12-23 07:08:34 PST
Created
attachment 77332
[details]
Proposed patch (Update V) Addressed the issues in review
comment #16
.
Dawit A.
Comment 18
2010-12-23 07:09:32 PST
Comment on
attachment 72451
[details]
proposed patch (Update III) Removed the r+ flag from the old patch...
WebKit Commit Bot
Comment 19
2010-12-23 08:23:39 PST
Comment on
attachment 77332
[details]
Proposed patch (Update V) Clearing flags on attachment: 77332 Committed
r74558
: <
http://trac.webkit.org/changeset/74558
>
WebKit Commit Bot
Comment 20
2010-12-23 08:23:47 PST
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 21
2010-12-23 09:05:13 PST
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
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug