[webkit-reviews] review denied: [Bug 36424] Web Inspector: AuditRules still use getMatchedCSSRules as a part of the img-related audit. : [Attachment 51284] [PATCH] Proposed solution

[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 36424: Web Inspector: AuditRules still use getMatchedCSSRules as a part of
the img-related audit.
https://bugs.webkit.org/show_bug.cgi?id=36424

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

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