[Webkit-unassigned] [Bug 32930] Web Inspector: Add a simplistic audit category

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Feb 8 05:48:58 PST 2010


https://bugs.webkit.org/show_bug.cgi?id=32930


Pavel Feldman <pfeldman at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #47413|review?                     |review-
               Flag|                            |




--- Comment #4 from Pavel Feldman <pfeldman at chromium.org>  2010-02-08 05:48:56 PST ---
(From update of attachment 47413)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list