Bug 51588 – Web Inspector: implement script beautifier prototype

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

[Pavel Podivilov]

Implement SourceFrame.formatSource method with fake formatter (s/;/;\n/). Display breakpoints and execution line according to mapping between original script and formatted script.

[Pavel Podivilov]

Created attachment 77408 [details]
Patch.

[Pavel Podivilov]

Comment on attachment 77408 [details]
Patch.

Format script feature is hidden behind Preferences.debugMode.

[Alexander Pavlov (apavlov)]

Comment on attachment 77408 [details]
Patch.

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

> WebCore/inspector/front-end/SourceFrame.js:795
> +        this._formattedSource = this._source.replace(/;/g, ";\n");

You should definitely sync up with Peter Rybin who has been writing a similar thing for the ChromeDevTools for Java. This replacement code definitely breaks up "for(...; ...; ...)" loops into not-so-beautiful lines. Can you fix this easily?

[Pavel Podivilov]

(In reply to comment #3)
> (From update of attachment 77408 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77408&action=review
> 
> > WebCore/inspector/front-end/SourceFrame.js:795
> > +        this._formattedSource = this._source.replace(/;/g, ";\n");
> 
> You should definitely sync up with Peter Rybin who has been writing a similar thing for the ChromeDevTools for Java. This replacement code definitely breaks up "for(...; ...; ...)" loops into not-so-beautiful lines. Can you fix this easily?

This s/;/;\n/ is used as a temporary stub. We are planning to switch to uglifyjs parser in the near future.

[Pavel Feldman]

Comment on attachment 77408 [details]
Patch.

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

Looks good with comment. Please fix it first.

> WebCore/inspector/front-end/SourceFrame.js:793
> +    formatSource: function()

It is worth extracting SourceFrameFromatter class that would be used by the SourceFrame for better code clarity.

[Pavel Podivilov]

Created attachment 77715 [details]
Patch.

Formatting and line number conversions extracted to ScriptFormatter class.

[Joseph Pecoraro]

Comment on attachment 77715 [details]
Patch.

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

> WebCore/ChangeLog:10
> +        * inspector/front-end/ScriptFormatter.js: Added.

I find it helpful when someone add comments in the ChangeLog; at the very
least for new files.

> WebCore/inspector/front-end/ScriptFormatter.js:92
> +        var re = new RegExp(/[\$\.\w]+|{|}|;/g);

Nit: (performance) I don't see a need for "new RegExp" here. Also, you already have the
regex literal, and regex literals perform better then new dynamic objects!

> WebCore/inspector/front-end/ScriptFormatter.js:93
> +        while(true) {

Nit: (style) This should be "while (true) {". You're missing a space.

> WebCore/inspector/front-end/ScriptsPanel.js:69
> +    this.formatButton.title = WebInspector.UIString("Format script.");

This looks like a new UIString. Please add it to localizedStrings.js. Note, because you use
git and localizedStrings.js is currently a "binary file" this unfortunately means the patch
can't be applied via the commit-queue and would probably need to be landed manually.
To create a git patch, for bugzilla, with the binary diff you could use: `git diff --binary`.
You may already have known this.

[Pavel Feldman]

> This looks like a new UIString. Please add it to localizedStrings.js. Note, because you use
> git and localizedStrings.js is currently a "binary file" this unfortunately means the patch
> can't be applied via the commit-queue and would probably need to be landed manually.
> To create a git patch, for bugzilla, with the binary diff you could use: `git diff --binary`.
> You may already have known this.

localizedStrings.js is a pain. Is it used in Safari only? Could we generate it in the build script? Or at least make it utf-8? We do have a number of JavaScript parsers / greps that might handle the task.

[Pavel Podivilov]

Created attachment 78519 [details]
Patch.

[Pavel Podivilov]

(In reply to comment #7)
> (From update of attachment 77715 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77715&action=review
> 
> > WebCore/ChangeLog:10
> > +        * inspector/front-end/ScriptFormatter.js: Added.
> 
> I find it helpful when someone add comments in the ChangeLog; at the very
> least for new files.
> 
> > WebCore/inspector/front-end/ScriptFormatter.js:92
> > +        var re = new RegExp(/[\$\.\w]+|{|}|;/g);
> 
> Nit: (performance) I don't see a need for "new RegExp" here. Also, you already have the
> regex literal, and regex literals perform better then new dynamic objects!
> 
> > WebCore/inspector/front-end/ScriptFormatter.js:93
> > +        while(true) {
> 
> Nit: (style) This should be "while (true) {". You're missing a space.
> 
> > WebCore/inspector/front-end/ScriptsPanel.js:69
> > +    this.formatButton.title = WebInspector.UIString("Format script.");
> 

Done.

> This looks like a new UIString. Please add it to localizedStrings.js. Note, because you use
> git and localizedStrings.js is currently a "binary file" this unfortunately means the patch
> can't be applied via the commit-queue and would probably need to be landed manually.
> To create a git patch, for bugzilla, with the binary diff you could use: `git diff --binary`.
> You may already have known this.

Bugzilla doesn't display binary diffs and binary patches cannot be merged. That's why I prefer to update localizedStrings.js just before committing.

[Pavel Podivilov]

Committed r76009: <http://trac.webkit.org/changeset/76009>