Bug 44518

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 Flags
patch
pfeldman: review-
patch
pfeldman: review+
patch
none
patch
pfeldman: review+, pfeldman: commit-queue-
patch yurys: review+

Description Andrey Kosyakov 2010-08-24 05:53:39 PDT
Add support for adding audit categories from WebInspector extensions.
Comment 1 Andrey Kosyakov 2010-08-24 06:17:21 PDT
Created attachment 65263 [details]
patch
Comment 2 Pavel Feldman 2010-08-25 02:23:16 PDT
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...
Comment 3 Andrey Kosyakov 2010-08-25 06:57:25 PDT
(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?
Comment 4 Pavel Feldman 2010-08-25 07:11:55 PDT
> 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.
Comment 5 Andrey Kosyakov 2010-08-26 07:00:02 PDT
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.
Comment 6 Early Warning System Bot 2010-08-26 07:18:31 PDT
Attachment 65557 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3793419
Comment 7 Pavel Feldman 2010-08-26 07:44:56 PDT
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.
Comment 8 Andrey Kosyakov 2010-08-26 09:12:50 PDT
Created attachment 65573 [details]
patch

- Added missing file
- Moved webInspector.audits.severity -> AuditResult.Severity
- Split audit tests into API dump and tests per se
Comment 9 WebKit Review Bot 2010-08-26 09:17:43 PDT
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.
Comment 10 Andrey Kosyakov 2010-08-26 09:26:12 PDT
Created attachment 65575 [details]
patch

- Capitalized severity constants
- Fixed EOLs
Comment 11 Pavel Feldman 2010-08-26 14:01:07 PDT
Comment on attachment 65575 [details]
patch

Lets land manually to be on the safe side.
Comment 12 WebKit Review Bot 2010-08-27 07:04:36 PDT
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
Comment 13 Andrey Kosyakov 2010-08-27 07:22:48 PDT
Manually committed r66218: http://trac.webkit.org/changeset/66218
Build fix committed as r66220: http://trac.webkit.org/changeset/66220
Comment 14 Andrey Kosyakov 2010-08-27 12:19:00 PDT
Reverted as r66239 due to GTK & Snow Leopard test failures:
http://trac.webkit.org/changeset/66239
Comment 15 Andrey Kosyakov 2010-08-31 01:39:19 PDT
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 16 Yury Semikhatsky 2010-08-31 04:43:45 PDT
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.
Comment 17 WebKit Review Bot 2010-08-31 06:18:33 PDT
http://trac.webkit.org/changeset/66477 might have broken Qt Linux Release
Comment 18 Andrey Kosyakov 2010-08-31 07:48:26 PDT
Manually committed r66477: http://trac.webkit.org/changeset/66477
Qt test fix committed as r66480: http://trac.webkit.org/changeset/66480