Bug 32930 – Web Inspector: Add a simplistic audit category

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Alexander Pavlov (apavlov)]

Once the Audits panel is in, we need to see it work with a sample audit category (something like "Page performance" or "Network utilization.)

[Alexander Pavlov (apavlov)]

Created attachment 47413 [details]
[PATCH] Proposed audit categories

[Eric Seidel (no email)]

Who should review this alexander?

[Pavel Feldman]

r=me

[Pavel Feldman]

Comment on attachment 47413 [details]
[PATCH] Proposed audit categories

Looks good overall. Some nits I'd like to see fixed before you land:

> +WebInspector.AuditCategories.Page = function() {

Page -> PagePerformance
Also I am not sure you need a special namespace (AuditCategories) for that.

> +
> +WebInspector.AuditCategories.Network = function() {

Network -> NetworkUtilization

> +    WebInspector.AuditCategory.call(
> +        this, WebInspector.AuditCategories.Network.AuditCategoryName);
> +
> +    this.addRule(new WebInspector.AuditRules.GzipRule());
> +    this.addRule(new WebInspector.AuditRules.ImageDimensionsRule({ScorePerImageUse: 5}));
> +    this.addRule(new WebInspector.AuditRules.CookieSizeRule({MinBytesThreshold: 400, MaxBytesThreshold: 1000}));
> +    this.addRule(new WebInspector.AuditRules.StaticCookielessRule({MinResources: 5}));
> +    this.addRule(new WebInspector.AuditRules.CombineJsResourcesRule({AllowedPerDomain: 2, ScorePerResource: 11}));
> +    this.addRule(new WebInspector.AuditRules.CombineCssResourcesRule({AllowedPerDomain: 2, ScorePerResource: 11}));
> +    this.addRule(new WebInspector.AuditRules.MinimizeDnsLookupsRule({HostCountThreshold: 4, ViolationDomainScore: 6}));
> +    this.addRule(new WebInspector.AuditRules.ParallelizeDownloadRule({OptimalHostnameCount: 4, MinRequestThreshold: 10, MinBalanceThreshold: 0.5}));
> +    this.addRule(new WebInspector.AuditRules.BrowserCacheControlRule());
> +    this.addRule(new WebInspector.AuditRules.ProxyCacheControlRule());
> +}

When is this instantiated? It would be great if it was happening upon audit start.

> -        this.resize();
> +        setTimeout(this.resize.bind(this), 0);

Looks like a hack.

> +WebInspector.AuditRules.linkifyResource = function(url)
> +{
> +  var element = document.createElement("a");
> +  element.className = "webkit-html-resource-link";
> +  element.href = url;
> +  element.preferredPanel = "resources";
> +  element.textContent = url;
> +  return element.outerHTML;

Please use WebInspector.displayNameForURL or even better WebInspector.linkifyURLAsNode instead.

> +}
> +
> +/**
> + * @param {Array} array Array of Elements (outerHTML is used) or strings (plain value is used as innerHTML)
> + */
> +WebInspector.AuditRules.arrayAsUL = function(array, shouldLinkify)
> +{
> +    if (!array.length)
> +        return "";
> +    var result = "<ul>";
> +    for (var i = 0; i < array.length; ++i) {
> +        result += "<li>";
> +        result += (array[i] instanceof Element ?
> +            array[i].outerHTML :
> +            (shouldLinkify ? WebInspector.AuditRules.linkifyResource(array[i]) : array[i]));
> +        result += "</li>";
> +    }
> +    result += "</ul>";
> +    return result;
> +}
> +

I am not a big fan of the string manipulation. Why not to use dom api here? Also, sounds like a utility function (for utilities.js).

> + // Gzip

These comments are not meaningful.

> +    isCompressed: function(resource)
> +    shouldCompress: function(resource)

These should be private

> +            }
> +            result.score = 100 - (penalizedResourceCount * this.getValue("ScorePerResource"));

Comment on a general engine structure. It would be nice if parameters object was explicitly passed into doRun and the check would be parameters.scorePerResource. I realize that it would not have a nice runtime check for this parameter, but you could add assertions on parameters in the beginning of the rule instead.

> +        } finally {
> +            callback(result);
> +        }

You are using finally a lot. Are you sure that you are able to render incomplete results? I think error message be displayed instead.

