Summary: | Web Inspector: add audits support to extension API | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andrey Kosyakov <caseq> | ||||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | abarth, apavlov, bweinstein, eric, joepeck, keishi, peter, pfeldman, pmuellr, podivilov, rik, timothy, webkit-ews, webkit.review.bot, yurys | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Attachments: |
|
Description
Andrey Kosyakov
2010-08-24 05:53:39 PDT
Created attachment 65263 [details]
patch
Comment on attachment 65263 [details] patch Overall looks good, few nits below. r- for naming + formatter. LayoutTests/inspector/extensions-audits-expected.txt:16 + cancel : <function> cancel -> done LayoutTests/inspector/extensions-audits.html:11 + function onStartAuditCategory(auditRun) auditRun -> testResult WebCore/inspector/front-end/AuditLauncherView.js:138 + insertBefore < this._sortedCategories.length && this._compareCategories(category, this._sortedCategories[insertBefore]) >= 0; Please fix indentation. WebCore/inspector/front-end/AuditLauncherView.js:142 + this._categoriesElement.insertBefore(categoryElement, this._categoriesElement.children[insertBefore]); insertBefore undefined will do appendChild, no need to fork the code. WebCore/inspector/front-end/AuditLauncherView.js:130 + categoryElement.firstChild.addEventListener("click", this._boundCategoryClickListener, false); you could encapsulate this listener addition into the _createCategoryElement in order not to be involved with the internal structure here. WebCore/inspector/front-end/AuditLauncherView.js:137 + for (var insertBefore = 0; I just found indexOfObjectInListSortedByFunction in the utilities.js WebCore/inspector/front-end/ExtensionAPI.js:50 + if (typeof callback != "function") !== WebCore/inspector/front-end/ExtensionAuditCategory.js:55 + runRules: function(resources, callback) runRules -> run ? WebCore/inspector/front-end/ExtensionAuditCategory.js:83 + addRuleResult: function(ruleDisplayName, severity, violationCount, details) addResult ? LayoutTests/inspector/extensions-audits.html:24 + type: webInspector.audits.nodeType.text, I'd suggest that we use the following form text parameters: ["Foo bar %h", "http://link"]. "children" should still be there in order to organize a very common tree structure. We could come up with more formatters as we go: %s for string %t for tree %T for table and such... (In reply to comment #2) > LayoutTests/inspector/extensions-audits.html:24 > + type: webInspector.audits.nodeType.text, > I'd suggest that we use the following form text parameters: > ["Foo bar %h", "http://link"]. > "children" should still be there in order to organize a very common tree structure. > We could come up with more formatters as we go: > %s for string > %t for tree > %T for table and such... I realize this may be handy, but still don't like this: it's not readable, is not localizable (unless we add parameter numbering there), not quite extensible and does not quite suit to a case when rendering one entity requires multiple values (e.g. URL could have two: a href value and a text within the tag). I suggest we either define named formatter classes, e.g. webInspector.audit.formatters.url(href, text) webInspector.audit.formatters.snippet(text) webInspector.audit.formatters.subtree(expanded, root, children) etc. or go for imperative interface, e.g. results.addChild(webInspector.audit.resultNodes.url(href, text)) The latter is pretty close to the way we have it in Audits now, but keeps type away from method names (i.e. we don't have AddFoo for every type Foo -- rather have Foo constructors and generic AddChild method). What do you think? > webInspector.audit.formatters.url(href, text) > webInspector.audit.formatters.snippet(text) > webInspector.audit.formatters.subtree(expanded, root, children) etc. > I don't see how this addresses disadvantages pointed out + it does not handle mixed content such as text with links well. I don't see a point in additional 'formatters' object, i'd rather add methods on results: var result = auditResults.addResult(); result.title = "Leverage browser caching"; result.details = ["See more info %h", ["http://foo", "here"]]; var childResult = result.addChild(); childResult.title = ["Number foo %d", 1]; addResult and addChild can have optional title and details parameters for shorter notation. Created attachment 65557 [details]
patch
- Renamed AuditRun to AuditResult
- Replaced node types with formatters in AuditResult
- Added support for concatenating different formatters and simplified passing text-only values.
Attachment 65557 [details] did not build on qt: Build output: http://queues.webkit.org/results/3793419 Comment on attachment 65557 [details]
patch
LayoutTests/inspector/extensions-audits.html:15
+ results.addResult("Passed rule", "this rule always passes ok", webInspector.audits.severity.info);
webInspector.audits.severity.info - too many letters! result.Severity.Info ?
WebCore/inspector/front-end/AuditsPanel.js:59
+ this._launcherView.addCategory(this.categoriesById[id]);
indent.
WebCore/inspector/front-end/AuditsPanel.js:163
+ category.run(resources, ruleResultReadyCallback.bind(null, result));
bind(null) looks strange.
LayoutTests/inspector/extensions-audits-expected.txt:13
+ category.onAuditStarted fired, results dump follows:
It'd be great to dump APIs to a separate expectations file for better readability.
Created attachment 65573 [details]
patch
- Added missing file
- Moved webInspector.audits.severity -> AuditResult.Severity
- Split audit tests into API dump and tests per se
Attachment 65573 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/inspector/front-end/AuditFormatters.js:1: One or more unexpected \r (^M) found; better to use only a \n [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 86 in 25 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 65575 [details]
patch
- Capitalized severity constants
- Fixed EOLs
Comment on attachment 65575 [details]
patch
Lets land manually to be on the safe side.
http://trac.webkit.org/changeset/66218 might have broken Qt Linux Release minimal, Qt Linux ARMv5 Release, Qt Linux ARMv7 Release, and Chromium Linux Release Manually committed r66218: http://trac.webkit.org/changeset/66218 Build fix committed as r66220: http://trac.webkit.org/changeset/66220 Reverted as r66239 due to GTK & Snow Leopard test failures: http://trac.webkit.org/changeset/66239 Created attachment 66022 [details]
patch
- Migrated tests to use inspector-test2.js
- Fixed dispatching messages from front-end to extension in test
- Added InspectorTest.enableResourceTracking() & InspectorTest.disableResourceTracking()
- Log exceptions in test initialization functions
Comment on attachment 66022 [details] patch > LayoutTests/inspector/extensions-audits-tests.js:39 > +test = function() var test = > LayoutTests/inspector/extensions-audits.html:14 > + log("category.onAuditStarted fired"); Why is it not just output? > LayoutTests/inspector/extensions-test.js:11 > + if (/^extension_/.exec(symbol) && typeof window[symbol] === "function") Should be test() since you don't use result array. http://trac.webkit.org/changeset/66477 might have broken Qt Linux Release Manually committed r66477: http://trac.webkit.org/changeset/66477 Qt test fix committed as r66480: http://trac.webkit.org/changeset/66480 |