236050 2022-02-02 17:32:28 -0800 Web Inspector: When selecting timeline records from a coalesced record bar, select the record nearest the cursor, not the first record in the group 2022-09-01 11:26:52 -0700 1 1 1 Unclassified WebKit Web Inspector WebKit Nightly Build All All RESOLVED FIXED InRadar P2 Normal --- 1 pangle pangle ews-watchlist hi inspector-bugzilla-changes webkit-bug-importer oldest_to_newest 1836725 0 pangle 2022-02-02 17:32:28 -0800 <rdar://78629845> 1836726 1 450722 pangle 2022-02-02 17:38:35 -0800 Created attachment 450722 Patch v1.0 1838262 2 450722 hi 2022-02-07 11:35:38 -0800 Comment on attachment 450722 Patch v1.0 View in context: https://bugs.webkit.org/attachment.cgi?id=450722&action=review r=me, nice change :) tho I am slightly concerned that this behavior might be lost on developers, as there's really no visual indication of this anywhere :/ > Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.js:288 > var barDuration = barEndTime - barStartTime; NIT: Can we replace this local variable with `this._cachedBarDuration` so we don't have a duplicate? > Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.js:429 > + if (!this._delegate?.timelineRecordBarClicked || !this._cachedBarDuration) NIT: I'd separate this into two `if` so that the `_delegate` logic isn't smushed up with some other internal state logic > Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.js:433 > + if (this._records.length === 1) > + this._delegate.timelineRecordBarClicked(this._records[0]); Is there a missing `return` here? > Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.js:443 > + this._delegate.timelineRecordBarClicked(record); > + return; NIT: Instead of having another `return`, you could `closestReecord = record;` and then just `break`; so there's only one path to exit this function (i.e. it runs to completion) instead of two. > Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.js:447 > + if (timeBetweenRecordAndTargetTime < closestRecordTimeDelta) { NIT: While this approach does look like it'd work, I think it may be more work than actually needed. Can we instead just find the closest record before and after the `targetRecordTime` and return whichever of those two is closer? As an example, if a combined record bar has four evenly spaced (sub)records and the developer clicks in the middle, there's really no reason to even consider the first and last (sub)records since there's another (sub)record after and before each respectively. At the very least you could `break` out of this loop once you've reached the first (sub)record after the `targetRecordTime`. 1895387 3 pangle 2022-08-31 18:47:47 -0700 Pull request: https://github.com/WebKit/WebKit/pull/3900 1895549 4 ews-feeder 2022-09-01 11:26:50 -0700 Committed 254056@main (e9a80fbb0fb7): <https://commits.webkit.org/254056@main> Reviewed commits have been landed. Closing PR #3900 and removing active labels. 450722 2022-02-02 17:38:35 -0800 2022-02-07 11:35:38 -0800 Patch v1.0 bug-236050-20220202173834.patch text/plain 4930 pangle U3VidmVyc2lvbiBSZXZpc2lvbjogMjg4OTk3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViSW5zcGVj dG9yVUkvQ2hhbmdlTG9nIGIvU291cmNlL1dlYkluc3BlY3RvclVJL0NoYW5nZUxvZwppbmRleCA0 ZmY5NjUwY2I1NWZkYmJmYWIwOGIwYjIyOTMwOWRkNzJkMWVmZWMxLi41YWJlZTdhODc5MTAwZTNj MTcyZmNiYjgzOGI4YzNlZWVmMTRmNTdmIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViSW5zcGVjdG9y VUkvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwyMyBAQAorMjAyMi0wMi0wMiAgUGF0cmljayBBbmdsZSAgPHBhbmdsZUBhcHBsZS5jb20+ CisKKyAgICAgICAgV2ViIEluc3BlY3RvcjogV2hlbiBzZWxlY3RpbmcgdGltZWxpbmUgcmVjb3Jk cyBmcm9tIGEgY29hbGVzY2VkIHJlY29yZCBiYXIsIHNlbGVjdCB0aGUgcmVjb3JkIG5lYXJlc3Qg dGhlIGN1cnNvciwgbm90IHRoZSBmaXJzdCByZWNvcmQgaW4gdGhlIGdyb3VwCisgICAgICAgIGh0 dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMzYwNTAKKworICAgICAgICBS ZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBXZSBub3cgYXR0ZW1wdCB0byBm aW5kIHRoZSBjbG9zZXN0IHBvc3NpYmxlIHJlY29yZCB0byB0aGUgbG9jYXRpb24gb24gdGhlIHRp bWVsaW5lIHRoZSByZWNvcmQgYmFyIHdhcyBjbGlja2VkLgorICAgICAgICBJZiB3ZSBjYW4gcHJv dmlkZSBhIHJlY29yZCB0aGF0IHN0YXJ0cyBiZWZvcmUgYW5kIGVuZCBhZnRlciB0aGUgcG9pbnQg dGhhdCB3YXMgY2xpY2tlZCB3ZSByZXR1cm4gdGhhdCByZWNvcmQsCisgICAgICAgIG90aGVyd2lz ZSBmYWxsaW5nIGJhY2sgdG8gdGhlIHJlY29yZCB0aGUgZWl0aGVyIHN0YXJ0ZWQgb3IgZW5kZWQg Y2xvc2VzdCB0byB0aGUgcG9pbnQgdGhhdCB3YXMgY2xpY2tlZC4gVGhpcworICAgICAgICBzaG91 bGQgbWFrZSBpdCBlYXNpZXIgdG8ganVtcCB0byByZWNvcmRzIHRoZSByZXByZXNlbnQgYSBsYXJn ZXIgcG9ydGlvbiBvZiB0aW1lLCBsaWtlIGEgc2xvdyBwYWludCwgd2l0aG91dAorICAgICAgICBo YXZpbmcgdG8gem9vbSBhbGwgdGhlIHdheSBpbiBvbiB0aGUgdGltZWxpbmUuCisKKyAgICAgICAg KiBVc2VySW50ZXJmYWNlL1ZpZXdzL1RpbWVsaW5lT3ZlcnZpZXdHcmFwaC5qczoKKyAgICAgICAg KFdJLlRpbWVsaW5lT3ZlcnZpZXdHcmFwaC5wcm90b3R5cGUudGltZWxpbmVSZWNvcmRCYXJDbGlj a2VkKToKKyAgICAgICAgKiBVc2VySW50ZXJmYWNlL1ZpZXdzL1RpbWVsaW5lUmVjb3JkQmFyLmpz OgorICAgICAgICAoV0kuVGltZWxpbmVSZWNvcmRCYXIpOgorICAgICAgICAoV0kuVGltZWxpbmVS ZWNvcmRCYXIucHJvdG90eXBlLnJlZnJlc2gpOgorICAgICAgICAoV0kuVGltZWxpbmVSZWNvcmRC YXIucHJvdG90eXBlLl9oYW5kbGVDbGljayk6CisKIDIwMjItMDItMDIgIERvbiBPbG1zdGVhZCAg PGRvbi5vbG1zdGVhZEBzb255LmNvbT4KIAogICAgICAgICBXZWJJbnNwZWN0b3I6IEFkZCBhZGRp dGlvbmFsIGltYWdlIE1JTUUgdHlwZXMgdG8gZnJvbnRlbmQKZGlmZiAtLWdpdCBhL1NvdXJjZS9X ZWJJbnNwZWN0b3JVSS9Vc2VySW50ZXJmYWNlL1ZpZXdzL1RpbWVsaW5lT3ZlcnZpZXdHcmFwaC5q cyBiL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2VySW50ZXJmYWNlL1ZpZXdzL1RpbWVsaW5lT3Zl cnZpZXdHcmFwaC5qcwppbmRleCAzMDgwZWZmNmVkNDRjNGU3NTczYjVlMDI4NGEwYjZjZTE3NzY0 N2Q3Li5hNmE1ZTE2NjE4NzI5N2RlMWU5ZTZjYTI3YWI0M2RjNDM2MzFlMjAyIDEwMDY0NAotLS0g YS9Tb3VyY2UvV2ViSW5zcGVjdG9yVUkvVXNlckludGVyZmFjZS9WaWV3cy9UaW1lbGluZU92ZXJ2 aWV3R3JhcGguanMKKysrIGIvU291cmNlL1dlYkluc3BlY3RvclVJL1VzZXJJbnRlcmZhY2UvVmll d3MvVGltZWxpbmVPdmVydmlld0dyYXBoLmpzCkBAIC0yNTksOSArMjU5LDkgQEAgV0kuVGltZWxp bmVPdmVydmlld0dyYXBoID0gY2xhc3MgVGltZWxpbmVPdmVydmlld0dyYXBoIGV4dGVuZHMgV0ku VmlldwogCiAgICAgLy8gVGltZWxpbmVSZWNvcmRCYXIgZGVsZWdhdGUKIAotICAgIHRpbWVsaW5l UmVjb3JkQmFyQ2xpY2tlZCh0aW1lbGluZVJlY29yZEJhcikKKyAgICB0aW1lbGluZVJlY29yZEJh ckNsaWNrZWQodGltZWxpbmVSZWNvcmQpCiAgICAgewotICAgICAgICB0aGlzLnNlbGVjdGVkUmVj b3JkID0gdGltZWxpbmVSZWNvcmRCYXIucmVjb3Jkc1swXTsKKyAgICAgICAgdGhpcy5zZWxlY3Rl ZFJlY29yZCA9IHRpbWVsaW5lUmVjb3JkOwogICAgIH0KIAogICAgIC8vIFByb3RlY3RlZApkaWZm IC0tZ2l0IGEvU291cmNlL1dlYkluc3BlY3RvclVJL1VzZXJJbnRlcmZhY2UvVmlld3MvVGltZWxp bmVSZWNvcmRCYXIuanMgYi9Tb3VyY2UvV2ViSW5zcGVjdG9yVUkvVXNlckludGVyZmFjZS9WaWV3 cy9UaW1lbGluZVJlY29yZEJhci5qcwppbmRleCAxOTBhYTAzZjJkMWNkYzJiYjVhOGVjODJkZTMx MGMyN2RmMjJhZTQzLi5hMjZmZjVjMDEwODU1YzJiZjhjY2M5MWQ0OTY2ZGQ2Y2ZiMzQyZWFmIDEw MDY0NAotLS0gYS9Tb3VyY2UvV2ViSW5zcGVjdG9yVUkvVXNlckludGVyZmFjZS9WaWV3cy9UaW1l bGluZVJlY29yZEJhci5qcworKysgYi9Tb3VyY2UvV2ViSW5zcGVjdG9yVUkvVXNlckludGVyZmFj ZS9WaWV3cy9UaW1lbGluZVJlY29yZEJhci5qcwpAQCAtMzgsNiArMzgsOCBAQCBXSS5UaW1lbGlu ZVJlY29yZEJhciA9IGNsYXNzIFRpbWVsaW5lUmVjb3JkQmFyIGV4dGVuZHMgV0kuT2JqZWN0CiAK ICAgICAgICAgdGhpcy5yZW5kZXJNb2RlID0gcmVuZGVyTW9kZTsKICAgICAgICAgdGhpcy5yZWNv cmRzID0gcmVjb3JkczsKKworICAgICAgICB0aGlzLl9jYWNoZWRCYXJEdXJhdGlvbiA9IG51bGw7 CiAgICAgfQogCiAgICAgc3RhdGljIGNyZWF0ZUNvbWJpbmVkQmFycyhyZWNvcmRzLCBzZWNvbmRz UGVyUGl4ZWwsIGdyYXBoRGF0YVNvdXJjZSwgY3JlYXRlQmFyQ2FsbGJhY2spCkBAIC0yODQsNiAr Mjg2LDcgQEAgV0kuVGltZWxpbmVSZWNvcmRCYXIgPSBjbGFzcyBUaW1lbGluZVJlY29yZEJhciBl eHRlbmRzIFdJLk9iamVjdAogICAgICAgICAgICAgYmFyRW5kVGltZSA9IGdyYXBoQ3VycmVudFRp bWU7CiAKICAgICAgICAgdmFyIGJhckR1cmF0aW9uID0gYmFyRW5kVGltZSAtIGJhclN0YXJ0VGlt ZTsKKyAgICAgICAgdGhpcy5fY2FjaGVkQmFyRHVyYXRpb24gPSBiYXJEdXJhdGlvbjsKIAogICAg ICAgICB2YXIgZ3JhcGhEdXJhdGlvbiA9IGdyYXBoRW5kVGltZSAtIGdyYXBoU3RhcnRUaW1lOwog CkBAIC00MjMsOCArNDI2LDMyIEBAIFdJLlRpbWVsaW5lUmVjb3JkQmFyID0gY2xhc3MgVGltZWxp bmVSZWNvcmRCYXIgZXh0ZW5kcyBXSS5PYmplY3QKICAgICAgICAgLy8gRW5zdXJlIHRoYXQgdGhl IGNvbnRhaW5lciAiY2xpY2siIGxpc3RlbmVyIGFkZGVkIGJ5IGBXSS5UaW1lbGluZU92ZXJ2aWV3 YCBpc24ndCBjYWxsZWQuCiAgICAgICAgIGV2ZW50Ll9fdGltZWxpbmVSZWNvcmRDbGlja0V2ZW50 SGFuZGxlZCA9IHRydWU7CiAKLSAgICAgICAgaWYgKHRoaXMuX2RlbGVnYXRlICYmIHRoaXMuX2Rl bGVnYXRlLnRpbWVsaW5lUmVjb3JkQmFyQ2xpY2tlZCkKLSAgICAgICAgICAgIHRoaXMuX2RlbGVn YXRlLnRpbWVsaW5lUmVjb3JkQmFyQ2xpY2tlZCh0aGlzKTsKKyAgICAgICAgaWYgKCF0aGlzLl9k ZWxlZ2F0ZT8udGltZWxpbmVSZWNvcmRCYXJDbGlja2VkIHx8ICF0aGlzLl9jYWNoZWRCYXJEdXJh dGlvbikKKyAgICAgICAgICAgIHJldHVybjsKKworICAgICAgICBpZiAodGhpcy5fcmVjb3Jkcy5s ZW5ndGggPT09IDEpCisgICAgICAgICAgICB0aGlzLl9kZWxlZ2F0ZS50aW1lbGluZVJlY29yZEJh ckNsaWNrZWQodGhpcy5fcmVjb3Jkc1swXSk7CisKKyAgICAgICAgbGV0IHJlbGF0aXZlTW91c2VY ID0gTnVtYmVyLmNvbnN0cmFpbihldmVudC5vZmZzZXRYIC8gdGhpcy5fZWxlbWVudC5vZmZzZXRX aWR0aCwgMCwgMSk7CisgICAgICAgIGxldCB0YXJnZXRSZWNvcmRUaW1lID0gdGhpcy5fcmVjb3Jk c1swXS5zdGFydFRpbWUgKyAodGhpcy5fY2FjaGVkQmFyRHVyYXRpb24gKiByZWxhdGl2ZU1vdXNl WCk7CisKKyAgICAgICAgbGV0IGNsb3Nlc3RSZWNvcmQgPSBudWxsOworICAgICAgICBsZXQgY2xv c2VzdFJlY29yZFRpbWVEZWx0YSA9IEluZmluaXR5OworICAgICAgICBmb3IgKGxldCByZWNvcmQg b2YgdGhpcy5fcmVjb3JkcykgeworICAgICAgICAgICAgaWYgKHRhcmdldFJlY29yZFRpbWUgPj0g cmVjb3JkLnN0YXJ0VGltZSAmJiB0YXJnZXRSZWNvcmRUaW1lIDw9IHJlY29yZC5lbmRUaW1lKSB7 CisgICAgICAgICAgICAgICAgdGhpcy5fZGVsZWdhdGUudGltZWxpbmVSZWNvcmRCYXJDbGlja2Vk KHJlY29yZCk7CisgICAgICAgICAgICAgICAgcmV0dXJuOworICAgICAgICAgICAgfQorCisgICAg ICAgICAgICBsZXQgdGltZUJldHdlZW5SZWNvcmRBbmRUYXJnZXRUaW1lID0gTWF0aC5taW4oTWF0 aC5hYnMocmVjb3JkLnN0YXJ0VGltZSAtIHRhcmdldFJlY29yZFRpbWUpLCBNYXRoLmFicyhyZWNv cmQuZW5kVGltZSAtIHRhcmdldFJlY29yZFRpbWUpKTsKKyAgICAgICAgICAgIGlmICh0aW1lQmV0 d2VlblJlY29yZEFuZFRhcmdldFRpbWUgPCBjbG9zZXN0UmVjb3JkVGltZURlbHRhKSB7CisgICAg ICAgICAgICAgICAgY2xvc2VzdFJlY29yZCA9IHJlY29yZDsKKyAgICAgICAgICAgICAgICBjbG9z ZXN0UmVjb3JkVGltZURlbHRhID0gdGltZUJldHdlZW5SZWNvcmRBbmRUYXJnZXRUaW1lOworICAg ICAgICAgICAgfQorICAgICAgICB9CisKKyAgICAgICAgY29uc29sZS5hc3NlcnQoY2xvc2VzdFJl Y29yZCk7CisgICAgICAgIHRoaXMuX2RlbGVnYXRlLnRpbWVsaW5lUmVjb3JkQmFyQ2xpY2tlZChj bG9zZXN0UmVjb3JkKTsKICAgICB9CiB9OwogCg==