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

[bugzilla-daemon at webkit.org]

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


Alexander Pavlov (apavlov) <apavlov at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
         AssignedTo|webkit-unassigned at lists.web |apavlov at chromium.org
                   |kit.org                     |
     Ever Confirmed|0                           |1




--- Comment #7 from Alexander Pavlov (apavlov) <apavlov at chromium.org>  2010-02-09 08:47:04 PST ---
(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.

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