Bug 36424 – Web Inspector: AuditRules still use getMatchedCSSRules as a part of the img-related audit.

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

[Pavel Feldman]

document.defaultView.getMatchedCSSRules should be replaced with InspectorBackend accessor.

[Alexander Pavlov (apavlov)]

Created attachment 51284 [details]
[PATCH] Proposed solution

[Pavel Feldman]

Comment on attachment 51284 [details]
[PATCH] Proposed solution

> +        function imageStylesReady(imageId, context, styles)
> +        {
> +            var widthFound, heightFound;
> +            --context.imagesLeft;
> +
> +            const node = WebInspector.domAgent.nodeForId(imageId);
> +            var src = node.getAttribute("src");
> +            if (!src) {
> +                invokeDoneIfNeeded(context);
> +                return;

You could have done this in the injected audit part.

> +            for (var name in styles.styleAttributes) {
> +                if (name === "width") {
> +                    widthFound = true;
>                      continue;
> -                var cssText = (image.style && image.style.cssText) ? image.style.cssText : "";
> -                var rules = document.defaultView.getMatchedCSSRules(image, "", true);
> -                if (!hasWidth(image, cssText, rules) || !hasHeight(image, cssText, rules)) {
> -                    found = true;
> -                    if (urlToNoDimensionCount.hasOwnProperty(image.src))
> -                        ++urlToNoDimensionCount[image.src];
> -                    else
> -                        urlToNoDimensionCount[image.src] = 1;
>                  }
> +                if (name === "height")
> +                    heightFound = true;
> +            }

This part sound confusing to me. Is it equivalent to ("width" in styles.styleAttributes && "height" in styles.styleAttributes)?


> +            for (var i = styles.matchedCSSRules.length - 1; i >= 0; --i) {
> +                var style = WebInspector.CSSStyleDeclaration.parseRule(styles.matchedCSSRules[i]).style;
> +                if (style.getPropertyValue("width") !== "")
> +                    widthFound = true;
> +                if (style.getPropertyValue("height") !== "")
> +                    heightFound = true;

You could add !heightFound || !widthFound into the for check in order to same time here.

> +                if (context.urlToNoDimensionCount.hasOwnProperty(src))

Why using hasOwnProperty here?

> +        function receivedImages(imageIds)
> +        {
> +            if (!imageIds || !imageIds.length)
> +                return callback(null);
> +            var context = {imagesLeft: imageIds.length, urlToNoDimensionCount: {}};

Wrong indent before for (var i = imageIds.length - 1; i >= 0; --i)'s closing tag.

> +InjectedScript.getElementsByTagName = function(tagName)
> +{

No need to expose this. Just expose pushNodePathToFrontend to the injected audit code. r- is for this.

[Alexander Pavlov (apavlov)]

Created attachment 51305 [details]
[PATCH] Comments addressed

[Pavel Feldman]

Comment on attachment 51305 [details]
[PATCH] Comments addressed

r+ with nits, please fix before landing.

> +            var widthFound, heightFound;

No need for these to live for so long - just declare them when assigning.

> +                var nodeId = this.pushNodePathToFrontend(nodes[i]);

I'd call this new method 'getNodeId'

[Alexander Pavlov (apavlov)]

Latest comments addressed.

Committing to http://svn.webkit.org/repository/webkit/trunk ...
        M       WebCore/ChangeLog
        M       WebCore/inspector/front-end/AuditRules.js
        M       WebCore/inspector/front-end/InjectedScript.js
Committed r56347