81662 2012-03-20 07:10:13 -0700 Web Inspector: REGRESSION Detached DOM Nodes are not highlighted 2012-03-21 09:48:32 -0700 1 1 1 Unclassified WebKit Web Inspector (Deprecated) 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 78411 0 loislo loislo apavlov barraclough bweinstein darin ggaren joepeck keishi loislo pfeldman pmuellr rik timothy yurys oldest_to_newest 583034 0 loislo 2012-03-20 07:10:13 -0700 %subj% 583038 1 132822 loislo 2012-03-20 07:14:27 -0700 Created attachment 132822 Patch 583042 2 132822 yurys 2012-03-20 07:17:50 -0700 Comment on attachment 132822 Patch Can we test this? 583043 3 132822 yurys 2012-03-20 07:18:28 -0700 Comment on attachment 132822 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132822&action=review > Source/WebCore/inspector/front-end/HeapSnapshot.js:646 > + return this.className.substr(0, length) === detachedDOMTree; I'd rather use indexOf(detachedDOMTree) === 0 583059 4 loislo 2012-03-20 07:39:01 -0700 Committed r111392: <http://trac.webkit.org/changeset/111392> 583607 5 darin 2012-03-20 16:36:57 -0700 (In reply to comment #3) > (From update of attachment 132822 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=132822&action=review > > > Source/WebCore/inspector/front-end/HeapSnapshot.js:646 > > + return this.className.substr(0, length) === detachedDOMTree; > > I'd rather use indexOf(detachedDOMTree) === 0 Why? That does a search for the string, not a prefix check. Is indexOf == 0 a good design pattern? Maybe the JavaScript engines special-case it? 583848 6 132822 yurys 2012-03-20 23:38:11 -0700 Comment on attachment 132822 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132822&action=review >>> Source/WebCore/inspector/front-end/HeapSnapshot.js:646 >>> + return this.className.substr(0, length) === detachedDOMTree; >> >> I'd rather use indexOf(detachedDOMTree) === 0 > > Why? That does a search for the string, not a prefix check. > > Is indexOf == 0 a good design pattern? Maybe the JavaScript engines special-case it? indexOf === 0 is just a common way for string prefix check in the inspector front-end code. Only heap profiler uses this substr approach. My point was that we should be consistent here. As for the performance, they should be equally fast given that we don't check for a prefix in strings much longer than the prefix itself. 583885 7 loislo 2012-03-21 01:21:35 -0700 Looks like we have no special-case for this. I've made an investigation and got the next results. aString = "012345678900123456789001234567890012345678900123456789001234567890012345678900123456789001234567890012345678900123456789001234567890" function test() { for (var j = 0; j < 1000000; ++j) doCompare(aString); } // time: V8:641ms, JSC:799 // produces garbage function doCompare(name) { return aString.substr(0, 6) === "Window"; } // time: V8:643ms, JSC:799ms // produces garbage function doCompare(name) { return aString.slice(0, 6) === "Window"; } // time: V8:928ms, JSC:866ms // no garbage function doCompare(name) { return aString.indexOf("Window") === 0; } // time: V8:565ms, JSC:890ms // no garbage function doCompare(name) { const t = "Window"; const l = t.length; for (var i = 0; i < l; ++i) if (name[i] !== t[i]) return false; return true; } 584240 8 loislo 2012-03-21 09:48:32 -0700 looks like I have to change this code to RegExp version http://jsperf.com/careful-with-that-indexof-eugene 132822 2012-03-20 07:14:27 -0700 2012-03-20 23:38:11 -0700 Patch bug-81662-20120320151425.patch text/plain 1411 loislo U3VidmVyc2lvbiBSZXZpc2lvbjogMTExMzg5CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYTdjMWU0YThhYWI5Y2Qw YzZhZDljYWZlOTJmY2M1MjlmMDZjODBkMS4uM2I5MTZmZWU3MWRkYjVjNzRiZWE4YWQwMDI4NTZj ODFkNTE5ZGNjYSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDEzIEBACisyMDEyLTAzLTIwICBJbHlh IFRpa2hvbm92c2t5ICA8bG9pc2xvQGNocm9taXVtLm9yZz4KKworICAgICAgICBXZWIgSW5zcGVj dG9yOiBSRUdSRVNTSU9OIERldGFjaGVkIERPTSBOb2RlcyBhcmUgbm90IGhpZ2hsaWdodGVkCisg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD04MTY2MgorCisg ICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogaW5zcGVjdG9y L2Zyb250LWVuZC9IZWFwU25hcHNob3QuanM6CisgICAgICAgIChXZWJJbnNwZWN0b3IuSGVhcFNu YXBzaG90Tm9kZS5wcm90b3R5cGUuZ2V0IGlzRGV0YWNoZWRET01UcmVlKToKKwogMjAxMi0wMy0x NSAgUGF2ZWwgUG9kaXZpbG92ICA8cG9kaXZpbG92QGNocm9taXVtLm9yZz4KIAogICAgICAgICBX ZWIgSW5zcGVjdG9yOiBzdXBwb3J0IGlubGluZSBzb3VyY2UgbWFwcy4KZGlmZiAtLWdpdCBhL1Nv dXJjZS9XZWJDb3JlL2luc3BlY3Rvci9mcm9udC1lbmQvSGVhcFNuYXBzaG90LmpzIGIvU291cmNl L1dlYkNvcmUvaW5zcGVjdG9yL2Zyb250LWVuZC9IZWFwU25hcHNob3QuanMKaW5kZXggNjk4MDNh MmE1OGFjMjU4NmYwNzc0OTY3NTc0NjNhOTE5NTUzMWUwNy4uYTlhMmQ2NzkxZmJiOWFkYTFiNzVj MTY3NWU3OTVhNGExNDFjNjFhZCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvaW5zcGVjdG9y L2Zyb250LWVuZC9IZWFwU25hcHNob3QuanMKKysrIGIvU291cmNlL1dlYkNvcmUvaW5zcGVjdG9y L2Zyb250LWVuZC9IZWFwU25hcHNob3QuanMKQEAgLTY0MSw3ICs2NDEsOSBAQCBXZWJJbnNwZWN0 b3IuSGVhcFNuYXBzaG90Tm9kZS5wcm90b3R5cGUgPSB7CiAKICAgICBnZXQgaXNEZXRhY2hlZERP TVRyZWUoKQogICAgIHsKLSAgICAgICAgcmV0dXJuIHRoaXMuY2xhc3NOYW1lID09PSAiRGV0YWNo ZWQgRE9NIHRyZWUiOworICAgICAgICBjb25zdCBkZXRhY2hlZERPTVRyZWUgPSAiRGV0YWNoZWQg RE9NIHRyZWUiOworICAgICAgICBjb25zdCBsZW5ndGggPSBkZXRhY2hlZERPTVRyZWUubGVuZ3Ro OworICAgICAgICByZXR1cm4gdGhpcy5jbGFzc05hbWUuc3Vic3RyKDAsIGxlbmd0aCkgPT09IGRl dGFjaGVkRE9NVHJlZTsKICAgICB9LAogCiAgICAgZ2V0IGlzUm9vdCgpCg==