51653 2010-12-27 13:25:13 -0800 Text selection code shouldn't be invoked when -webkit-user-select is none 2014-02-03 03:50:51 -0800 1 1 1 Unclassified WebKit HTML Editing 528+ (Nightly build) PC All RESOLVED INVALID Performance, Qt, QtTriaged P2 Normal --- 1 noam webkit-unassigned commit-queue eric joel.parks menard rniwa oldest_to_newest 327104 0 noam 2010-12-27 13:25:13 -0800 This has found to have a major performance impact on some use cases, mainly when using finger-touch or mouse to scroll. It seems like even when user-select is turned off, the logic to break the text into words/characters, which is part of the selection logic, is unnecessarily invoked. The text-breaking code in some cases can be performance intensive. This can be rectified by making sure that the user-select code is on before starting the text-breaking logic. 327105 1 77517 noam 2010-12-27 13:35:05 -0800 Created attachment 77517 Patch 327114 2 77517 kenneth 2010-12-27 14:15:47 -0800 Comment on attachment 77517 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77517&action=review > WebCore/page/EventHandler.cpp:611 > + const RenderStyle* style = targetRenderer->style(false); It would be nice to know what the false stood for. Without knowing that this code is quite unreadable. 327116 3 noam 2010-12-27 14:22:12 -0800 > It would be nice to know what the false stood for. Without knowing that this code is quite unreadable. Yep, good ole boolean trap. It stands for "firstLine" :) In this case we want the regular style. I'd make that more readable after I get some more comments about the content... 327117 4 77519 noam 2010-12-27 14:26:20 -0800 Created attachment 77519 Patch 327355 5 77519 eric 2010-12-28 13:58:47 -0800 Comment on attachment 77519 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77519&action=review Good idea, we should just clean this up slightly. > WebCore/page/EventHandler.cpp:617 > + // Don't modify the selection for nodes with -webkit-user-select: none. > + const RenderStyle* style = targetRenderer->style(); > + const EUserSelect userSelect = style->userSelect(); > + if (userSelect == SELECT_NONE) { > + const RenderStyle* styleForFirstLine = targetRenderer->firstLineStyle(); > + if (styleForFirstLine == style || styleForFirstLine->userSelect() == SELECT_NONE) > + return; > + } This feels like a separate (private or static) function. if (!selectionEnabled()) return; or something like that, no? 327357 6 noam 2010-12-28 14:01:59 -0800 (In reply to comment #5) > (From update of attachment 77519 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=77519&action=review > > This feels like a separate (private or static) function. > > if (!selectionEnabled()) > return; > > or something like that, no? Sure thing. Will repost promptly. 327375 7 77578 noam 2010-12-28 14:19:16 -0800 Created attachment 77578 Patch 327378 8 77578 eric 2010-12-28 14:20:56 -0800 Comment on attachment 77578 Patch LGTM. Thanks. 327452 9 77578 commit-queue 2010-12-28 17:27:17 -0800 Comment on attachment 77578 Patch Rejecting attachment 77578 from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2 Last 500 characters of output: .......................................................................................................................................................................................................... editing/selection .................................................... editing/selection/5333725.html -> failed Exiting early after 1 failures. 5182 tests run. 105.66s total testing time 5181 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 3 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/7290229 332221 10 eric 2011-01-11 03:12:04 -0800 Looks like this needs an updated patch. 332278 11 noam 2011-01-11 05:19:49 -0800 (In reply to comment #10) > Looks like this needs an updated patch. I agree but I'm not sure how to fix it :) 371721 12 joel.parks 2011-03-22 15:30:52 -0700 adding keyword "InTSW" for tracking by QtWebKit EM 378506 13 rniwa 2011-04-03 00:52:02 -0700 Is this bug fixed or we haven't landed the patch yet because of the test failure? 378553 14 noam 2011-04-03 08:24:18 -0700 The test failure actually pointed to a problem that makes the patch invalid. Need to find a different way to do this. 975824 15 jturcotte 2014-02-03 03:50:51 -0800 === Bulk closing of Qt bugs === If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it. If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines. 77517 2010-12-27 13:35:05 -0800 2010-12-27 14:26:15 -0800 Patch bug-51653-20101227133503.patch text/plain 1884 noam ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg YjE3Nzc3YTUzMzFlZjNiYjQ1MjAxYzllYjY0NjliNjA3YWZjNjk0OS4uYmE3ZmViMGNmODk2NWYy MjEwYjMxZTA2NzA5NDExMDYwZGIzMzE5ZiAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cK KysrIGIvV2ViQ29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxOCBAQAorMjAxMC0xMi0yNyAgTm9h bSBSb3NlbnRoYWwgIDxub2FtLnJvc2VudGhhbEBub2tpYS5jb20+CisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgVGV4dCBzZWxlY3Rpb24gY29kZSBzaG91 bGRuJ3QgYmUgaW52b2tlZCB3aGVuIC13ZWJraXQtdXNlci1zZWxlY3QgaXMgbm9uZS4KKyAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTUxNjUzCisKKyAgICAg ICAgQ2hlY2sgdGhlIHN0eWxlIGZvciB0aGUgdGFyZ2V0IG5vZGUgcmVuZGVyZXIgYmVmb3JlIHN0 YXJ0aW5nIHRoZSB1c2VyIHNlbGVjdGlvbi4KKyAgICAgICAgT25seSBwZXJmb3JtIHRoZSB1c2Vy IHNlbGVjdGlvbiB0ZXN0cyBpZiAtd2Via2l0LXVzZXItc2VsZWN0IGlzIHNldCB0byAidGV4dCIu CisKKyAgICAgICAgTm8gbmV3IHRlc3RzLiBUaGlzIGlzIGFuIG9wdGltaXphdGlvbiB0byBmdW5j dGlvbmFsaXR5IHRoYXQncyBjb3ZlcmVkIGJ5IGV4aXN0aW5nIHRlc3RzLgorCisgICAgICAgICog cGFnZS9FdmVudEhhbmRsZXIuY3BwOgorICAgICAgICAoV2ViQ29yZTo6RXZlbnRIYW5kbGVyOjp1 cGRhdGVTZWxlY3Rpb25Gb3JNb3VzZURyYWcpOgorCiAyMDEwLTEyLTI0ICBEYW4gQmVybnN0ZWlu ICA8bWl0ekBhcHBsZS5jb20+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgQWxleGV5IFByb3NrdXJ5 YWtvdi4KZGlmZiAtLWdpdCBhL1dlYkNvcmUvcGFnZS9FdmVudEhhbmRsZXIuY3BwIGIvV2ViQ29y ZS9wYWdlL0V2ZW50SGFuZGxlci5jcHAKaW5kZXggMGM1MDcxZTg2MTVhNGIxZWQ1ODBmOGMzODEx NTg2MTBiNzEzZjg3Ny4uOWRlZGVlYzI4OGVlYzQ1YjQ5NjRlMjYzN2M4ZmMwMWQxOTVlY2I2YyAx MDA2NDQKLS0tIGEvV2ViQ29yZS9wYWdlL0V2ZW50SGFuZGxlci5jcHAKKysrIGIvV2ViQ29yZS9w YWdlL0V2ZW50SGFuZGxlci5jcHAKQEAgLTYwNyw2ICs2MDcsMTUgQEAgdm9pZCBFdmVudEhhbmRs ZXI6OnVwZGF0ZVNlbGVjdGlvbkZvck1vdXNlRHJhZyhOb2RlKiB0YXJnZXROb2RlLCBjb25zdCBJ bnRQb2ludCYKICAgICBpZiAoIXRhcmdldFJlbmRlcmVyKQogICAgICAgICByZXR1cm47CiAKKyAg ICAvLyBEb24ndCBtb2RpZnkgdGhlIHNlbGVjdGlvbiBmb3Igbm9kZXMgd2l0aCAtd2Via2l0LXVz ZXItc2VsZWN0OiBub25lLgorICAgIGNvbnN0IFJlbmRlclN0eWxlKiBzdHlsZSA9IHRhcmdldFJl bmRlcmVyLT5zdHlsZShmYWxzZSk7CisgICAgY29uc3QgRVVzZXJTZWxlY3QgdXNlclNlbGVjdCA9 IHN0eWxlLT51c2VyU2VsZWN0KCk7CisgICAgaWYgKHVzZXJTZWxlY3QgPT0gU0VMRUNUX05PTkUp IHsKKyAgICAgICAgY29uc3QgUmVuZGVyU3R5bGUqIHN0eWxlRm9yRmlyc3RMaW5lID0gdGFyZ2V0 UmVuZGVyZXItPnN0eWxlKHRydWUpOworICAgICAgICBpZiAoc3R5bGVGb3JGaXJzdExpbmUgPT0g c3R5bGUgfHwgc3R5bGVGb3JGaXJzdExpbmUtPnVzZXJTZWxlY3QoKSA9PSBTRUxFQ1RfTk9ORSkK KyAgICAgICAgICAgIHJldHVybjsKKyAgICB9CisKICAgICBWaXNpYmxlUG9zaXRpb24gdGFyZ2V0 UG9zaXRpb24odGFyZ2V0UmVuZGVyZXItPnBvc2l0aW9uRm9yUG9pbnQobG9jYWxQb2ludCkpOwog CiAgICAgLy8gRG9uJ3QgbW9kaWZ5IHRoZSBzZWxlY3Rpb24gaWYgd2UncmUgbm90IG9uIGEgbm9k ZS4K 77519 2010-12-27 14:26:20 -0800 2010-12-28 14:19:12 -0800 Patch bug-51653-20101227142618.patch text/plain 1884 noam ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg YjE3Nzc3YTUzMzFlZjNiYjQ1MjAxYzllYjY0NjliNjA3YWZjNjk0OS4uYmE3ZmViMGNmODk2NWYy MjEwYjMxZTA2NzA5NDExMDYwZGIzMzE5ZiAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cK KysrIGIvV2ViQ29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxOCBAQAorMjAxMC0xMi0yNyAgTm9h bSBSb3NlbnRoYWwgIDxub2FtLnJvc2VudGhhbEBub2tpYS5jb20+CisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgVGV4dCBzZWxlY3Rpb24gY29kZSBzaG91 bGRuJ3QgYmUgaW52b2tlZCB3aGVuIC13ZWJraXQtdXNlci1zZWxlY3QgaXMgbm9uZS4KKyAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTUxNjUzCisKKyAgICAg ICAgQ2hlY2sgdGhlIHN0eWxlIGZvciB0aGUgdGFyZ2V0IG5vZGUgcmVuZGVyZXIgYmVmb3JlIHN0 YXJ0aW5nIHRoZSB1c2VyIHNlbGVjdGlvbi4KKyAgICAgICAgT25seSBwZXJmb3JtIHRoZSB1c2Vy IHNlbGVjdGlvbiB0ZXN0cyBpZiAtd2Via2l0LXVzZXItc2VsZWN0IGlzIHNldCB0byAidGV4dCIu CisKKyAgICAgICAgTm8gbmV3IHRlc3RzLiBUaGlzIGlzIGFuIG9wdGltaXphdGlvbiB0byBmdW5j dGlvbmFsaXR5IHRoYXQncyBjb3ZlcmVkIGJ5IGV4aXN0aW5nIHRlc3RzLgorCisgICAgICAgICog cGFnZS9FdmVudEhhbmRsZXIuY3BwOgorICAgICAgICAoV2ViQ29yZTo6RXZlbnRIYW5kbGVyOjp1 cGRhdGVTZWxlY3Rpb25Gb3JNb3VzZURyYWcpOgorCiAyMDEwLTEyLTI0ICBEYW4gQmVybnN0ZWlu ICA8bWl0ekBhcHBsZS5jb20+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgQWxleGV5IFByb3NrdXJ5 YWtvdi4KZGlmZiAtLWdpdCBhL1dlYkNvcmUvcGFnZS9FdmVudEhhbmRsZXIuY3BwIGIvV2ViQ29y ZS9wYWdlL0V2ZW50SGFuZGxlci5jcHAKaW5kZXggMGM1MDcxZTg2MTVhNGIxZWQ1ODBmOGMzODEx NTg2MTBiNzEzZjg3Ny4uYWVmMTVlMDBhN2JmY2E3YTc5ODM3ODEwMWEzY2VmYzczODllYzk5YiAx MDA2NDQKLS0tIGEvV2ViQ29yZS9wYWdlL0V2ZW50SGFuZGxlci5jcHAKKysrIGIvV2ViQ29yZS9w YWdlL0V2ZW50SGFuZGxlci5jcHAKQEAgLTYwNyw2ICs2MDcsMTUgQEAgdm9pZCBFdmVudEhhbmRs ZXI6OnVwZGF0ZVNlbGVjdGlvbkZvck1vdXNlRHJhZyhOb2RlKiB0YXJnZXROb2RlLCBjb25zdCBJ bnRQb2ludCYKICAgICBpZiAoIXRhcmdldFJlbmRlcmVyKQogICAgICAgICByZXR1cm47CiAKKyAg ICAvLyBEb24ndCBtb2RpZnkgdGhlIHNlbGVjdGlvbiBmb3Igbm9kZXMgd2l0aCAtd2Via2l0LXVz ZXItc2VsZWN0OiBub25lLgorICAgIGNvbnN0IFJlbmRlclN0eWxlKiBzdHlsZSA9IHRhcmdldFJl bmRlcmVyLT5zdHlsZSgpOworICAgIGNvbnN0IEVVc2VyU2VsZWN0IHVzZXJTZWxlY3QgPSBzdHls ZS0+dXNlclNlbGVjdCgpOworICAgIGlmICh1c2VyU2VsZWN0ID09IFNFTEVDVF9OT05FKSB7Cisg ICAgICAgIGNvbnN0IFJlbmRlclN0eWxlKiBzdHlsZUZvckZpcnN0TGluZSA9IHRhcmdldFJlbmRl cmVyLT5maXJzdExpbmVTdHlsZSgpOworICAgICAgICBpZiAoc3R5bGVGb3JGaXJzdExpbmUgPT0g c3R5bGUgfHwgc3R5bGVGb3JGaXJzdExpbmUtPnVzZXJTZWxlY3QoKSA9PSBTRUxFQ1RfTk9ORSkK KyAgICAgICAgICAgIHJldHVybjsKKyAgICB9CisKICAgICBWaXNpYmxlUG9zaXRpb24gdGFyZ2V0 UG9zaXRpb24odGFyZ2V0UmVuZGVyZXItPnBvc2l0aW9uRm9yUG9pbnQobG9jYWxQb2ludCkpOwog CiAgICAgLy8gRG9uJ3QgbW9kaWZ5IHRoZSBzZWxlY3Rpb24gaWYgd2UncmUgbm90IG9uIGEgbm9k ZS4K 77578 2010-12-28 14:19:16 -0800 2011-01-11 05:19:30 -0800 Patch bug-51653-20101228141915.patch text/plain 2006 noam ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg YjE3Nzc3YTUzMzFlZjNiYjQ1MjAxYzllYjY0NjliNjA3YWZjNjk0OS4uYmE3ZmViMGNmODk2NWYy MjEwYjMxZTA2NzA5NDExMDYwZGIzMzE5ZiAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cK KysrIGIvV2ViQ29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxOCBAQAorMjAxMC0xMi0yNyAgTm9h bSBSb3NlbnRoYWwgIDxub2FtLnJvc2VudGhhbEBub2tpYS5jb20+CisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgVGV4dCBzZWxlY3Rpb24gY29kZSBzaG91 bGRuJ3QgYmUgaW52b2tlZCB3aGVuIC13ZWJraXQtdXNlci1zZWxlY3QgaXMgbm9uZS4KKyAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTUxNjUzCisKKyAgICAg ICAgQ2hlY2sgdGhlIHN0eWxlIGZvciB0aGUgdGFyZ2V0IG5vZGUgcmVuZGVyZXIgYmVmb3JlIHN0 YXJ0aW5nIHRoZSB1c2VyIHNlbGVjdGlvbi4KKyAgICAgICAgT25seSBwZXJmb3JtIHRoZSB1c2Vy IHNlbGVjdGlvbiB0ZXN0cyBpZiAtd2Via2l0LXVzZXItc2VsZWN0IGlzIHNldCB0byAidGV4dCIu CisKKyAgICAgICAgTm8gbmV3IHRlc3RzLiBUaGlzIGlzIGFuIG9wdGltaXphdGlvbiB0byBmdW5j dGlvbmFsaXR5IHRoYXQncyBjb3ZlcmVkIGJ5IGV4aXN0aW5nIHRlc3RzLgorCisgICAgICAgICog cGFnZS9FdmVudEhhbmRsZXIuY3BwOgorICAgICAgICAoV2ViQ29yZTo6RXZlbnRIYW5kbGVyOjp1 cGRhdGVTZWxlY3Rpb25Gb3JNb3VzZURyYWcpOgorCiAyMDEwLTEyLTI0ICBEYW4gQmVybnN0ZWlu ICA8bWl0ekBhcHBsZS5jb20+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgQWxleGV5IFByb3NrdXJ5 YWtvdi4KZGlmZiAtLWdpdCBhL1dlYkNvcmUvcGFnZS9FdmVudEhhbmRsZXIuY3BwIGIvV2ViQ29y ZS9wYWdlL0V2ZW50SGFuZGxlci5jcHAKaW5kZXggMGM1MDcxZTg2MTVhNGIxZWQ1ODBmOGMzODEx NTg2MTBiNzEzZjg3Ny4uZWNkZmE1NzdmZWE5NmRjMTUxODZkNTE5MTA0NjUxYzRlN2U2MmQ3OSAx MDA2NDQKLS0tIGEvV2ViQ29yZS9wYWdlL0V2ZW50SGFuZGxlci5jcHAKKysrIGIvV2ViQ29yZS9w YWdlL0V2ZW50SGFuZGxlci5jcHAKQEAgLTU5Miw2ICs1OTIsMTIgQEAgdm9pZCBFdmVudEhhbmRs ZXI6OnVwZGF0ZVNlbGVjdGlvbkZvck1vdXNlRHJhZygpCiAgICAgdXBkYXRlU2VsZWN0aW9uRm9y TW91c2VEcmFnKHJlc3VsdC5pbm5lck5vZGUoKSwgcmVzdWx0LmxvY2FsUG9pbnQoKSk7CiB9CiAK K3N0YXRpYyBib29sIGlzVXNlclNlbGVjdGlvbkVuYWJsZWQoUmVuZGVyT2JqZWN0KiByZW5kZXJl cikKK3sKKyAgICByZXR1cm4gKHJlbmRlcmVyLT5zdHlsZSgpLT51c2VyU2VsZWN0KCkgIT0gU0VM RUNUX05PTkUKKyAgICAgICAgICAgIHx8IHJlbmRlcmVyLT5maXJzdExpbmVTdHlsZSgpLT51c2Vy U2VsZWN0KCkgIT0gU0VMRUNUX05PTkUpOworfQorCiB2b2lkIEV2ZW50SGFuZGxlcjo6dXBkYXRl U2VsZWN0aW9uRm9yTW91c2VEcmFnKE5vZGUqIHRhcmdldE5vZGUsIGNvbnN0IEludFBvaW50JiBs b2NhbFBvaW50KQogewogICAgIGlmICghbV9tb3VzZURvd25NYXlTdGFydFNlbGVjdCkKQEAgLTYw Nyw2ICs2MTMsOSBAQCB2b2lkIEV2ZW50SGFuZGxlcjo6dXBkYXRlU2VsZWN0aW9uRm9yTW91c2VE cmFnKE5vZGUqIHRhcmdldE5vZGUsIGNvbnN0IEludFBvaW50JgogICAgIGlmICghdGFyZ2V0UmVu ZGVyZXIpCiAgICAgICAgIHJldHVybjsKIAorICAgIGlmICghaXNVc2VyU2VsZWN0aW9uRW5hYmxl ZCh0YXJnZXRSZW5kZXJlcikpCisgICAgICAgIHJldHVybjsKKwogICAgIFZpc2libGVQb3NpdGlv biB0YXJnZXRQb3NpdGlvbih0YXJnZXRSZW5kZXJlci0+cG9zaXRpb25Gb3JQb2ludChsb2NhbFBv aW50KSk7CiAKICAgICAvLyBEb24ndCBtb2RpZnkgdGhlIHNlbGVjdGlvbiBpZiB3ZSdyZSBub3Qg b24gYSBub2RlLgo=