Bug 66656

Summary: Web Inspector: Prepare utilities.js for compilation
Product: WebKit Reporter: Pavel Feldman <pfeldman>
Component: Web Inspector (Deprecated)Assignee: Pavel Feldman <pfeldman>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 66655    
Attachments:
Description Flags
Patch
none
Patch
none
Patch tonyg: review+

Description Pavel Feldman 2011-08-22 03:13:23 PDT
Do not use "this" within non-prototypes, get rid of WebInspector dependencies
Comment 1 Pavel Feldman 2011-08-22 06:16:54 PDT
Created attachment 104669 [details]
Patch
Comment 2 Pavel Feldman 2011-08-22 06:53:54 PDT
Created attachment 104673 [details]
Patch
Comment 3 Andrey Kosyakov 2011-08-22 07:22:56 PDT
Comment on attachment 104673 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104673&action=review

> Source/WebCore/inspector/front-end/ConsoleView.js:999
> +        formatters.s = formatters.f = formatters.i = formatters.d = valueFormatter;

Split to multiple lines or use a for?

> Source/WebCore/inspector/front-end/DOMAgent.js:249
> +        var currentNode = node.parentNode;
> +        while (currentNode) {
> +            if (this === currentNode)
> +                return true;
> +            currentNode = currentNode.parentNode;
> +        }

nit: this is better written as a for loop (I know you're just moving it around, hence just a nit)

> Source/WebCore/inspector/front-end/DOMAgent.js:255
> +        return descendant !== null && descendant.isAncestor(this);

return !!descendant && descendant.isAncestor(this);

> Source/WebCore/inspector/front-end/DOMAgent.js:264
> +        if (!this.nodeValue.length)
> +            return true;
> +        return this.nodeValue.match(/^[\s\xA0]+$/);

nit: do we need an explicit optimization here? also, why do we use match() instead of test()?
This would be more readable as /^\s*$/.test(this.nodeValue);

> Source/WebCore/inspector/front-end/ElementsPanel.js:482
> +            if (!updateBreadcrumbs && (this.selectedDOMNode() === parent || this.selectedDOMNode().isAncestor(parent)))

This seems to have slightly different semantics to the old code.

> Source/WebCore/inspector/front-end/utilities.js:481
>      while (currentNode) {
> -        if (ancestor === currentNode)
> +        if (this === currentNode)
>              return true;
>          currentNode = currentNode.parentNode;
>      }

nit: for?

> Source/WebCore/inspector/front-end/utilities.js:487
> +    return descendant !== null && descendant.isAncestor(this);

!!descendant && ...
Comment 4 Pavel Feldman 2011-08-22 07:32:36 PDT
Comment on attachment 104673 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104673&action=review

>> Source/WebCore/inspector/front-end/ConsoleView.js:999
>> +        formatters.s = formatters.f = formatters.i = formatters.d = valueFormatter;
> 
> Split to multiple lines or use a for?

Done.

>> Source/WebCore/inspector/front-end/DOMAgent.js:249
>> +        }
> 
> nit: this is better written as a for loop (I know you're just moving it around, hence just a nit)

I don't want to change anything...

>> Source/WebCore/inspector/front-end/DOMAgent.js:255
>> +        return descendant !== null && descendant.isAncestor(this);
> 
> return !!descendant && descendant.isAncestor(this);

Done.

>> Source/WebCore/inspector/front-end/utilities.js:481
>>      }
> 
> nit: for?

Did not want to change much

>> Source/WebCore/inspector/front-end/utilities.js:487
>> +    return descendant !== null && descendant.isAncestor(this);
> 
> !!descendant && ...

Done.
Comment 5 Pavel Feldman 2011-08-22 07:35:02 PDT
Created attachment 104678 [details]
Patch
Comment 6 Andrey Kosyakov 2011-08-22 07:44:25 PDT
Comment on attachment 104678 [details]
Patch

LGTM
Comment 7 Tony Gentilcore 2011-08-23 02:10:27 PDT
Comment on attachment 104678 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104678&action=review

> Source/WebCore/inspector/front-end/inspector.js:1720
> +Number.secondsToString = function(seconds, higherResolution)

Might as well fix these two methods up with some nice JS doc comments now.
Comment 8 Pavel Feldman 2011-08-23 02:23:46 PDT
Committed r93584: <http://trac.webkit.org/changeset/93584>