Bug 225009 – Web Inspector: Default Audits script are minified in release builds

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Patrick Angle]

This means that when entering Edit mode in the Audit tab all of the default audits, many meant to server as examples while writing audits, are a block of minified code instead of in their original, non-minified form.

<rdar://76778007>

[Patrick Angle]

Created attachment 426966 [details]
Patch v1.0

[Devin Rousso]

Comment on attachment 426966 [details]
Patch v1.0

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

> Source/WebInspectorUI/Scripts/combine-resources.pl:131
> +    concatenateIncludedFilesMatchingPattern($outputStylesheetName, "<link rel=\"stylesheet\" href=\"($inputDirectoryPattern)\">", defined $skipConcatenateTag ? "" : "<link rel=\"stylesheet\" href=\"$outputStylesheetName\">") if defined $outputStylesheetName;
> +    concatenateIncludedFilesMatchingPattern($outputScriptName, "<script src=\"($inputDirectoryPattern)\"><\/script>", defined $skipConcatenateTag ? "" : "<script src=\"$outputScriptName\"></script>") if defined $outputScriptName;

Why is `$skipConcatenateTag` actually needed?  Wouldn't any files we want to concatenate get skipped anyway because of `$inputDirectoryPattern`?

> Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:357
> +       '--output-script-name', 'NonMinified.js',

Does this eventually get deleted after it's appended?

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:349
> +                    new WI.AuditTestCase("level-pass", removeWhitespace(WI.DefaultAudits.levelPass), {description: WI.UIString("This is what the result of a passing test with no data looks like.")}),

we should either `console.assert(WI.DefaultAudits)` or even just bail if it's not there, just in case :)

> Source/WebInspectorUI/UserInterface/NonMinified/DefaultAudits.js:26
> +WI.DefaultAudits = {

Instead of having this all be part of the object literal, could we do something like
```
WI.DefaultAudits = {};

WI.DefaultAudits.levelPass = function() {
    return {level: "pass"};
};

...
```
and then get replace `removeWhitespace` with `.toString()`?

> Source/WebInspectorUI/UserInterface/Test.html:289
> +    <script src="NonMinified/DefaultAudits.js"></script>

Can we add a test that `NonMinified` actually works?  i.e. a test that confirms the presence of `"    "` (four spaces) inside `WI.DefaultAudits.testPass`

[Patrick Angle]

Comment on attachment 426966 [details]
Patch v1.0

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

>> Source/WebInspectorUI/Scripts/combine-resources.pl:131
>> +    concatenateIncludedFilesMatchingPattern($outputScriptName, "<script src=\"($inputDirectoryPattern)\"><\/script>", defined $skipConcatenateTag ? "" : "<script src=\"$outputScriptName\"></script>") if defined $outputScriptName;
> 
> Why is `$skipConcatenateTag` actually needed?  Wouldn't any files we want to concatenate get skipped anyway because of `$inputDirectoryPattern`?

The default pattern is ignored when an input directory is explicitly provided. This is how we gather the NonMinified sources from Main.html. Later the combined (but non minified) NonMinified.js will be appended to the end of Main.js, so we don't want to add a script tag for it here.

>> Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:357
>> +       '--output-script-name', 'NonMinified.js',
> 
> Does this eventually get deleted after it's appended?

No, but this is a derived source that doesn't end up in the target directory (e.g. this file will be in `WebKitBuild/WebInspectorUI.build/Release/WebInspectorUI.build/DerivedSources` but not in `WebKitBuild/Release/WebInspectorUI.framework`). It seems helpful for debugging the build that after a build this intermediate file is present along with the rest of the intermediate derived sources.

>> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:349
>> +                    new WI.AuditTestCase("level-pass", removeWhitespace(WI.DefaultAudits.levelPass), {description: WI.UIString("This is what the result of a passing test with no data looks like.")}),
> 
> we should either `console.assert(WI.DefaultAudits)` or even just bail if it's not there, just in case :)

👍🏻

>> Source/WebInspectorUI/UserInterface/NonMinified/DefaultAudits.js:26
>> +WI.DefaultAudits = {
> 
> Instead of having this all be part of the object literal, could we do something like
> ```
> WI.DefaultAudits = {};
> 
> WI.DefaultAudits.levelPass = function() {
>     return {level: "pass"};
> };
> 
> ...
> ```
> and then get replace `removeWhitespace` with `.toString()`?

👍🏻

>> Source/WebInspectorUI/UserInterface/Test.html:289
>> +    <script src="NonMinified/DefaultAudits.js"></script>
> 
> Can we add a test that `NonMinified` actually works?  i.e. a test that confirms the presence of `"    "` (four spaces) inside `WI.DefaultAudits.testPass`

Absolutely!

[Patrick Angle]

Comment on attachment 426966 [details]
Patch v1.0

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

>>> Source/WebInspectorUI/UserInterface/Test.html:289
>>> +    <script src="NonMinified/DefaultAudits.js"></script>
>> 
>> Can we add a test that `NonMinified` actually works?  i.e. a test that confirms the presence of `"    "` (four spaces) inside `WI.DefaultAudits.testPass`
> 
> Absolutely!

I've written a test for this that will be in the next patch I upload. After finishing the test, I realized that such a test would already pass, though, since these files are part of TestCombined.js, which is the combined, but not minified, sum of almost all the included JS in Test.html. However, the test will help to make sure we are including the new file in the `NonMinified` directory, so it still seems worth having.

[Patrick Angle]

Created attachment 427159 [details]
Patch v1.1 - Added test, review notes

[Blaze Burg]

Comment on attachment 427159 [details]
Patch v1.1 - Added test, review notes

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

r=me, this is excellent!

> Source/WebInspectorUI/ChangeLog:14
> +        important to the script. For example, the source for the default audits are visible to the user, and therefor we

Nit: therefore

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:386
> +                new WI.AuditTestCase("testMenuRoleForRequiredChildren", WI.DefaultAudits.testMenuRoleForRequiredChildren.toString(), {description: WI.UIString("Ensure that elements of role \u0022%s\u0022 and \u0022%s\u0022 have required owned elements in accordance with WAI-ARIA.").format(WI.unlocalizedString("menu"), WI.unlocalizedString("menubar")), supports: 1}),

It's rather unfortunate that we have to stringify the functions at every call site.

> LayoutTests/inspector/audit/non-minified-default-audits.html:8
> +    let suite = InspectorTest.createAsyncSuite("Audit.NonMinifiedDefaultAudits");

Nice!

[Patrick Angle]

Created attachment 427203 [details]
Patch v1.2 - Review note

[EWS]

Committed r276680 (237097@main): <https://commits.webkit.org/237097@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 427203 [details].