Bug 161159 - Web Inspector: Minification detection produces false positives for small resources
Summary: Web Inspector: Minification detection produces false positives for small reso...
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: Nikita Vasilyev
URL:
Keywords: InRadar
: 150947 153726 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-08-24 13:33 PDT by Nikita Vasilyev
Modified: 2016-09-10 13:08 PDT (History)
7 users (show)

See Also:


Attachments
[HTML] Reduction (274 bytes, text/html)
2016-08-24 13:33 PDT, Nikita Vasilyev
no flags Details
WIP (1.96 KB, patch)
2016-08-24 16:55 PDT, Nikita Vasilyev
nvasilyev: review-
nvasilyev: commit-queue-
Details | Formatted Diff | Diff
Patch (4.58 KB, patch)
2016-08-25 15:29 PDT, Nikita Vasilyev
nvasilyev: review-
nvasilyev: commit-queue-
Details | Formatted Diff | Diff
Patch (12.23 KB, patch)
2016-08-25 15:40 PDT, Nikita Vasilyev
bburg: review-
bburg: commit-queue-
Details | Formatted Diff | Diff
Patch (12.48 KB, patch)
2016-08-25 17:18 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Patch (12.47 KB, patch)
2016-08-26 13:33 PDT, Nikita Vasilyev
bburg: review+
bburg: commit-queue-
Details | Formatted Diff | Diff
Patch (12.45 KB, patch)
2016-09-01 13:45 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2016-08-24 13:33:06 PDT
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.
Comment 1 Radar WebKit Bug Importer 2016-08-24 13:33:36 PDT
<rdar://problem/27995306>
Comment 2 Nikita Vasilyev 2016-08-24 16:55:30 PDT
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?
Comment 3 Nikita Vasilyev 2016-08-24 22:44:18 PDT
*** Bug 153726 has been marked as a duplicate of this bug. ***
Comment 4 Nikita Vasilyev 2016-08-25 15:29:38 PDT
Created attachment 287032 [details]
Patch
Comment 5 Nikita Vasilyev 2016-08-25 15:35:57 PDT
Comment on attachment 287032 [details]
Patch

Forgot to include actual test files.
Comment 6 Nikita Vasilyev 2016-08-25 15:40:14 PDT
Created attachment 287033 [details]
Patch
Comment 7 Nikita Vasilyev 2016-08-25 15:41:26 PDT
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 :(
Comment 8 Nikita Vasilyev 2016-08-25 15:45:39 PDT
I also made this webpage to test minification detection as I type:
http://nv.github.io/webkit-inspector-bugs/minification-detection/
Comment 9 BJ Burg 2016-08-25 16:40:21 PDT
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?
Comment 10 Nikita Vasilyev 2016-08-25 17:14:35 PDT
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).
Comment 11 Nikita Vasilyev 2016-08-25 17:18:19 PDT
Created attachment 287049 [details]
Patch
Comment 12 Joseph Pecoraro 2016-08-26 11:47:20 PDT
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.
Comment 13 Nikita Vasilyev 2016-08-26 13:33:04 PDT
Created attachment 287139 [details]
Patch
Comment 14 Nikita Vasilyev 2016-08-26 13:37:32 PDT
(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.
Comment 15 BJ Burg 2016-09-01 11:04:31 PDT
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.
Comment 16 Nikita Vasilyev 2016-09-01 13:45:49 PDT
Created attachment 287671 [details]
Patch
Comment 17 Nikita Vasilyev 2016-09-01 13:51:28 PDT
(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.
Comment 18 WebKit Commit Bot 2016-09-01 14:06:42 PDT
Comment on attachment 287671 [details]
Patch

Clearing flags on attachment: 287671

Committed r205314: <http://trac.webkit.org/changeset/205314>
Comment 19 WebKit Commit Bot 2016-09-01 14:06:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Nikita Vasilyev 2016-09-10 13:08:49 PDT
*** Bug 150947 has been marked as a duplicate of this bug. ***