Bug 36172 – Web Inspector: Reimplement style-related audits using native API

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

[Alexander Pavlov (apavlov)]

Recently [e-mail] introduced style inspection through the native code rather than injected script to avoid cross-domain security issues. Audits that need docuemnt styles should be reimplemented in the same manner.

[Alexander Pavlov (apavlov)]

Created attachment 50816 [details]
[PATCH] Proposed solution

[WebKit Review Bot]

Attachment 50816 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/inspector/InspectorBackend.cpp:367:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexander Pavlov (apavlov)]

Created attachment 50842 [details]
[PATCH] Style fixed

[Pavel Feldman]

Comment on attachment 50842 [details]
[PATCH] Style fixed

r+ with comments. Please fix before landing.

> +    ListHashSet<RefPtr<Document> > copy = m_documents;

No need to iterate copy in sync call, just iterate the source.

> +        for (unsigned i = 0, size = list->length(); i < size; ++i) {

We don't do such optimization in the rest 99% of this class, be consistent.

> -            matchedCSSRules.set(i, buildObjectForRule(static_cast<CSSStyleRule*>(rule)));
> +            matchedCSSRules.set(counter++, buildObjectForRule(static_cast<CSSStyleRule*>(rule)));

Thanks for fixing this.

> +    result.set("docId", m_documentNodeToIdMap.get(styleSheet->doc()));

Strictly speaking, this is documentElementId, not docId.

> +    result.set("cssRules", styleSheetRules);

Nit: pick single name for both parts: either cssRules or styleSheetRules.

> +    PassRefPtr<CSSRuleList> cssRuleList = CSSRuleList::create(styleSheet, true);
> +    if (cssRuleList) {

Nit: You could do if (!cssRuleList) return here (less nesting).

> +    result.set("cssText", rule->cssText());

Nice, we can now try implementing Edit As Text on style rule!

> +
> +                    if (unusedRules.length) {
> +                        var entry = { type: styleSheet.href ? "href" : "inline",
> +                            totalSize: stylesheetSize,
> +                            unusedSize: unusedStylesheetSize,
> +                            location: styleSheet.href ? styleSheet.href : ++inlineBlockOrdinal,
> +                            unusedRules: unusedRules };
> +                        routineResult.styleSheets.push(entry);

Please fix this this before landing: This is no longer an injected routine, no need to use intermediate routineResult jsonable object here. There is also no need in its styleSheets property, no need in unusedRules array above. Just get rid of all of that code and dump audit results from within selectorsCallback. You have all necessary info for that. I can take a look at the patch before you land if you have questions on this one!

> +                    }
> +                }
> +
> +                if (!totalUnusedStylesheetSize)
> +                    return callback(null);
>  
> -            for (var i = 0; i < routineResult.styleSheets.length; ++i) {
> -                var stylesheet = routineResult.styleSheets[i];
> +                var totalUnusedPercent = Math.round(100 * totalUnusedStylesheetSize / totalStylesheetSize);
> +                var summary = result.addChild(String.sprintf("%d%% of CSS (estimated) is not used by the current page.", totalUnusedPercent), true);
>  
> -                var url = stylesheet.type === "href" ? WebInspector.linkifyURL(stylesheet.location) : String.sprintf("Inline block #%s", stylesheet.location);
> -                var pctUnused = Math.round(100 * stylesheet.unusedSize / stylesheet.totalSize);
> -                var entry = summary.addChild(String.sprintf("%s: %d%% (estimated) is not used by the current page.", url, pctUnused));
> +                for (var i = 0; i < routineResult.styleSheets.length; ++i) {
> +                    var stylesheet = routineResult.styleSheets[i];
>  
> -                for (var j = 0; j < stylesheet.unusedRules.length; ++j)
> -                    entry.addSnippet(stylesheet.unusedRules[j]);
> +                    var url = stylesheet.type === "href" ? WebInspector.linkifyURL(stylesheet.location) : String.sprintf("Inline block #%s", stylesheet.location);
> +                    var pctUnused = Math.round(100 * stylesheet.unusedSize / stylesheet.totalSize);
> +                    var entry = summary.addChild(String.sprintf("%s: %d%% (estimated) is not used by the current page.", url, pctUnused));
>  
> -                result.violationCount += stylesheet.unusedRules.length;
> +                    for (var j = 0; j < stylesheet.unusedRules.length; ++j)
> +                        entry.addSnippet(stylesheet.unusedRules[j]);
> +
> +                    result.violationCount += stylesheet.unusedRules.length;
> +                }
> +
> +                callback(result);
>              }
>  
> -            callback(result);
> +            function routine(selectorArray)
> +            {
> +                var result = {};
> +                for (var i = 0; i < selectorArray.length; ++i) {
> +                    var nodes = document.querySelectorAll(selectorArray[i]);
> +                    if (nodes && nodes.length)
> +                        result[selectorArray[i]] = true;
> +                }
> +                return result;
> +            }
> +
> +            WebInspector.AuditRules.evaluateInTargetWindow(routine, [selectors], selectorsCallback.bind(null, callback, styleSheets, testedSelectors));
>          }
>  
>          function routine()
> @@ -306,48 +370,11 @@ WebInspector.AuditRules.UnusedCssRule.prototype = {
>              var styleSheets = document.styleSheets;
>              if (!styleSheets)
>                  return false;
> -            var routineResult = { styleSheets: [] };
> -            var inlineBlockOrdinal = 0;
> -            var totalStylesheetSize = 0;
> -            var totalUnusedStylesheetSize = 0;
> -            var pseudoSelectorRegexp = /:hover|:link|:active|:visited|:focus/;
> -            for (var i = 0; i < styleSheets.length; ++i) {
> -                var styleSheet = styleSheets[i];
> -                if (!styleSheet.cssRules)
> -                    continue;
> -                var stylesheetSize = 0;
> -                var unusedStylesheetSize = 0;
> -                var unusedRules = [];
> -                for (var curRule = 0; curRule < styleSheet.cssRules.length; ++curRule) {
> -                    var rule = styleSheet.cssRules[curRule];
> -                    var textLength = rule.cssText ? rule.cssText.length : 0;
> -                    stylesheetSize += textLength;
> -                    if (rule.type !== 1 || rule.selectorText.match(pseudoSelectorRegexp))
> -                        continue;
> -                    var nodes = document.querySelectorAll(rule.selectorText);
> -                    if (nodes && nodes.length)
> -                        continue;
> -                    unusedStylesheetSize += textLength;
> -                    unusedRules.push(rule.selectorText);
> -                }
> -                totalStylesheetSize += stylesheetSize;
> -                totalUnusedStylesheetSize += unusedStylesheetSize;
> -
> -                if (unusedRules.length) {
> -                    var entry = { type: styleSheet.href ? "href" : "inline",
> -                                  totalSize: stylesheetSize,
> -                                  unusedSize: unusedStylesheetSize,
> -                                  location: styleSheet.href ? styleSheet.href : ++inlineBlockOrdinal,
> -                                  unusedRules: unusedRules };
> -                    routineResult.styleSheets.push(entry);
> -                }
> -            }
> -            routineResult.totalSize = totalStylesheetSize;
> -            routineResult.unusedSize = totalUnusedStylesheetSize;
> +
>              return routineResult;
>          }
>  
> -        WebInspector.AuditRules.evaluateInTargetWindow(routine, evalCallback);
> +        InspectorBackend.getAllStyles(WebInspector.Callback.wrap(evalCallback));
>      }
>  }
>  
> @@ -669,7 +696,7 @@ WebInspector.AuditRules.ImageDimensionsRule.prototype = {
>              return found ? {totalImages: images.length, map: urlToNoDimensionCount} : null;
>          }
>  
> -        WebInspector.AuditRules.evaluateInTargetWindow(routine, evalCallback.bind(this));
> +        WebInspector.AuditRules.evaluateInTargetWindow(routine, null, evalCallback.bind(this));
>      }
>  }
>  
> @@ -743,7 +770,7 @@ WebInspector.AuditRules.CssInHeadRule.prototype = {
>              return found ? urlToViolationsArray : null;
>          }
>  
> -        WebInspector.AuditRules.evaluateInTargetWindow(routine, evalCallback);
> +        WebInspector.AuditRules.evaluateInTargetWindow(routine, null, evalCallback);
>      }
>  }
>  
> @@ -790,7 +817,7 @@ WebInspector.AuditRules.StylesScriptsOrderRule.prototype = {
>              return [ lateStyleUrls, cssBeforeInlineCount ];
>          }
>  
> -        WebInspector.AuditRules.evaluateInTargetWindow(routine, evalCallback.bind(this));
> +        WebInspector.AuditRules.evaluateInTargetWindow(routine, null, evalCallback.bind(this));
>      }
>  }
>  
> diff --git a/WebCore/inspector/front-end/DOMAgent.js b/WebCore/inspector/front-end/DOMAgent.js
> index 62fed77..83fede3 100644
> --- a/WebCore/inspector/front-end/DOMAgent.js
> +++ b/WebCore/inspector/front-end/DOMAgent.js
> @@ -718,6 +718,7 @@ WebInspector.didRemoveNode = WebInspector.Callback.processCallback;
>  WebInspector.didGetEventListenersForNode = WebInspector.Callback.processCallback;
>  
>  WebInspector.didGetStyles = WebInspector.Callback.processCallback;
> +WebInspector.didGetAllStyles = WebInspector.Callback.processCallback;
>  WebInspector.didGetInlineStyle = WebInspector.Callback.processCallback;
>  WebInspector.didGetComputedStyle = WebInspector.Callback.processCallback;
>  WebInspector.didApplyStyleText = WebInspector.Callback.processCallback;
> diff --git a/WebCore/inspector/front-end/InjectedScript.js b/WebCore/inspector/front-end/InjectedScript.js
> index 8c7d48e..2a2a252 100644
> --- a/WebCore/inspector/front-end/InjectedScript.js
> +++ b/WebCore/inspector/front-end/InjectedScript.js
> @@ -786,9 +786,10 @@ InjectedScript.createProxyObject = function(object, objectId, abbreviate)
>      return result;
>  }
>  
> -InjectedScript.evaluateOnSelf = function(funcBody)
> +InjectedScript.evaluateOnSelf = function(funcBody, args)
>  {
> -    return window.eval("(" + funcBody + ")();");
> +    var func = window.eval("(" + funcBody + ")");
> +    return func.apply(this, args || []);
>  }
>  
>  InjectedScript.CallFrameProxy = function(id, callFrame)

