Bug 52706 - Web Inspector: implement beautification of scripts inlined in html documents
Summary: Web Inspector: implement beautification of scripts inlined in html documents
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Pavel Podivilov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-19 02:12 PST by Pavel Podivilov
Modified: 2011-02-07 07:16 PST (History)
11 users (show)

See Also:


Attachments
Patch. (23.74 KB, patch)
2011-01-19 02:12 PST, Pavel Podivilov
yurys: review-
Details | Formatted Diff | Diff
Patch. (32.71 KB, patch)
2011-01-31 06:35 PST, Pavel Podivilov
yurys: review-
Details | Formatted Diff | Diff
Rebase. (30.12 KB, patch)
2011-02-04 02:19 PST, Pavel Podivilov
yurys: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Podivilov 2011-01-19 02:12:00 PST
Web Inspector: implement beautification of scripts inlined in html documents
Comment 1 Pavel Podivilov 2011-01-19 02:12:41 PST
Created attachment 79404 [details]
Patch.
Comment 2 Yury Semikhatsky 2011-01-27 08:17:55 PST
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.
Comment 3 Pavel Podivilov 2011-01-31 06:35:45 PST
Created attachment 80637 [details]
Patch.
Comment 4 Pavel Podivilov 2011-01-31 06:36:35 PST
Comment on attachment 80637 [details]
Patch.

All comments addressed.
Comment 5 Yury Semikhatsky 2011-02-02 06:33:32 PST
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.
Comment 6 Yury Semikhatsky 2011-02-02 06:46:53 PST
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.
Comment 7 Pavel Podivilov 2011-02-03 03:50:49 PST
(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.
Comment 8 Pavel Podivilov 2011-02-03 03:55:29 PST
(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.
Comment 9 Yury Semikhatsky 2011-02-03 04:41:40 PST
(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.
Comment 10 Pavel Podivilov 2011-02-04 02:19:39 PST
Created attachment 81200 [details]
Rebase.
Comment 11 Pavel Podivilov 2011-02-04 02:19:52 PST
> > 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.
Comment 12 Pavel Podivilov 2011-02-07 07:16:08 PST
Committed r77810: <http://trac.webkit.org/changeset/77810>