Summary: | Web Inspector: implement script beautifier prototype | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Pavel Podivilov <podivilov> | ||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Pavel Podivilov <podivilov> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | apavlov, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, yurys | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Pavel Podivilov
2010-12-24 02:07:37 PST
Created attachment 77408 [details]
Patch.
Comment on attachment 77408 [details]
Patch.
Format script feature is hidden behind Preferences.debugMode.
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? (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 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. Created attachment 77715 [details]
Patch.
Formatting and line number conversions extracted to ScriptFormatter class.
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. > 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.
Created attachment 78519 [details]
Patch.
(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. Committed r76009: <http://trac.webkit.org/changeset/76009> |