[Alexander Pavlov (apavlov)]

Created attachment 50888 [details]
[PATCH] Comments addressed

[Pavel Feldman]

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

I think you did not address my major complain.

[Alexander Pavlov (apavlov)]

Created attachment 50897 [details]
[PATCH] Remove intermediate storage and second loop

[Alexander Pavlov (apavlov)]

(In reply to comment #6)
> (From update of attachment 50888 [details])
> I think you did not address my major complain.

I don't think we get any tangible gain with this change (actually, it's a speed vs. space tradeoff). Anyway, the fix is in place.

[Pavel Feldman]

Comment on attachment 50897 [details]
[PATCH] Remove intermediate storage and second loop

> +                var styleSheetEntries = [];

Unused.

[Alexander Pavlov (apavlov)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
        M       WebCore/ChangeLog
        M       WebCore/inspector/InspectorBackend.cpp
        M       WebCore/inspector/InspectorBackend.h
        M       WebCore/inspector/InspectorBackend.idl
        M       WebCore/inspector/InspectorDOMAgent.cpp
        M       WebCore/inspector/InspectorDOMAgent.h
        M       WebCore/inspector/InspectorFrontend.cpp
        M       WebCore/inspector/InspectorFrontend.h
        M       WebCore/inspector/front-end/AuditRules.js
        M       WebCore/inspector/front-end/DOMAgent.js
        M       WebCore/inspector/front-end/InjectedScript.js
Committed r56107