Bug 51588 - Web Inspector: implement script beautifier prototype
Summary: Web Inspector: implement script beautifier prototype
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: 2010-12-24 02:07 PST by Pavel Podivilov
Modified: 2011-01-18 02:16 PST (History)
10 users (show)

See Also:


Attachments
Patch. (15.74 KB, patch)
2010-12-24 02:08 PST, Pavel Podivilov
pfeldman: review-
Details | Formatted Diff | Diff
Patch. (26.08 KB, patch)
2010-12-31 05:00 PST, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Patch. (26.41 KB, patch)
2011-01-11 05:25 PST, Pavel Podivilov
pfeldman: 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 2010-12-24 02:07:37 PST
Implement SourceFrame.formatSource method with fake formatter (s/;/;\n/). Display breakpoints and execution line according to mapping between original script and formatted script.
Comment 1 Pavel Podivilov 2010-12-24 02:08:27 PST
Created attachment 77408 [details]
Patch.
Comment 2 Pavel Podivilov 2010-12-24 02:10:43 PST
Comment on attachment 77408 [details]
Patch.

Format script feature is hidden behind Preferences.debugMode.
Comment 3 Alexander Pavlov (apavlov) 2010-12-24 02:13:50 PST
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?
Comment 4 Pavel Podivilov 2010-12-24 02:34:24 PST
(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.
Comment 5 Pavel Feldman 2010-12-30 06:14:01 PST
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.
Comment 6 Pavel Podivilov 2010-12-31 05:00:22 PST
Created attachment 77715 [details]
Patch.

Formatting and line number conversions extracted to ScriptFormatter class.
Comment 7 Joseph Pecoraro 2011-01-10 23:16:42 PST
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.
Comment 8 Pavel Feldman 2011-01-11 00:09:12 PST
> 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.
Comment 9 Pavel Podivilov 2011-01-11 05:25:17 PST
Created attachment 78519 [details]
Patch.
Comment 10 Pavel Podivilov 2011-01-11 05:26:07 PST
(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.
Comment 11 Pavel Podivilov 2011-01-18 02:16:00 PST
Committed r76009: <http://trac.webkit.org/changeset/76009>