[webkit-reviews] review denied: [Bug 103945] Web Inspector: Add context menu item to re-run an audit from results : [Attachment 188297] Patch

[bugzilla-daemon at webkit.org]

Alexander Pavlov (apavlov) <apavlov at chromium.org> has denied Vivek Galatage
<vivekg at webkit.org>'s request for review:
Bug 103945: Web Inspector: Add context menu item to re-run an audit from
results
https://bugs.webkit.org/show_bug.cgi?id=103945

Attachment 188297: Patch
https://bugs.webkit.org/attachment.cgi?id=188297&action=review

------- Additional Comments from Alexander Pavlov (apavlov)
<apavlov at chromium.org>
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.AuditRulesToB
eProcessed, { 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.Audi
tInitiated, this._initiateAudit.bind(this));

Use the last parameter (the receiver object) instead:

this._auditController.addEventListener(WebInspector.AuditController.Events.Audi
tInitiated, 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