Bug 96803 – Web Inspector: [Extensions API] explicitly manage extension audit progress

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

[Andrey Kosyakov]

Currently we update extension audit progress implicitly, based on number of results added and an expected number of results declared when creating the category.
It's suggested to make progress indication explicit to the extension code.

[Andrey Kosyakov]

Created attachment 164179 [details]
Patch

[Pavel Feldman]

Comment on attachment 164179 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164179&action=review

Should this new API have a test?

> Source/WebCore/inspector/front-end/AuditsPanel.js:284
> +                progress.done();

Thisbshould not be needed

[Yury Semikhatsky]

Comment on attachment 164179 [details]
Patch

r- per pfeldman's comments.

[Andrey Kosyakov]

Created attachment 164410 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 164410 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164410&action=review

> Source/WebCore/ChangeLog:3
> +        Web Inspector: [Extensions API] exlpicitly manage extension audit progress

Please fix this line, too

> Source/WebCore/ChangeLog:12
> +        - retain old magic for computing audit progress if extension specifies extension results count;

Remove trailing ';'

> Source/WebCore/ChangeLog:34
> +        (WebInspector.ProgressIndicator.prototype.done): Assure only first call to done() has effect;

Ditto

> Source/WebCore/inspector/front-end/AuditsPanel.js:132
> +            category.run(requests, ruleResultReadyCallback.bind(this, result), categoryDoneCallback.bind(this), subprogresses[i]);

Can you instantiate subprogresses here instead? Or do you need to always know the full amount of work up front?

> Source/WebCore/inspector/front-end/ExtensionAPI.js:514
> +            console.warn("Passing resultCouint to audits.addCategory() is deprecated. Use AuditResult.updateProgress() instead.");

resultCouint -> resultCount

[Andrey Kosyakov]

Comment on attachment 164410 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164410&action=review

>> Source/WebCore/ChangeLog:12
>> +        - retain old magic for computing audit progress if extension specifies extension results count;
> 
> Remove trailing ';'

replaced with '.'

>> Source/WebCore/inspector/front-end/AuditsPanel.js:132
>> +            category.run(requests, ruleResultReadyCallback.bind(this, result), categoryDoneCallback.bind(this), subprogresses[i]);
> 
> Can you instantiate subprogresses here instead? Or do you need to always know the full amount of work up front?

The reason I'm doing this upfront is that when all children are marked as done, the parent progress indicator is marked as done and hence is removed from view.
So this is a guard against an audit category potentially marking its progress as done very early and hence causing the entire progress bar to disappear.
I'm moving this down closer to usage, though.

>> Source/WebCore/inspector/front-end/ExtensionAPI.js:514
>> +            console.warn("Passing resultCouint to audits.addCategory() is deprecated. Use AuditResult.updateProgress() instead.");
> 
> resultCouint -> resultCount

fixed

[Andrey Kosyakov]

Created attachment 164507 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 164507 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164507&action=review

> Source/WebCore/inspector/front-end/AuditsPanel.js:101
> +        var compositeProgress = new WebInspector.CompositeProgress(this._progress);

Please move this below, somewhere around var subprogresses = [];

[Andrey Kosyakov]

Committed r128875: <http://trac.webkit.org/changeset/128875>

[Raphael Kubo da Costa (:rakuco)]

I get an additional message here when running inspector/extensions/extensions-audit-api.html on EFL:

   CONSOLE MESSAGE: line 480: Passing resultCount to audits.addCategory() is deprecated. Use AuditResult.updateProgress() instead.

[Andrey Kosyakov]

(In reply to comment #10)
> I get an additional message here when running inspector/extensions/extensions-audit-api.html on EFL:
> 
>    CONSOLE MESSAGE: line 480: Passing resultCount to audits.addCategory() is deprecated. Use AuditResult.updateProgress() instead.

The console message is there by design, however, the test expectations differ in EFL because other platforms just don't dump console output coming from inspector page to test output (I remember fixing this for chromium in bug 69632). So the long term fix would be to get efl's DRT changed to skip inspector console output as well, and short term would be to add custom expectations or skip the test. It looks like this test is already skipped in LayoutTests/platform/efl/Skipped, I wonder if EFL is using both NRWT and older run-webkit-test that uses Skipped?

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #11)
> (In reply to comment #10)
> > I get an additional message here when running inspector/extensions/extensions-audit-api.html on EFL:
> > 
> >    CONSOLE MESSAGE: line 480: Passing resultCount to audits.addCategory() is deprecated. Use AuditResult.updateProgress() instead.
> 
> The console message is there by design, however, the test expectations differ in EFL because other platforms just don't dump console output coming from inspector page to test output (I remember fixing this for chromium in bug 69632). So the long term fix would be to get efl's DRT changed to skip inspector console output as well

Thanks for the information, I will work on that.

> It looks like this test is already skipped in LayoutTests/platform/efl/Skipped, I wonder if EFL is using both NRWT and older run-webkit-test that uses Skipped?

EFL uses NRWT; Skipped has extensions-api.html and extensions-audits.html, but not extensions-audit-apis.html :-)

[Andrey Kosyakov]

(In reply to comment #12)
> EFL uses NRWT; Skipped has extensions-api.html and extensions-audits.html, but not extensions-audit-apis.html :-)

Ouch, sorry -- I missed it's extensions-audit-apis.html. Then I guess we could as well avoid using that deprecated parameter there -- it's not really required for the test and I guess it would be the must practical fix, as it would save us another Skipped test.
The problem of inspector console output, though, remains relevant for some other tests (extensions-audits.html would be one example).