[webkit-reviews] review granted: [Bug 36172] Web Inspector: Reimplement style-related audits using native API : [Attachment 50842] [PATCH] Style fixed
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Mar 16 23:45:04 PDT 2010
Pavel Feldman <pfeldman at chromium.org> has granted Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 36172: Web Inspector: Reimplement style-related audits using native API
https://bugs.webkit.org/show_bug.cgi?id=36172
Attachment 50842: [PATCH] Style fixed
https://bugs.webkit.org/attachment.cgi?id=50842&action=review
------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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)
More information about the webkit-reviews
mailing list