Summary: | Web Inspector: Minification detection produces false positives for small resources | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nikita Vasilyev <nvasilyev> | ||||||||||||||||
Component: | Web Inspector | Assignee: | Nikita Vasilyev <nvasilyev> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | bburg, commit-queue, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
Attachments: |
|
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?
*** Bug 153726 has been marked as a duplicate of this bug. *** Created attachment 287032 [details]
Patch
Comment on attachment 287032 [details]
Patch
Forgot to include actual test files.
Created attachment 287033 [details]
Patch
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 :( I also made this webpage to test minification detection as I type: http://nv.github.io/webkit-inspector-bugs/minification-detection/ 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 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). Created attachment 287049 [details]
Patch
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. Created attachment 287139 [details]
Patch
(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 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. Created attachment 287671 [details]
Patch
(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 on attachment 287671 [details] Patch Clearing flags on attachment: 287671 Committed r205314: <http://trac.webkit.org/changeset/205314> All reviewed patches have been landed. Closing bug. *** Bug 150947 has been marked as a duplicate of this bug. *** |
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.