Bug 36424

Summary: Web Inspector: AuditRules still use getMatchedCSSRules as a part of the img-related audit.
Product: WebKit Reporter: Pavel Feldman <pfeldman>
Component: Web Inspector (Deprecated)Assignee: Alexander Pavlov (apavlov) <apavlov>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, pfeldman
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
[PATCH] Proposed solution
pfeldman: review-
[PATCH] Comments addressed pfeldman: review+

Description Pavel Feldman 2010-03-20 23:17:30 PDT
document.defaultView.getMatchedCSSRules should be replaced with InspectorBackend accessor.
Comment 1 Alexander Pavlov (apavlov) 2010-03-22 07:44:40 PDT
Created attachment 51284 [details]
[PATCH] Proposed solution
Comment 2 Pavel Feldman 2010-03-22 09:19:28 PDT
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.
Comment 3 Alexander Pavlov (apavlov) 2010-03-22 10:45:42 PDT
Created attachment 51305 [details]
[PATCH] Comments addressed
Comment 4 Pavel Feldman 2010-03-22 11:05:58 PDT
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'
Comment 5 Alexander Pavlov (apavlov) 2010-03-22 11:23:53 PDT
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