Bug 192946 - Web Inspector: Autoformat doesn't work on icloud.com (javascript-packed.js)
Summary: Web Inspector: Autoformat doesn't work on icloud.com (javascript-packed.js)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-12-20 12:34 PST by Joseph Pecoraro
Modified: 2018-12-20 15:17 PST (History)
4 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (6.34 KB, patch)
2018-12-20 12:46 PST, Joseph Pecoraro
hi: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2018-12-20 12:34:41 PST
Autoformat doesn't work on icloud.com (javascript-packed.js)

Steps to Reproduce:
1. Inspect iCloud.com
2. Open javascript-packed.js
  => Doesn't auto-format

Notes:
• The file starts out with: 7300+ characters that are unminified license / header doc characters. This causes our isTextLikelyMinified to fail.
Comment 1 Joseph Pecoraro 2018-12-20 12:35:24 PST
<rdar://problem/42546126>
Comment 2 Joseph Pecoraro 2018-12-20 12:46:30 PST
Created attachment 357851 [details]
[PATCH] Proposed Fix

I still think the best solution might be to auto format if any line is longer than 250 characters, or something like that. But a start/end check of 2500 characters seems reasonable. We can bring that back to 5000 if there is a counterexample.
Comment 3 Devin Rousso 2018-12-20 13:05:09 PST
Comment on attachment 357851 [details]
[PATCH] Proposed Fix

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

rs=me, nice test

> LayoutTests/inspector/formatting/is-text-likely-minified.html:111
> +    let sampleHeader = `

NIT: const.

> LayoutTests/inspector/formatting/is-text-likely-minified.html:127
> +    let longHeaderSource = sampleHeader.repeat(10) + jQueryMinifiedSourceFragment;

NIT: const.

> LayoutTests/inspector/formatting/is-text-likely-minified.html:129
> +        name: "JQuerySnippet.Minified",

Use a different name, like "JQuerySnippet.MinifiedWithHeader".

NIT: jQuery is stylized as "jQuery" not "JQuery", so we might want to follow that.

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1621
> +    for (let i = start; i < end; i++) {

NIT: ++i

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1623
>          if (char === " ")

Should we care about \r as well?
Comment 4 Joseph Pecoraro 2018-12-20 15:10:10 PST
> > Source/WebInspectorUI/UserInterface/Base/Utilities.js:1623
> >          if (char === " ")
> 
> Should we care about \r as well?

Interesting. Yes probably but it is very rare to have only \r line endings.
Comment 5 Joseph Pecoraro 2018-12-20 15:17:03 PST
https://trac.webkit.org/r239467