Bug 66656 – Web Inspector: Prepare utilities.js for compilation

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

[Pavel Feldman]

Do not use "this" within non-prototypes, get rid of WebInspector dependencies

[Pavel Feldman]

Created attachment 104669 [details]
Patch

[Pavel Feldman]

Created attachment 104673 [details]
Patch

[Andrey Kosyakov]

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 && ...

[Pavel Feldman]

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.

[Pavel Feldman]

Created attachment 104678 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 104678 [details]
Patch

LGTM

[Tony Gentilcore]

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.

[Pavel Feldman]

Committed r93584: <http://trac.webkit.org/changeset/93584>