[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
bugzilla-daemon at webkit.org
Mon Mar 22 09:19:27 PDT 2010
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.
More information about the webkit-reviews
mailing list