193416 2019-01-14 16:19:35 -0800 Only run the node comparison code in FrameSelection::respondToNodeModification() for range selections 2019-01-15 14:49:20 -0800 1 1 1 Unclassified WebKit HTML Editing WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 simon.fraser simon.fraser commit-queue rniwa saam simon.fraser webkit-bug-importer wenson_hsieh oldest_to_newest 1494378 0 simon.fraser 2019-01-14 16:19:35 -0800 Only run the node comparison code in FrameSelection::respondToNodeModification() for range selections 1494382 1 359095 simon.fraser 2019-01-14 16:22:42 -0800 Created attachment 359095 Patch 1494384 2 simon.fraser 2019-01-14 16:23:59 -0800 I measured this as a ~1.5% improvement on Speedometer2. 1494405 3 webkit-bug-importer 2019-01-14 16:52:30 -0800 <rdar://problem/47270794> 1494416 4 359095 wenson_hsieh 2019-01-14 17:16:22 -0800 Comment on attachment 359095 Patch Nice! 1494417 5 359095 wenson_hsieh 2019-01-14 17:25:03 -0800 Comment on attachment 359095 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359095&action=review > Source/WebCore/editing/FrameSelection.cpp:545 > + if (compareResult == Range::NODE_BEFORE_AND_AFTER || compareResult == Range::NODE_INSIDE) { Doesn't this mean that we set clearRenderTreeSelection to true if either the selection completely encompasses node, or node completely encompasses the selection? It seems like in the latter case, we can enter this if statement even with a caret selection. We should double check that we either never get here with a caret selection, or that if we get here with a caret selection, we didn't need to clear the render tree selection in the first place. 1494471 6 wenson_hsieh 2019-01-14 19:09:02 -0800 (In reply to Wenson Hsieh from comment #5) > Comment on attachment 359095 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=359095&action=review > > > Source/WebCore/editing/FrameSelection.cpp:545 > > + if (compareResult == Range::NODE_BEFORE_AND_AFTER || compareResult == Range::NODE_INSIDE) { > > Doesn't this mean that we set clearRenderTreeSelection to true if either the > selection completely encompasses node, or node completely encompasses the > selection? It seems like in the latter case, we can enter this if statement > even with a caret selection. > > We should double check that we either never get here with a caret selection, > or that if we get here with a caret selection, we didn't need to clear the > render tree selection in the first place. From a hallway conversation with Ryosuke, it looks like we don't need to clear the render tree selection here because the node that encompasses the selection is going away anyways, so it doesn't look like this change should break anything. 1494483 7 359095 commit-queue 2019-01-14 19:31:47 -0800 Comment on attachment 359095 Patch Clearing flags on attachment: 359095 Committed r239971: <https://trac.webkit.org/changeset/239971> 1494484 8 commit-queue 2019-01-14 19:31:49 -0800 All reviewed patches have been landed. Closing bug. 1494828 9 saam 2019-01-15 14:48:52 -0800 Looks like a 1% speedometer2 progression! 1494829 10 saam 2019-01-15 14:49:20 -0800 (In reply to Simon Fraser (smfr) from comment #2) > I measured this as a ~1.5% improvement on Speedometer2. I see you've already measured this :) The bots agree 359095 2019-01-14 16:22:42 -0800 2019-01-14 19:31:47 -0800 Patch bug-193416-20190114162242.patch text/plain 3110 simon.fraser U3VidmVyc2lvbiBSZXZpc2lvbjogMjM5OTMwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNDk0NjgxMzE3YTI0MWIw ZDU3ODgyNWNkNjQyMzU2ZTZhYjc1NDA3Zi4uN2E2ZGE0MjI5ZTE3NGNiZjNkNmJjYzE4MmQyNjg5 Mzg5OTAyNjQ3MiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2IEBACisyMDE5LTAxLTE0ICBTaW1v biBGcmFzZXIgIDxzaW1vbi5mcmFzZXJAYXBwbGUuY29tPgorCisgICAgICAgIE9ubHkgcnVuIHRo ZSBub2RlIGNvbXBhcmlzb24gY29kZSBpbiBGcmFtZVNlbGVjdGlvbjo6cmVzcG9uZFRvTm9kZU1v ZGlmaWNhdGlvbigpIGZvciByYW5nZSBzZWxlY3Rpb25zCisgICAgICAgIGh0dHBzOi8vYnVncy53 ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xOTM0MTYKKworICAgICAgICBSZXZpZXdlZCBieSBO T0JPRFkgKE9PUFMhKS4KKworICAgICAgICBUaGUgY29kZSBpbnNpZGUgdGhlIG1fc2VsZWN0aW9u LmZpcnN0UmFuZ2UoKSBjbGF1c2UgbmVlZHMgdG8gb25seSBydW4gZm9yIG5vbi1jb2xsYXBzZWQg c2VsZWN0aW9ucywgYW5kCisgICAgICAgIGl0IHNob3dzIHVwIG9uIFNwZWVkb21ldGVyIHByb2Zp bGVzIHNvIG9wdGltaXplIHRvIG9ubHkgcnVuIHRoaXMgY29kZSBpZiB3ZSBoYXZlIGEgc2VsZWN0 aW9uIHJhbmdlLgorCisgICAgICAgICogZWRpdGluZy9GcmFtZVNlbGVjdGlvbi5jcHA6CisgICAg ICAgIChXZWJDb3JlOjpGcmFtZVNlbGVjdGlvbjo6cmVzcG9uZFRvTm9kZU1vZGlmaWNhdGlvbik6 CisKIDIwMTktMDEtMTQgIE15bGVzIEMuIE1heGZpZWxkICA8bW1heGZpZWxkQGFwcGxlLmNvbT4K IAogICAgICAgICBbV0hMU0xdIEFzc29ydGVkIGNsZWFudXAKZGlmZiAtLWdpdCBhL1NvdXJjZS9X ZWJDb3JlL2VkaXRpbmcvRnJhbWVTZWxlY3Rpb24uY3BwIGIvU291cmNlL1dlYkNvcmUvZWRpdGlu Zy9GcmFtZVNlbGVjdGlvbi5jcHAKaW5kZXggODA3M2FkN2ZjN2UwZjc1N2QyYzdjMzMzZGY4YmE4 ZGYxYTVkZTNlNy4uMTQxNmEyNTlkZTJmYzUxMTk1ZGYyYzRjNzkxOWMyOGQ2Zjk5MjdlNSAxMDA2 NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvZWRpdGluZy9GcmFtZVNlbGVjdGlvbi5jcHAKKysrIGIv U291cmNlL1dlYkNvcmUvZWRpdGluZy9GcmFtZVNlbGVjdGlvbi5jcHAKQEAgLTUzNywxNiArNTM3 LDE4IEBAIHZvaWQgRnJhbWVTZWxlY3Rpb246OnJlc3BvbmRUb05vZGVNb2RpZmljYXRpb24oTm9k ZSYgbm9kZSwgYm9vbCBiYXNlUmVtb3ZlZCwgYm9vCiAgICAgICAgICAgICBtX3NlbGVjdGlvbi5z ZXRXaXRob3V0VmFsaWRhdGlvbihtX3NlbGVjdGlvbi5zdGFydCgpLCBtX3NlbGVjdGlvbi5lbmQo KSk7CiAgICAgICAgIGVsc2UKICAgICAgICAgICAgIG1fc2VsZWN0aW9uLnNldFdpdGhvdXRWYWxp ZGF0aW9uKG1fc2VsZWN0aW9uLmVuZCgpLCBtX3NlbGVjdGlvbi5zdGFydCgpKTsKLSAgICB9IGVs c2UgaWYgKFJlZlB0cjxSYW5nZT4gcmFuZ2UgPSBtX3NlbGVjdGlvbi5maXJzdFJhbmdlKCkpIHsK LSAgICAgICAgYXV0byBjb21wYXJlTm9kZVJlc3VsdCA9IHJhbmdlLT5jb21wYXJlTm9kZShub2Rl KTsKLSAgICAgICAgaWYgKCFjb21wYXJlTm9kZVJlc3VsdC5oYXNFeGNlcHRpb24oKSkgewotICAg ICAgICAgICAgYXV0byBjb21wYXJlUmVzdWx0ID0gY29tcGFyZU5vZGVSZXN1bHQucmVsZWFzZVJl dHVyblZhbHVlKCk7Ci0gICAgICAgICAgICBpZiAoY29tcGFyZVJlc3VsdCA9PSBSYW5nZTo6Tk9E RV9CRUZPUkVfQU5EX0FGVEVSIHx8IGNvbXBhcmVSZXN1bHQgPT0gUmFuZ2U6Ok5PREVfSU5TSURF KSB7Ci0gICAgICAgICAgICAgICAgLy8gSWYgd2UgZGlkIG5vdGhpbmcgaGVyZSwgd2hlbiB0aGlz IG5vZGUncyByZW5kZXJlciB3YXMgZGVzdHJveWVkLCB0aGUgcmVjdCB0aGF0IGl0IAotICAgICAg ICAgICAgICAgIC8vIG9jY3VwaWVkIHdvdWxkIGJlIGludmFsaWRhdGVkLCBidXQsIHNlbGVjdGlv biBnYXBzIHRoYXQgY2hhbmdlIGFzIGEgcmVzdWx0IG9mIAotICAgICAgICAgICAgICAgIC8vIHRo ZSByZW1vdmFsIHdvdWxkbid0IGJlIGludmFsaWRhdGVkLgotICAgICAgICAgICAgICAgIC8vIEZJ WE1FOiBEb24ndCBkbyBzbyBtdWNoIHVubmVjZXNzYXJ5IGludmFsaWRhdGlvbi4KLSAgICAgICAg ICAgICAgICBjbGVhclJlbmRlclRyZWVTZWxlY3Rpb24gPSB0cnVlOworICAgIH0gZWxzZSBpZiAo aXNSYW5nZSgpKSB7CisgICAgICAgIGlmIChSZWZQdHI8UmFuZ2U+IHJhbmdlID0gbV9zZWxlY3Rp b24uZmlyc3RSYW5nZSgpKSB7CisgICAgICAgICAgICBhdXRvIGNvbXBhcmVOb2RlUmVzdWx0ID0g cmFuZ2UtPmNvbXBhcmVOb2RlKG5vZGUpOworICAgICAgICAgICAgaWYgKCFjb21wYXJlTm9kZVJl c3VsdC5oYXNFeGNlcHRpb24oKSkgeworICAgICAgICAgICAgICAgIGF1dG8gY29tcGFyZVJlc3Vs dCA9IGNvbXBhcmVOb2RlUmVzdWx0LnJlbGVhc2VSZXR1cm5WYWx1ZSgpOworICAgICAgICAgICAg ICAgIGlmIChjb21wYXJlUmVzdWx0ID09IFJhbmdlOjpOT0RFX0JFRk9SRV9BTkRfQUZURVIgfHwg Y29tcGFyZVJlc3VsdCA9PSBSYW5nZTo6Tk9ERV9JTlNJREUpIHsKKyAgICAgICAgICAgICAgICAg ICAgLy8gSWYgd2UgZGlkIG5vdGhpbmcgaGVyZSwgd2hlbiB0aGlzIG5vZGUncyByZW5kZXJlciB3 YXMgZGVzdHJveWVkLCB0aGUgcmVjdCB0aGF0IGl0CisgICAgICAgICAgICAgICAgICAgIC8vIG9j Y3VwaWVkIHdvdWxkIGJlIGludmFsaWRhdGVkLCBidXQsIHNlbGVjdGlvbiBnYXBzIHRoYXQgY2hh bmdlIGFzIGEgcmVzdWx0IG9mCisgICAgICAgICAgICAgICAgICAgIC8vIHRoZSByZW1vdmFsIHdv dWxkbid0IGJlIGludmFsaWRhdGVkLgorICAgICAgICAgICAgICAgICAgICAvLyBGSVhNRTogRG9u J3QgZG8gc28gbXVjaCB1bm5lY2Vzc2FyeSBpbnZhbGlkYXRpb24uCisgICAgICAgICAgICAgICAg ICAgIGNsZWFyUmVuZGVyVHJlZVNlbGVjdGlvbiA9IHRydWU7CisgICAgICAgICAgICAgICAgfQog ICAgICAgICAgICAgfQogICAgICAgICB9CiAgICAgfQo=