Bug 103945 – Web Inspector: Re-factor Audits Panel to enable re-run of previously audits.

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

[Alice Boxhall]

Similar to the context menu on CPU profiles, add a context menu to the audit result item in the left-hand panel which allows re-running the given audit.

[Sankeerth V S]

My understanding is that, A context menu in "Split View" with option, {Re-run current audit} state should be created. 

I intend to work on this feature but before that I would like to know the feedback.

[Pavel Feldman]

(In reply to comment #1)
> My understanding is that, A context menu in "Split View" with option, {Re-run current audit} state should be created. 
> 
> I intend to work on this feature but before that I would like to know the feedback.

I think it makes more sense to provide a context menu with the single "Re-run" item on the result row in the left sidebar.

[Sankeerth V S]

Created attachment 180128 [details]
Audits Panel Changes

Please look into the Url for the diff. http://pastebin.com/raw.php?i=4JxmA3Mp

I have also attached the screenshots of the context menu shown along with some other UI related changes. I would be glad to receive the feedback on it.

[Andrey Kosyakov]

(In reply to comment #3)
> 
> I have also attached the screenshots of the context menu shown along with some other UI related changes. I would be glad to receive the feedback on it.

Looks mostly good -- could you please upload the diff as an attachment for review?

[Vivek Galatage]

Created attachment 180320 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 180320 [details]
Patch

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

> Source/WebCore/inspector/front-end/AuditLauncherView.js:137
> +                    this._sortedCategories[category]._checkboxElement.checked = true;

I don't think we should be updating selection in UI when re-running the audit.

> Source/WebCore/inspector/front-end/AuditLauncherView.js:148
> +            this._auditPresentStateElement.checked = true;
> +        else
> +            this._auditReloadedStateElement.checked = true;

ditto.

> Source/WebCore/inspector/front-end/AuditsPanel.js:114
> +        var element = event.srcElement;

event.target?

> Source/WebCore/inspector/front-end/AuditsPanel.js:117
> +        if (event.srcElement === this.sidebarElement || element.treeElement) {

How do we handle the case when the first condition is true?

> Source/WebCore/inspector/front-end/AuditsPanel.js:122
> +            contextMenu.appendItem(WebInspector.UIString(reRunWithPresentState), this._reRunAudit.bind(element.treeElement), undefined);
> +            contextMenu.appendItem(WebInspector.UIString(reRunWithReload), this._reRunAuditWithReload.bind(element.treeElement), undefined);

Drop undefined

> Source/WebCore/inspector/front-end/AuditsPanel.js:123
> +            if(this._launcherView._currentCategoriesCount && (this.auditResultsTreeElement.children != 0))

if( => if (
(this.auditResultsTreeElement.children != 0) => this.auditResultsTreeElement.children

> Source/WebCore/inspector/front-end/AuditsPanel.js:139
> +    _reRunAudit: function()
> +    {
> +        this._panel.auditsItemTreeElement.revealAndSelect();
> +        this._panel._launcherView.reRunAudit(this.categoryIds, true);
> +    },
> +
> +    _reRunAuditWithReload: function()
> +    {
> +        this._panel.auditsItemTreeElement.revealAndSelect();
> +        this._panel._launcherView.reRunAudit(this.categoryIds, false);
> +    },
> +

Looks like this could be one function with a bool parameter -- you bind them anyway.

> Source/WebCore/inspector/front-end/AuditsPanel.js:610
> + * @param {Array.<string>} catIds

I know this has been here before, but WebKit style is to avoid abbreviations where possible -- so please use categoryIds (abbreviating identifiers to ids is common), or even categoriesToRun.

> Source/WebCore/inspector/front-end/AuditsPanel.js:625
> +    var title = "";
> +    for (var i = 0; i < results.length - 1; ++i) {
> +        title += results[i].title + ", ";
> +    }
> +    title += results[i].title;

var title = results.select("title").join(", ")

[Alexander Pavlov (apavlov)]

Comment on attachment 180320 [details]
Patch

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

> Source/WebCore/inspector/front-end/AuditLauncherView.js:136
> +                if (catIds[i] === this._sortedCategories[category]._id) {

You should never access private fields of types from other files. WI.AuditCategory has "get id()" to this end, so just remove the leading underscore.

> Source/WebCore/inspector/front-end/AuditLauncherView.js:145
> +        if (runImmediately)

Optionally, this could be shortened to:

var checkedElement = runImmediately ? this._auditPresentStateElement : this._auditReloadedStateElement;
checkedElement.checked = true;

> Source/WebCore/inspector/front-end/AuditLauncherView.js:170
>          else

"else" on the same line as }

> Source/WebCore/inspector/front-end/AuditsPanel.js:112
> +    _contextMenu: function()

It appears that you build the full contextMenu and show it only
if (this._launcherView._currentCategoriesCount && (this.auditResultsTreeElement.children != 0)).

To avoid the unnecessary work, start the function with (if it got it right):

if (!this._launcherView._currentCategoriesCount || this.auditResultsTreeElement.children.length)
    return;

> Source/WebCore/inspector/front-end/AuditsPanel.js:115
> +        while(element && !element.treeElement)

Whitespace after "while"

>> Source/WebCore/inspector/front-end/AuditsPanel.js:117
>> +        if (event.srcElement === this.sidebarElement || element.treeElement) {
> 
> How do we handle the case when the first condition is true?

"element" may be null here

> Source/WebCore/inspector/front-end/AuditsPanel.js:121
> +            contextMenu.appendItem(WebInspector.UIString(reRunWithPresentState), this._reRunAudit.bind(element.treeElement), undefined);

Do not use "undefined" unless in the middle of the argument list. Just omit it here.

>> Source/WebCore/inspector/front-end/AuditsPanel.js:122
>> +            contextMenu.appendItem(WebInspector.UIString(reRunWithReload), this._reRunAuditWithReload.bind(element.treeElement), undefined);
> 
> Drop undefined

Ditto

>> Source/WebCore/inspector/front-end/AuditsPanel.js:123
>> +            if(this._launcherView._currentCategoriesCount && (this.auditResultsTreeElement.children != 0))
> 
> if( => if (
> (this.auditResultsTreeElement.children != 0) => this.auditResultsTreeElement.children

1) Whitespace after "if"
2) WebKit never uses explicit [in]equality checks against 0. Please take a close look at http://www.webkit.org/coding/coding-style.html.
3) Did you want to use this.auditResultsTreeElement.children.length instead?
4) As a side note, we always use "===" to compare the identities.

> Source/WebCore/inspector/front-end/AuditsPanel.js:134
> +    _reRunAuditWithReload: function()

You don't need this function. Just add the "reload" parameter (can be {boolean=}) to _reRunAudit and bind it in the appendItem() calls above.

> Source/WebCore/inspector/front-end/AuditsPanel.js:196
> +        for (var category = 0; category < this._launcherView._sortedCategories.length; ++category) {

The "category" name seems misleading in this context (it is just an index into the array). Use the simple "i" or "categoryIndex" (or something equally unambiguous).

> Source/WebCore/inspector/front-end/AuditsPanel.js:621
> +    var title = "";

Please rewrite this as something similar to this:

var titles = [];
for (var i = 0; i < results.length; ++i)
    titles.push(results[i].title);
WebInspector.SidebarTreeElement.call(this, "audit-result-sidebar-tree-item", String.sprintf("Run %d - %s", ordinal, titles.join(", "), "", {}, false);


I'm also wary about this title growing arbitrarily long. Should we perhaps limit its length somehow?

[Alexander Pavlov (apavlov)]

As agreed offline, we can retain the UI update for the time of audit re-run, but then we should restore the original settings, since the user has never asked for this change.

[Vivek Galatage]

Created attachment 180484 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 180484 [details]
Patch

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

> Source/WebCore/inspector/front-end/AuditLauncherView.js:172
> +            if (!runImmediately)

so if we pass false as runImmediately, we will use UI as an override?
I think the code would be a bit more readableif this function is split in two -- one that gets categories & runImmediately flag from UI and another that starts audit with given categories and runImmediately flag.

> Source/WebCore/inspector/front-end/AuditsPanel.js:117
> +        if (element && element.treeElement) {

we prefer early bail-outs to large blocks under if, so please replace with:
if (!element)
    return;

[Alexander Pavlov (apavlov)]

Comment on attachment 180484 [details]
Patch

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

>> Source/WebCore/inspector/front-end/AuditLauncherView.js:172
>> +            if (!runImmediately)
> 
> so if we pass false as runImmediately, we will use UI as an override?
> I think the code would be a bit more readableif this function is split in two -- one that gets categories & runImmediately flag from UI and another that starts audit with given categories and runImmediately flag.

As discussed on IRC, this should act as an API method of the audit runner Model, and the arguments should come from the UI (provided by a View method).

[Sankeerth V S]

Created attachment 180782 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 180782 [details]
Patch

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

> Source/WebCore/inspector/front-end/AuditLauncherView.js:131
> +    setReRunAuditRunning: function(categoryIds, runImmediately)

This doesn't seem to check if another audit is currently running?

Also, why not refactor _setAuditRunning so that it handles both cases (or even separate startAudit/stopAudit)? It might use some trampoline-like method for starting normally, in order to provide the categoryIds and runImmediately values from the UI, and in other cases it would get run with parameters of the audit being re-run.

> Source/WebCore/inspector/front-end/AuditLauncherView.js:174
> +        if (!categoryIds) {

This looks bad. If you have decided to use the method this way, let's always pass in something, and compute this "something" e.g. in the caller method.

> Source/WebCore/inspector/front-end/AuditLauncherView.js:222
> +    restoreUIComponentState: function()

This should by symmetric to _preserveUIComponentsState:

1) Should be made private and get invoked from inside this class
2) Should use plural along with its counterpart: _preserveUIComponentsState

> Source/WebCore/inspector/front-end/AuditsPanel.js:125
> +        var contextmenu = new WebInspector.ContextMenu(event);

The name should be "contextMenu": http://www.webkit.org/coding/coding-style.html#names-basic

> Source/WebCore/inspector/front-end/AuditsPanel.js:126
> +        var reRunWithPresentState = WebInspector.useLowerCaseMenuTitles() ? "Re-run with present state" : "Re-run With Present State";

You don't need to initialize both of these at the same time, and you can fold it into a single variable (value conditioned on element.treeElement.runImmediately), so your contextMenu.appendItem will not be duplicated.

> Source/WebCore/inspector/front-end/AuditsPanel.js:139
> +    _reRunAudit: function(categoryIds, runImmediately)

I believe that this method should receive the sidebar tree element corresponding to

> Source/WebCore/inspector/front-end/AuditsPanel.js:201
> +        for (var category = 0; category < this._launcherView._sortedCategories.length; ++category)

category -> i ?

Also, you should have curly braces around a [multiline] block: http://www.webkit.org/coding/coding-style.html#braces-blocks

> Source/WebCore/inspector/front-end/AuditsPanel.js:205
> +        var runImmediately = this._launcherView._auditPresentStateElement.checked;

AuditsPanel should never manage/read the launcherView state

> Source/WebCore/inspector/front-end/AuditsPanel.js:211
> +            if (this._launcherView.isReRunAudit)

Why is this called from here? This is certainly a responsibility of launcherCallback(), since (a) AuditsPanel has never touched the UI of the launcher view (this was done inside the launcher view code), (b) AuditsPanel should not know anything about the launcher view structure at all (if you alter its UI, you'll have to patch AuditsPanel, too.)

[Vivek Galatage]

Created attachment 188297 [details]
Patch

[WebKit Review Bot]

Comment on attachment 188297 [details]
Patch

Attachment 188297 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16542538

New failing tests:
inspector/extensions/extensions-audits.html
inspector/extensions/extensions-audits-content-script.html
inspector/extensions/extensions-audits-api.html

[Alexander Pavlov (apavlov)]

Comment on attachment 188297 [details]
Patch

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

> Source/WebCore/inspector/front-end/AuditController.js:34
> +    AuditCancelled: "AuditCancelled",

JFYI: Most of the Inspector code sticks to the American spelling, "...Canceled". There are a few files where this is violated, though...

> Source/WebCore/inspector/front-end/AuditController.js:35
> +    AuditRulesToBeProcessed: "AuditRulesToBeProcessed",

AuditCategoryStarted

> Source/WebCore/inspector/front-end/AuditController.js:88
> +    notifyRulesToBeProcessed: function(categoryId, rulesCount)

follow the rename here and elsewhere

> Source/WebCore/inspector/front-end/AuditController.js:90
> +        this.dispatchEventToListeners(WebInspector.AuditController.Events.AuditRulesToBeProcessed, { categoryId: categoryId, rulesCount: rulesCount });

Web Inspector seems to use singular in this case: "ruleCount" (lineCount, resourceCount, etc.)

> Source/WebCore/inspector/front-end/AuditLauncherView.js:105
> +     * @param {event} Event

This is WebInspector.Event, not an ordinary DOM Event. Pls fix all instances.

> Source/WebCore/inspector/front-end/AuditLauncherView.js:118
> +        this._progressIndicator.hide();

So, cancelling a re-run audit will not restore the UI components state? Is the audit cancellation any different from the audit finishing normally from the view's standpoint? Do we really need two separate events for these cases?

> Source/WebCore/inspector/front-end/AuditLauncherView.js:144
> +                this._subprogresses[i].setTotalWork(rulesCount);

Why not make this._subprogresses a map (categoryId -> subprogress)? This will make your life a lot easier.

> Source/WebCore/inspector/front-end/AuditLauncherView.js:249
> +    _preserveUIComponentsState: function()

You can merge this with _updateUIComponentsState, since they are only called together, and name it something like _prepareUIForAuditReRun().

> Source/WebCore/inspector/front-end/AuditLauncherView.js:305
> +            this._stopAudit();

This must be called from _onAuditCancelled, otherwise cancelling the audit any other way will retain the view in an inconsistent state.

> Source/WebCore/inspector/front-end/AuditsPanel.js:64
> +    this._auditController.addEventListener(WebInspector.AuditController.Events.AuditInitiated, this._initiateAudit.bind(this));

Use the last parameter (the receiver object) instead:

this._auditController.addEventListener(WebInspector.AuditController.Events.AuditInitiated, this._initiateAudit, this);

> Source/WebCore/inspector/front-end/AuditsPanel.js:124
> +        if (!element || this._auditController.isAuditRunning())

The this._auditController.isAuditRunning() check should be the first line of the method.

> Source/WebCore/inspector/front-end/AuditsPanel.js:127
> +        if (element.treeElement) {

You don't need this check. If you have reached this line, element.treeElement is defined.

> Source/WebCore/inspector/front-end/AuditsPanel.js:130
> +            var reRunWithPresentState = WebInspector.useLowerCaseMenuTitles() ? "Re-run with present state" : "Re-run With Present State";

Please avoid unnecessary string computations for contextMenuTitle

> Source/WebCore/inspector/front-end/AuditsPanel.js:191
> +     _auditFinishedCallback: function(categoryIds, runImmediately, mainResourceURL, results)

Bad indentation introduced

> Source/WebCore/inspector/front-end/AuditsPanel.js:208
> +     * @param {Event} event

This is WebInspector.Event

[Vivek Galatage]

Comment on attachment 188297 [details]
Patch

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

Thank you for the review comments. I will incorporate all of these comments. Although I have some queries about the below comment

>> Source/WebCore/inspector/front-end/AuditLauncherView.js:118
>> +        this._progressIndicator.hide();
> 
> So, cancelling a re-run audit will not restore the UI components state? Is the audit cancellation any different from the audit finishing normally from the view's standpoint? Do we really need two separate events for these cases?

If the user is canceling the ReRunning of the audit, and if we allow the UI to be restored, the user will get a surprise flicker with the UI controls. i.e. the state of the controls will be changed in transit. I think in my opinion this will be little weird user experience. Should we allow this? Hence I have not called the restore UI from the _onAuditCanceled. So this is where these both events differ and hence separated.

[Vivek Galatage]

Created attachment 188838 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 188838 [details]
Patch

This patch is conflicting with the current AuditController.js, removing r?