[webkit-reviews] review denied: [Bug 32930] Web Inspector: Add a simplistic audit category : [Attachment 47413] [PATCH] Proposed audit categories
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Feb 8 05:48:55 PST 2010
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?
More information about the webkit-reviews
mailing list