[webkit-reviews] review denied: [Bug 32930] Web Inspector: Add a simplistic audit category : [Attachment 47413] [PATCH] Proposed audit categories

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 32930: Web Inspector: Add a simplistic audit category
https://bugs.webkit.org/show_bug.cgi?id=32930

Attachment 47413: [PATCH] Proposed audit categories
https://bugs.webkit.org/attachment.cgi?id=47413&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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?