> +    getUnusedStylesheetRatioMessage: function(unusedLength, key)
> +    {
> +        var parts = key.split("-", 3);
> +    getUnusedTotalRatioMessage: function(unusedLength, totalLength)
> +    {
> +        var pctUnused = Math.round(unusedLength / totalLength * 100);

Here and in other places: should these be private?

> +        function EvalCallback(evalResult, isException)
> +        {

function evalCallback(...)
{

> +
> +        var routine = function() {

function routine()
{

> +            var images = document.getElementsByTagName("img");
> +            const widthRegExp = /width[^:;]*:/gim;
> +            const heightRegExp = /height[^:;]*:/gim;
> +
> +            function hasDimension(element, cssText, rules, regexp, attributeName) {
> +                if (element.attributes.getNamedItem(attributeName) != null ||
> +                    (cssText && cssText.match(regexp)))

Weird wrap. Why wrapping here at all?

[Timothy Hatcher]

(In reply to comment #4)
> > +WebInspector.AuditCategories.Page = function() {
> 
> Page -> PagePerformance
> Also I am not sure you need a special namespace (AuditCategories) for that.

I like having the AuditCategories namespace.

[Alexander Pavlov (apavlov)]

(In reply to comment #5)
> (In reply to comment #4)
> > > +WebInspector.AuditCategories.Page = function() {
> > 
> > Page -> PagePerformance
> > Also I am not sure you need a special namespace (AuditCategories) for that.
> 
> I like having the AuditCategories namespace.

The categories are retrieved from this namespace during construction:

for (var categoryCtorID in WebInspector.AuditCategories)

Also, the object name (PagePerformance or whatever) becomes the category ID internally, which seems to save space on explicit category IDs.

[Alexander Pavlov (apavlov)]

(In reply to comment #4)

The comments inline pertain to the patch following shortly.

> (From update of attachment 47413 [details])
> Looks good overall. Some nits I'd like to see fixed before you land:
> 
> > +WebInspector.AuditCategories.Page = function() {
> 
> Page -> PagePerformance

Done.

> Also I am not sure you need a special namespace (AuditCategories) for that.

The namespace is scanned for audit categories by the engine (see comment #6).

> > +
> > +WebInspector.AuditCategories.Network = function() {
> 
> Network -> NetworkUtilization

Done.

> > +    this.addRule(new WebInspector.AuditRules.ParallelizeDownloadRule({OptimalHostnameCount: 4, MinRequestThreshold: 10, MinBalanceThreshold: 0.5}));
> > +    this.addRule(new WebInspector.AuditRules.BrowserCacheControlRule());
> > +    this.addRule(new WebInspector.AuditRules.ProxyCacheControlRule());
> > +}
> 
> When is this instantiated? It would be great if it was happening upon audit
> start.

Done lazy instantiation.

> > -        this.resize();
> > +        setTimeout(this.resize.bind(this), 0);
> Looks like a hack.

Moved into show().

> > +WebInspector.AuditRules.linkifyResource = function(url)
> > +{
> > +  var element = document.createElement("a");
> 
> Please use WebInspector.displayNameForURL or even better
> WebInspector.linkifyURLAsNode instead.

Done.

> > +}
> > +
> > +/**
> > + * @param {Array} array Array of Elements (outerHTML is used) or strings (plain value is used as innerHTML)
> > + */
> > +WebInspector.AuditRules.arrayAsUL = function(array, shouldLinkify)
> > +{
> > +    if (!array.length)
> > +        return "";
> > +    var result = "<ul>";
> > +    for (var i = 0; i < array.length; ++i) {
> I am not a big fan of the string manipulation. Why not to use dom api here?

Done.

> Also, sounds like a utility function (for utilities.js).

It doesn't seem so much useful as to put it into utilities.js. Interested parties (i.e. audit authors) are free to use it from inside the AuditRules namespace.


> > + // Gzip
> 
> These comments are not meaningful.

Done.

> > +    isCompressed: function(resource)
> > +    shouldCompress: function(resource)
> 
> These should be private

Done.

> > +            }
> > +            result.score = 100 - (penalizedResourceCount * this.getValue("ScorePerResource"));
> 
> Comment on a general engine structure. It would be nice if parameters object
> was explicitly passed into doRun and the check would be
> parameters.scorePerResource. I realize that it would not have a nice runtime
> check for this parameter, but you could add assertions on parameters in the
> beginning of the rule instead.

We can elaborate on this later.

> > +        } finally {
> > +            callback(result);
> > +        }
> 
> You are using finally a lot. Are you sure that you are able to render
> incomplete results? I think error message be displayed instead.

"Incomplete" means "Underreported" in this context. The engine runs rules asynchronously (since they can run asynchronous code) and counts down the number of remaining rules. If a rule fails to report to its callback, the engine will remain in a hung state waiting for more callback invocations. finally's are there as guards to avoid the Audits panel crash. Should we think of something different?

> > +    getUnusedStylesheetRatioMessage: function(unusedLength, key)
> > +    {
> > +        var parts = key.split("-", 3);
> > +    getUnusedTotalRatioMessage: function(unusedLength, totalLength)
> > +    {
> > +        var pctUnused = Math.round(unusedLength / totalLength * 100);
> 
> Here and in other places: should these be private?

Done.

> > +        function EvalCallback(evalResult, isException)
> > +        {
> 
> function evalCallback(...)
> {

Done.

> > +
> > +        var routine = function() {
> 
> function routine()
> {

Done.

> > +            var images = document.getElementsByTagName("img");
> > +            const widthRegExp = /width[^:;]*:/gim;
> > +            const heightRegExp = /height[^:;]*:/gim;
> > +
> > +            function hasDimension(element, cssText, rules, regexp, attributeName) {
> > +                if (element.attributes.getNamedItem(attributeName) != null ||
> > +                    (cssText && cssText.match(regexp)))
> 
> Weird wrap. Why wrapping here at all?

Removed wrapping.

[Alexander Pavlov (apavlov)]

Created attachment 48421 [details]
[PATCH] Comments addressed

[Alexander Pavlov (apavlov)]

A minor fix applied.

Committing to http://svn.webkit.org/repository/webkit/trunk ...
        M       WebCore/ChangeLog
        M       WebCore/WebCore.gypi
        M       WebCore/WebCore.vcproj/WebCore.vcproj
        A       WebCore/inspector/front-end/AuditCategories.js
        M       WebCore/inspector/front-end/AuditLauncherView.js
        A       WebCore/inspector/front-end/AuditRules.js
        M       WebCore/inspector/front-end/AuditsPanel.js
        M       WebCore/inspector/front-end/WebKit.qrc
        M       WebCore/inspector/front-end/audits.css
        M       WebCore/inspector/front-end/inspector.html
        M       WebCore/inspector/front-end/inspector.js
Committed r54591