Bug 161159 – Web Inspector: Minification detection produces false positives for small resources

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Created attachment 286885 [details]
[HTML] Reduction

Steps:
1. Open attached HTML file.
2. Open Inspector in Resources tab.
3. Select the only available CSS resource.

Expected:
"{}" icon is black (inactive), pretty printing is turned off.

Actual:
"{}" icon is blue (active), pretty printing is turned on.

[Radar WebKit Bug Importer]

<rdar://problem/27995306>

[Nikita Vasilyev]

Created attachment 286912 [details]
WIP

Setting r- since I don't have any tests yet.

I'd like to write unit tests for _isLikelyMinified.
_isLikelyMinified is defined in a view (Views/SourceCodeTextEditor.js).
I can make this method static and move it out of a Views/SourceCodeTextEditor.js.
Should I do it, and if so, where should I move it?

[Nikita Vasilyev]

*** Bug 153726 has been marked as a duplicate of this bug. ***

[Nikita Vasilyev]

Created attachment 287032 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 287032 [details]
Patch

Forgot to include actual test files.

[Nikita Vasilyev]

Created attachment 287033 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 287033 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1410
> +    let i = 0;

I moved this out of the for loop since it's used after the loop.

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1429
> +    ratio = whiteSpaceScore / i;

I added this line. Without it, every resource with less than 500 characters was considered minified :(

[Nikita Vasilyev]

I also made this webpage to test minification detection as I type:
http://nv.github.io/webkit-inspector-bugs/minification-detection/

[Blaze Burg]

Comment on attachment 287033 [details]
Patch

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

> LayoutTests/inspector/formatting/is-resource-likely-minified.html:10
> +    let suite = InspectorTest.createAsyncSuite("isLikelyMinified");

Please call the suite "Formatting.isLikelyMinified"

> LayoutTests/inspector/formatting/is-resource-likely-minified.html:13
> +        name: "Not minified: small resource",

See comment below

> LayoutTests/inspector/formatting/is-resource-likely-minified.html:15
> +            InspectorTest.expectThat(!isResourceLikelyMinified("var x = 42;\nvar y = 24;\n"), "Not minified");

See comment below

> LayoutTests/inspector/formatting/is-resource-likely-minified.html:95
> +        name: "Not minified: jQuery",

This does not follow test case naming conventions. A better name would be JQuerySnippet.Unminified.

> LayoutTests/inspector/formatting/is-resource-likely-minified.html:102
> +    var jQueryMinifiedSourceFragment = `/*! jQuery v3.1.0 | (c) jQuery Foundation | jquery.org/license */

Nit: let

> LayoutTests/inspector/formatting/is-resource-likely-minified.html:106
> +        name: "Minified: jQuery",

This does not follow test case naming conventions. A better name would be JQuerySnippet.Minified.

> LayoutTests/inspector/formatting/is-resource-likely-minified.html:107
> +        test: function(resolve, reject) {

Use an arrow function.

> LayoutTests/inspector/formatting/is-resource-likely-minified.html:108
> +            InspectorTest.expectThat(isResourceLikelyMinified(jQueryMinifiedSourceFragment), "Minified");

The expectation string here doesn't make sense. It should be something like "jQuery snippet should be classified as minified."

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1405
> +function isResourceLikelyMinified(content)

Nit: isTextLikelyMinified

There's nothing specific to resources about this function.

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1407
> +    let whiteSpaceScore = 0;

Nit: whitespaceScore

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1432
> +}

Did you mean to change the ratio?

[Nikita Vasilyev]

Comment on attachment 287033 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1432
>> +}
> 
> Did you mean to change the ratio?

Yes, but I forgot to add a comment. The ratio thresholds were increased to accommodate
for the changes in whiteSpaceScore (a tab character is counted as 4 and a new line as 8).

[Nikita Vasilyev]

Created attachment 287049 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 287049 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1405
> +function isTextLikelyMinified(content)

I think all of my original review comments and questions still apply. I see now you are addressing the \t comment I originally raised. But I still question the extra checks between characters 500-5000:
https://bugs.webkit.org/show_bug.cgi?id=150876#c13

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1412
> +    let i = 0;
> +    let size = Math.min(5000, content.length);
> +    for (; i < size; i++) {

Why not `let i` inside the loop?

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1429
> +    ratio = whitespaceScore / i;

This should be `size` not `i`, right? We only read 5000 characters, not 5001.

[Nikita Vasilyev]

Created attachment 287139 [details]
Patch

[Nikita Vasilyev]

(In reply to comment #12)
> Comment on attachment 287049 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=287049&action=review
> 
> > Source/WebInspectorUI/UserInterface/Base/Utilities.js:1405
> > +function isTextLikelyMinified(content)
> 
> I think all of my original review comments and questions still apply. I see
> now you are addressing the \t comment I originally raised. But I still
> question the extra checks between characters 500-5000:
> https://bugs.webkit.org/show_bug.cgi?id=150876#c13

I measured how long does it take for isTextLikelyMinified to execute on unminified jQuery file. It was 0.2-0.7ms, which is very fast, so I removed `if (i >= 500) ...` block.

[Blaze Burg]

Comment on attachment 287139 [details]
Patch

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

r=me

> LayoutTests/inspector/formatting/is-text-likely-minified.html:6
> +window.onload = runTest;

We almost always prefer this to be in <body onload="runTest();"> just for consistency with 1000's of other tests.

> LayoutTests/inspector/formatting/is-text-likely-minified.html:20
> +    let jquerySourceFragment = `/*eslint-disable no-unused-vars*/

It would be nice to assign these big literals to globals in a separate file in LayoutTests/inspector/formatting/resources/. Its difficult to read the test flow.

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1408
> +    const autoFormatWhitespaceRatio = 0.2; // 20%

This comment is unnecessary.

[Nikita Vasilyev]

Created attachment 287671 [details]
Patch

[Nikita Vasilyev]

(In reply to comment #15)
> > LayoutTests/inspector/formatting/is-text-likely-minified.html:20
> > +    let jquerySourceFragment = `/*eslint-disable no-unused-vars*/
> 
> It would be nice to assign these big literals to globals in a separate file
> in LayoutTests/inspector/formatting/resources/. Its difficult to read the
> test flow.

It turned out to be more difficult as expected so I left it as is
because "test" function didn't have access to global variables.
I don't know why, possibly it's converted to a string (via .toString()) and
then evaluated.

[WebKit Commit Bot]

Comment on attachment 287671 [details]
Patch

Clearing flags on attachment: 287671

Committed r205314: <http://trac.webkit.org/changeset/205314>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Nikita Vasilyev]

*** Bug 150947 has been marked as a duplicate of this bug. ***