44252 2010-08-19 05:11:34 -0700 QWebView::selectionChanged() is never emitted. 2011-04-19 05:15:29 -0700 1 1 1 Unclassified WebKit WebKit Qt 528+ (Nightly build) All All RESOLVED FIXED Qt P2 Normal --- 0 marc webkit-qt-unassigned ademar commit-queue hausmann kenneth kling tonikitoo webkit.review.bot oldest_to_newest 266660 0 64830 marc 2010-08-19 05:11:34 -0700 Created attachment 64830 Proposed fix - apply with patch -p0 QWebView has a selectionChanged() signal that is never emitted. I can't find a connect() to it in the source code, and setting a breakpoint on it, and QWebPage::selectionChanged(), the latter is hit while the former is not. It's rather obvious it's been forgotten to be connected :) 266662 1 webkit.review.bot 2010-08-19 05:15:26 -0700 Attachment 64830 did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 Total errors found: 0 in 0 files If any of these errors are false positives, please file a bug against check-webkit-style. 266761 2 64830 kling 2010-08-19 08:39:42 -0700 Comment on attachment 64830 Proposed fix - apply with patch -p0 Looks sane, but you need a ChangeLog entry. See http://trac.webkit.org/wiki/QtWebKitContrib for more information. 267147 3 64928 marc 2010-08-19 23:34:20 -0700 Created attachment 64928 Proposed fix v2 (with ChangeLog) 267148 4 webkit.review.bot 2010-08-19 23:38:50 -0700 Attachment 64928 did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKit/qt/ChangeLog:7: Line contains tab character. [whitespace/tab] [5] WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebview.cpp" Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style. 267160 5 64932 marc 2010-08-20 00:47:02 -0700 Created attachment 64932 Proposed fix v3 (with ChangeLog w/o tabs) 267209 6 64932 tonikitoo 2010-08-20 05:00:49 -0700 Comment on attachment 64932 Proposed fix v3 (with ChangeLog w/o tabs) Looks ok. Please do the same for qgraphicswebview.cpp 267216 7 marc 2010-08-20 05:44:03 -0700 QGraphicsWebView doesn't _have_ a selectionChanged() signal. Maybe it should have one, but this report is about an _existing_ signal not being emitted. 267220 8 64947 marc 2010-08-20 05:56:19 -0700 Created attachment 64947 Add a selectionChanged() signal to QGraphicsWebView as per Antonio Gomes' request 267222 9 64947 kenneth 2010-08-20 06:00:28 -0700 Comment on attachment 64947 Add a selectionChanged() signal to QGraphicsWebView as per Antonio Gomes' request Shouldn't you add the selectedText method as well? Please make sure that there are no other important selection methods missing? Do we really want all these methods from QWebPage everywhere? Simon? 267269 10 tonikitoo 2010-08-20 09:30:38 -0700 > Do we really want all these methods from QWebPage everywhere? Simon? Good point. We possibly do not need them all connected to our view's. @marc, can not you make use of qwebpage's selectionChanged signal for your needs? If so, we could make it wont fix. Btw, if we decide to go for it , you should combine both you previous patch with your newer patch in one. 267603 11 marc 2010-08-21 13:43:09 -0700 > @marc, can not you make use of qwebpage's selectionChanged signal for your > needs? If so, we could make it wont fix. The point of the original report was that QWebView::selectionChanged() _which exists and is documented_ is never emitted. You cannot remove that signal due to binary compatibility. You can, of course, obsolete it, but that's not my call to make (and not my task to implement). Either way, the solution can't be wontfix. The second patch was a request from you guys. Take it or leave it, I don't care either way :) > Btw, if we decide to go for it , you should combine both you previous > patch with your newer patch in one. Sorry, I've been through enough iterations of this patch already :) 267723 12 64932 tonikitoo 2010-08-22 16:15:28 -0700 Comment on attachment 64932 Proposed fix v3 (with ChangeLog w/o tabs) (In reply to comment #11) > The point of the original report was that QWebView::selectionChanged() _which exists and is documented_ is never emitted. You cannot remove that signal due to binary compatibility. You can, of course, obsolete it, but that's not my call to make (and not my task to implement). Either way, the solution can't be wontfix. Ok, I was wrong. r=me on your 3rd patch. > The second patch was a request from you guys. Take it or leave it, I don't care either way :) I do not think we need to have this signal to QGraphicsWebView. At least not without a public discussion on the mainling list about. So lets go for your first approach. 267724 13 64947 tonikitoo 2010-08-22 16:17:08 -0700 Comment on attachment 64947 Add a selectionChanged() signal to QGraphicsWebView as per Antonio Gomes' request Clearing revie on this patch, since I blindly requested for it. If you still want that, lets discuss in the mailing list first. 267728 14 64932 commit-queue 2010-08-22 17:14:26 -0700 Comment on attachment 64932 Proposed fix v3 (with ChangeLog w/o tabs) Clearing flags on attachment: 64932 Committed r65789: <http://trac.webkit.org/changeset/65789> 267729 15 commit-queue 2010-08-22 17:14:32 -0700 All reviewed patches have been landed. Closing bug. 269253 16 ademar 2010-08-25 08:06:05 -0700 Revision r65789 cherry-picked into qtwebkit-2.1 with commit be1fdcbd1af1fab0c579606fce82303b36188ca1 64830 2010-08-19 05:11:34 -0700 2010-08-19 23:34:20 -0700 Proposed fix - apply with patch -p0 qwebview-fix-selectionChanged.diff text/plain 498 marc LS0tIFdlYktpdC9xdC9BcGkvcXdlYnZpZXcuY3BwCisrKyBXZWJLaXQvcXQvQXBpL3F3ZWJ2aWV3 LmNwcH4KQEAgLTQxMCw2ICs0MTAsOCBAQCB2b2lkIFFXZWJWaWV3OjpzZXRQYWdlKFFXZWJQYWdl KiBwYWdlKQogICAgICAgICAgICAgICAgIHRoaXMsIFNJR05BTChzdGF0dXNCYXJNZXNzYWdlKFFT dHJpbmcpKSk7CiAgICAgICAgIGNvbm5lY3QoZC0+cGFnZSwgU0lHTkFMKGxpbmtDbGlja2VkKFFV cmwpKSwKICAgICAgICAgICAgICAgICB0aGlzLCBTSUdOQUwobGlua0NsaWNrZWQoUVVybCkpKTsK KyAgICAgICAgY29ubmVjdChkLT5wYWdlLCBTSUdOQUwoc2VsZWN0aW9uQ2hhbmdlZCgpKSwKKyAg ICAgICAgICAgICAgICB0aGlzLCBTSUdOQUwoc2VsZWN0aW9uQ2hhbmdlZCgpKSk7CiAKICAgICAg ICAgY29ubmVjdChkLT5wYWdlLCBTSUdOQUwobWljcm9Gb2N1c0NoYW5nZWQoKSksCiAgICAgICAg ICAgICAgICAgdGhpcywgU0xPVCh1cGRhdGVNaWNyb0ZvY3VzKCkpKTsK 64928 2010-08-19 23:34:20 -0700 2010-08-20 00:47:02 -0700 Proposed fix v2 (with ChangeLog) qwebview-fix-selectionChanged-v2.diff text/plain 1178 marc ZGlmZiAtLWdpdCBhL1dlYktpdC9xdC9BcGkvcXdlYnZpZXcuY3BwIGIvV2ViS2l0L3F0L0FwaS9x d2Vidmlldy5jcHAKaW5kZXggNWU3MjY1Zi4uMTBjZjYwYyAxMDA2NDQKLS0tIGEvV2ViS2l0L3F0 L0FwaS9xd2Vidmlldy5jcHAKKysrIGIvV2ViS2l0L3F0L0FwaS9xd2Vidmlldy5jcHAKQEAgLTQx NSw2ICs0MTUsOCBAQCB2b2lkIFFXZWJWaWV3OjpzZXRQYWdlKFFXZWJQYWdlKiBwYWdlKQogICAg ICAgICAgICAgICAgIHRoaXMsIFNJR05BTChzdGF0dXNCYXJNZXNzYWdlKFFTdHJpbmcpKSk7CiAg ICAgICAgIGNvbm5lY3QoZC0+cGFnZSwgU0lHTkFMKGxpbmtDbGlja2VkKFFVcmwpKSwKICAgICAg ICAgICAgICAgICB0aGlzLCBTSUdOQUwobGlua0NsaWNrZWQoUVVybCkpKTsKKyAgICAgICAgY29u bmVjdChkLT5wYWdlLCBTSUdOQUwoc2VsZWN0aW9uQ2hhbmdlZCgpKSwKKyAgICAgICAgICAgICAg ICB0aGlzLCBTSUdOQUwoc2VsZWN0aW9uQ2hhbmdlZCgpKSk7CiAKICAgICAgICAgY29ubmVjdChk LT5wYWdlLCBTSUdOQUwobWljcm9Gb2N1c0NoYW5nZWQoKSksCiAgICAgICAgICAgICAgICAgdGhp cywgU0xPVCh1cGRhdGVNaWNyb0ZvY3VzKCkpKTsKZGlmZiAtLWdpdCBhL1dlYktpdC9xdC9DaGFu Z2VMb2cgYi9XZWJLaXQvcXQvQ2hhbmdlTG9nCmluZGV4IDkyNDI4ZGYuLjE0ZDFkODggMTAwNjQ0 Ci0tLSBhL1dlYktpdC9xdC9DaGFuZ2VMb2cKKysrIGIvV2ViS2l0L3F0L0NoYW5nZUxvZwpAQCAt MSwzICsxLDE0IEBACisyMDEwLTA4LTE5ICBNYXJjIE11dHogIDxtYXJjLm11dHpAa2RhYi5jb20+ CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgW1F0XSBB Y3R1YWxseSBlbWl0IHRoZSBRV2ViVmlldzo6c2VsZWN0aW9uQ2hhbmdlZCgpIHNpZ25hbAorCisJ aHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTQ0MjUyCisKKyAgICAgICAg KiBBcGkvcXdlYnZpZXcuY3BwOgorICAgICAgICAoUVdlYlZpZXc6OnNldFBhZ2UpOiBDb25uZWN0 IFFXZWJQYWdlOjpzZWxlY3Rpb25DaGFuZ2VkKCkgdG8gUVdlYlZpZXc6OnNlbGVjdGlvbkNoYW5n ZWQoKQorCiAyMDEwLTA4LTE5ICBCYWxhenMgS2VsZW1lbiAgPGtiQGluZi51LXN6ZWdlZC5odT4K IAogICAgICAgICBSZXZpZXdlZCBieSBBbnRvbmlvIEdvbWVzLgo= 64932 2010-08-20 00:47:02 -0700 2010-08-22 17:14:26 -0700 Proposed fix v3 (with ChangeLog w/o tabs) qwebview-fix-selectionChanged-v3.diff text/plain 1185 marc ZGlmZiAtLWdpdCBhL1dlYktpdC9xdC9BcGkvcXdlYnZpZXcuY3BwIGIvV2ViS2l0L3F0L0FwaS9x d2Vidmlldy5jcHAKaW5kZXggNWU3MjY1Zi4uMTBjZjYwYyAxMDA2NDQKLS0tIGEvV2ViS2l0L3F0 L0FwaS9xd2Vidmlldy5jcHAKKysrIGIvV2ViS2l0L3F0L0FwaS9xd2Vidmlldy5jcHAKQEAgLTQx NSw2ICs0MTUsOCBAQCB2b2lkIFFXZWJWaWV3OjpzZXRQYWdlKFFXZWJQYWdlKiBwYWdlKQogICAg ICAgICAgICAgICAgIHRoaXMsIFNJR05BTChzdGF0dXNCYXJNZXNzYWdlKFFTdHJpbmcpKSk7CiAg ICAgICAgIGNvbm5lY3QoZC0+cGFnZSwgU0lHTkFMKGxpbmtDbGlja2VkKFFVcmwpKSwKICAgICAg ICAgICAgICAgICB0aGlzLCBTSUdOQUwobGlua0NsaWNrZWQoUVVybCkpKTsKKyAgICAgICAgY29u bmVjdChkLT5wYWdlLCBTSUdOQUwoc2VsZWN0aW9uQ2hhbmdlZCgpKSwKKyAgICAgICAgICAgICAg ICB0aGlzLCBTSUdOQUwoc2VsZWN0aW9uQ2hhbmdlZCgpKSk7CiAKICAgICAgICAgY29ubmVjdChk LT5wYWdlLCBTSUdOQUwobWljcm9Gb2N1c0NoYW5nZWQoKSksCiAgICAgICAgICAgICAgICAgdGhp cywgU0xPVCh1cGRhdGVNaWNyb0ZvY3VzKCkpKTsKZGlmZiAtLWdpdCBhL1dlYktpdC9xdC9DaGFu Z2VMb2cgYi9XZWJLaXQvcXQvQ2hhbmdlTG9nCmluZGV4IDkyNDI4ZGYuLjE0YjdmMmMgMTAwNjQ0 Ci0tLSBhL1dlYktpdC9xdC9DaGFuZ2VMb2cKKysrIGIvV2ViS2l0L3F0L0NoYW5nZUxvZwpAQCAt MSwzICsxLDE0IEBACisyMDEwLTA4LTE5ICBNYXJjIE11dHogIDxtYXJjLm11dHpAa2RhYi5jb20+ CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgW1F0XSBB Y3R1YWxseSBlbWl0IHRoZSBRV2ViVmlldzo6c2VsZWN0aW9uQ2hhbmdlZCgpIHNpZ25hbAorCisg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD00NDI1MgorCisg ICAgICAgICogQXBpL3F3ZWJ2aWV3LmNwcDoKKyAgICAgICAgKFFXZWJWaWV3OjpzZXRQYWdlKTog Q29ubmVjdCBRV2ViUGFnZTo6c2VsZWN0aW9uQ2hhbmdlZCgpIHRvIFFXZWJWaWV3OjpzZWxlY3Rp b25DaGFuZ2VkKCkKKwogMjAxMC0wOC0xOSAgQmFsYXpzIEtlbGVtZW4gIDxrYkBpbmYudS1zemVn ZWQuaHU+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgQW50b25pbyBHb21lcy4K 64947 2010-08-20 05:56:19 -0700 2010-08-22 16:17:07 -0700 Add a selectionChanged() signal to QGraphicsWebView as per Antonio Gomes' request qgraphicswebview-add-selectionChanged.diff text/plain 2176 marc ZGlmZiAtLWdpdCBhL1dlYktpdC9xdC9BcGkvcWdyYXBoaWNzd2Vidmlldy5jcHAgYi9XZWJLaXQv cXQvQXBpL3FncmFwaGljc3dlYnZpZXcuY3BwCmluZGV4IDdmNjZkOTguLmY5MDMyODggMTAwNjQ0 Ci0tLSBhL1dlYktpdC9xdC9BcGkvcWdyYXBoaWNzd2Vidmlldy5jcHAKKysrIGIvV2ViS2l0L3F0 L0FwaS9xZ3JhcGhpY3N3ZWJ2aWV3LmNwcApAQCAtMTk5LDYgKzE5OSwxNCBAQCB2b2lkIFFHcmFw aGljc1dlYlZpZXdQcml2YXRlOjpfcV9zY2FsZUNoYW5nZWQoKQogKi8KIAogLyohCisgICAgXGZu IHZvaWQgUUdyYXBoaWNzV2ViVmlldzo6c2VsZWN0aW9uQ2hhbmdlZCgpCisKKyAgICBUaGlzIHNp Z25hbCBpcyBlbWl0dGVkIHdoZW5ldmVyIHRoZSBzZWxlY3Rpb24gb2YgdGhlIHdlYiBwYWdlIGNo YW5nZXMuCisKKyAgICBcc2EgUVdlYlBhZ2U6OnNlbGVjdGlvbkNoYW5nZWQoKQorKi8KKworLyoh CiAgICAgXGZuIHZvaWQgUUdyYXBoaWNzV2ViVmlldzo6dXJsQ2hhbmdlZChjb25zdCBRVXJsICZ1 cmwpCiAKICAgICBUaGlzIHNpZ25hbCBpcyBlbWl0dGVkIHdoZW4gdGhlIFxhIHVybCBvZiB0aGUg dmlldyBjaGFuZ2VzLgpAQCAtNDkwLDYgKzQ5OCw4IEBAIHZvaWQgUUdyYXBoaWNzV2ViVmlldzo6 c2V0UGFnZShRV2ViUGFnZSogcGFnZSkKICAgICAgICAgICAgIHRoaXMsIFNJR05BTChzdGF0dXNC YXJNZXNzYWdlKFFTdHJpbmcpKSk7CiAgICAgY29ubmVjdChkLT5wYWdlLCBTSUdOQUwobGlua0Ns aWNrZWQoUVVybCkpLAogICAgICAgICAgICAgdGhpcywgU0lHTkFMKGxpbmtDbGlja2VkKFFVcmwp KSk7CisgICAgY29ubmVjdChkLT5wYWdlLCBTSUdOQUwoc2VsZWN0aW9uQ2hhbmdlZCgpKSwKKyAg ICAgICAgICAgIHRoaXMsIFNJR05BTChzZWxlY3Rpb25DaGFuZ2VkKCkpKTsKICAgICBjb25uZWN0 KGQtPnBhZ2UsIFNJR05BTChkZXN0cm95ZWQoKSksCiAgICAgICAgICAgICB0aGlzLCBTTE9UKF9x X3BhZ2VEZXN0cm95ZWQoKSkpOwogI2lmICFkZWZpbmVkKFFUX05PX0lNKSAmJiAoZGVmaW5lZChR X1dTX1gxMSkgfHwgZGVmaW5lZChRX1dTX1FXUykgfHwgZGVmaW5lZChRX09TX1NZTUJJQU4pKQpk aWZmIC0tZ2l0IGEvV2ViS2l0L3F0L0FwaS9xZ3JhcGhpY3N3ZWJ2aWV3LmggYi9XZWJLaXQvcXQv QXBpL3FncmFwaGljc3dlYnZpZXcuaAppbmRleCA3N2YwMzU5Li5mNjJlOGNkIDEwMDY0NAotLS0g YS9XZWJLaXQvcXQvQXBpL3FncmFwaGljc3dlYnZpZXcuaAorKysgYi9XZWJLaXQvcXQvQXBpL3Fn cmFwaGljc3dlYnZpZXcuaApAQCAtMTE0LDYgKzExNCw3IEBAIFFfU0lHTkFMUzoKICAgICB2b2lk IGxvYWRQcm9ncmVzcyhpbnQgcHJvZ3Jlc3MpOwogICAgIHZvaWQgdXJsQ2hhbmdlZChjb25zdCBR VXJsJik7CiAgICAgdm9pZCB0aXRsZUNoYW5nZWQoY29uc3QgUVN0cmluZyYpOworICAgIHZvaWQg c2VsZWN0aW9uQ2hhbmdlZCgpOwogICAgIHZvaWQgaWNvbkNoYW5nZWQoKTsKICAgICB2b2lkIHN0 YXR1c0Jhck1lc3NhZ2UoY29uc3QgUVN0cmluZyYgbWVzc2FnZSk7CiAgICAgdm9pZCBsaW5rQ2xp Y2tlZChjb25zdCBRVXJsJik7CmRpZmYgLS1naXQgYS9XZWJLaXQvcXQvQ2hhbmdlTG9nIGIvV2Vi S2l0L3F0L0NoYW5nZUxvZwppbmRleCAxNGI3ZjJjLi5jMGVkZTM0IDEwMDY0NAotLS0gYS9XZWJL aXQvcXQvQ2hhbmdlTG9nCisrKyBiL1dlYktpdC9xdC9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNCBA QAorMjAxMC0wOC0yMCAgTWFyYyBNdXR6ICA8bWFyYy5tdXR6QGtkYWIuY29tPgorCisgICAgICAg IFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFtRdF0gQWRkIFFHcmFwaGlj c1dlYlZpZXc6OnNlbGVjdGlvbkNoYW5nZWQoKSBzaWduYWwgZm9yCisgICAgICAgIGNvbnNpc3Rl bmN5IHdpdGggUVdlYlZpZXcuCisKKyAgICAgICAgKiBBcGkvcWdyYXBoaWNzd2Vidmlldy5jcHA6 CisgICAgICAgIChRR3JhcGhpY3NXZWJWaWV3OjpzZXRQYWdlKTogcmVsYXkgc2VsZWN0aW9uQ2hh bmdlZCgpIHNpZ25hbCBmcm9tIFFXZWJQYWdlCisgICAgICAgICogQXBpL3FncmFwaGljc3dlYnZp ZXcuaDogKG5ldykgc2VsZWN0aW9uQ2hhbmdlZCgpIHNpZ25hbAorCiAyMDEwLTA4LTE5ICBNYXJj IE11dHogIDxtYXJjLm11dHpAa2RhYi5jb20+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZ IChPT1BTISkuCg==