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

[bugzilla-daemon at webkit.org]

Alexander Pavlov (apavlov) <apavlov at chromium.org> has denied Sankeerth V S
<sankeerth.vs at samsung.com>'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 180782: Patch
https://bugs.webkit.org/attachment.cgi?id=180782&action=review

------- Additional Comments from Alexander Pavlov (apavlov)
<apavlov at chromium.org>
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.)