Bug 81662

Summary: Web Inspector: REGRESSION Detached DOM Nodes are not highlighted
Product: WebKit Reporter: Ilya Tikhonovsky <loislo>
Component: Web Inspector (Deprecated)Assignee: Ilya Tikhonovsky <loislo>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, barraclough, bweinstein, darin, ggaren, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 78411    
Attachments:
Description Flags
Patch yurys: review+

Description Ilya Tikhonovsky 2012-03-20 07:10:13 PDT
%subj%
Comment 1 Ilya Tikhonovsky 2012-03-20 07:14:27 PDT
Created attachment 132822 [details]
Patch
Comment 2 Yury Semikhatsky 2012-03-20 07:17:50 PDT
Comment on attachment 132822 [details]
Patch

Can we test this?
Comment 3 Yury Semikhatsky 2012-03-20 07:18:28 PDT
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
Comment 4 Ilya Tikhonovsky 2012-03-20 07:39:01 PDT
Committed r111392: <http://trac.webkit.org/changeset/111392>
Comment 5 Darin Adler 2012-03-20 16:36:57 PDT
(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?
Comment 6 Yury Semikhatsky 2012-03-20 23:38:11 PDT
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.
Comment 7 Ilya Tikhonovsky 2012-03-21 01:21:35 PDT
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;
}
Comment 8 Ilya Tikhonovsky 2012-03-21 09:48:32 PDT
looks like I have to change this code to RegExp version
http://jsperf.com/careful-with-that-indexof-eugene