Bug 81662 – Web Inspector: REGRESSION Detached DOM Nodes are not highlighted

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Ilya Tikhonovsky]

%subj%

[Ilya Tikhonovsky]

Created attachment 132822 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 132822 [details]
Patch

Can we test this?

[Yury Semikhatsky]

Comment on attachment 132822 [details]
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

[Ilya Tikhonovsky]

Committed r111392: <http://trac.webkit.org/changeset/111392>

[Darin Adler]

(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?

[Yury Semikhatsky]

Comment on attachment 132822 [details]
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.

[Ilya Tikhonovsky]

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;
}

[Ilya Tikhonovsky]

looks like I have to change this code to RegExp version
http://jsperf.com/careful-with-that-indexof-eugene