Bug 52706 – Web Inspector: implement beautification of scripts inlined in html documents

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

[Pavel Podivilov]

Web Inspector: implement beautification of scripts inlined in html documents

[Pavel Podivilov]

Created attachment 79404 [details]
Patch.

[Yury Semikhatsky]

Comment on attachment 79404 [details]
Patch.

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

> Source/WebCore/ChangeLog:5
> +        Web Inspector: implement beautification of scripts inlined in html documents.

Could you elaborate on what kind of beautification this change provides?

> Source/WebCore/inspector/front-end/SourceFrame.js:308
> +        var textViewerLineNumber = this._model.originalLocationToFormattedLineNumber(this._executionLine - 1, 0);

What is this -1 for? Can you wrap this decrement in a method with a descriptive name?

> Source/WebCore/inspector/front-end/SourceFrameModel.js:31
> +WebInspector.SourceFrameModel = function(content, scripts)

I'd rather rename this class since it has nothing specific to SourceFrame. I'd rather call it SourcePositionMap or something that would better reflect it's intent.

> Source/WebCore/inspector/front-end/SourceFrameModel.js:38
> +    format: function(callback)

This method(and other ones related to script formatting) should be a part of ScriptFormatter and the SourceFrameModel should become an immutable object created by the formatter. This way we could use original script and identity mapping for not formatted scripts and replace them in case we apply some formatting rules.

> Source/WebCore/inspector/front-end/SourceFrameModel.js:121
> +        if (this.__scriptLocations)

Is there a reason for making this getter lazy?

> Source/WebCore/inspector/front-end/SourceFrameModel.js:122
> +            return this.__scriptLocations;

We don't use double underscore prefix in inspector front-end.

> Source/WebCore/inspector/front-end/SourceFrameModel.js:127
> +            this.__scriptLocations.push({ offset: offset, length: script.length });

__scriptLocations stores ranges not locations, we need to rename it

> Source/WebCore/inspector/front-end/SourceFrameModel.js:135
> +        if (!this._formatted)

Let's break this and the following methods with the same check into two classes: one will represent identity mapping and will be used for not formatted scripts and another one will be created by ScriptFormatter and will do all the position translation.

> Source/WebCore/inspector/front-end/SourceFrameModel.js:178
> +        if (this.__lineEndings)

Please don't use __ in inspector code, you could give the variable a more specific name instead if you want to avoid name conflicts.

> Source/WebCore/inspector/front-end/SourceFrameModel.js:187
> +        if (this.__formattedLineEndings)

I don't see why we need this getter to be lazy.

[Pavel Podivilov]

Created attachment 80637 [details]
Patch.

[Pavel Podivilov]

Comment on attachment 80637 [details]
Patch.

All comments addressed.

[Yury Semikhatsky]

Comment on attachment 80637 [details]
Patch.

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

> Source/WebCore/inspector/front-end/ScriptFormatter.js:96
> +        var text = "";

It might be more efficient to put the text chunks into an array first and call .join at the end.

> Source/WebCore/inspector/front-end/ScriptFormatter.js:125
> +        worker.onmessage = messageHandler.bind(this);

You can call callback.bind instead.

> Source/WebCore/inspector/front-end/SourceFrame.js:903
> +        if (!this._formattedContent)

Why the SourceFrame discriminate between formatted and not formatted content? all specific can be easily hidden behind the content interface and you wouldn't have these two methods at all.

[Yury Semikhatsky]

Comment on attachment 80637 [details]
Patch.

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

> Source/WebCore/inspector/front-end/SourceFrameContent.js:34
> +        text = text.replace(/\r\n/g, "\n");

This will make positions in the text differ from those in the original text.

[Pavel Podivilov]

(In reply to comment #5)
> (From update of attachment 80637 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80637&action=review
> 
> > Source/WebCore/inspector/front-end/ScriptFormatter.js:96
> > +        var text = "";
> 
> It might be more efficient to put the text chunks into an array first and call .join at the end.
> 

I think we should use "+" operator here, since it is more efficient and readable way to concatenate strings comparing to the .join method (at least for v8).

> > Source/WebCore/inspector/front-end/ScriptFormatter.js:125
> > +        worker.onmessage = messageHandler.bind(this);
> 
> You can call callback.bind instead.

messageHandler gets properties from event.data, this can't be done with callback.bind.

> 
> > Source/WebCore/inspector/front-end/SourceFrame.js:903
> > +        if (!this._formattedContent)
> 
> Why the SourceFrame discriminate between formatted and not formatted content? all specific can be easily hidden behind the content interface and you wouldn't have these two methods at all.

I think that a method named "originalLocationToTextViewerLineNumber" or "scriptLocationForFormattedLineNumber" would look strange as a part of SourceFrameContent interface with not formatted implementation and formatted implementation. Also creating new hierarchy just to hide contents of two tiny methods doesn't seem justified.

[Pavel Podivilov]

(In reply to comment #6)
> (From update of attachment 80637 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80637&action=review
> 
> > Source/WebCore/inspector/front-end/SourceFrameContent.js:34
> > +        text = text.replace(/\r\n/g, "\n");
> 
> This will make positions in the text differ from those in the original text.

WebKit lexer normalizes line endings (see http://trac.webkit.org/changeset/60790). Scripts are passed to VM with normalized line endings. However, requestContent returns content as is, with original line endings. That's why we have to normalize content here.

[Yury Semikhatsky]

(In reply to comment #8)
> (In reply to comment #6)
> > (From update of attachment 80637 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=80637&action=review
> > 
> > > Source/WebCore/inspector/front-end/SourceFrameContent.js:34
> > > +        text = text.replace(/\r\n/g, "\n");
> > 
> > This will make positions in the text differ from those in the original text.
> 
> WebKit lexer normalizes line endings (see http://trac.webkit.org/changeset/60790). Scripts are passed to VM with normalized line endings. However, requestContent returns content as is, with original line endings. That's why we have to normalize content here.

It deserves e a detailed comment in the code.

[Pavel Podivilov]

Created attachment 81200 [details]
Rebase.

[Pavel Podivilov]

> > WebKit lexer normalizes line endings (see http://trac.webkit.org/changeset/60790). Scripts are passed to VM with normalized line endings. However, requestContent returns content as is, with original line endings. That's why we have to normalize content here.
> 
> It deserves e a detailed comment in the code.

Done.

[Pavel Podivilov]

Committed r77810: <http://trac.webkit.org/changeset/77810>