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

[bugzilla-daemon at webkit.org]

Alexander Pavlov (apavlov) <apavlov at chromium.org> has denied Vivek Galatage
<vivek.vg 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 180320: Patch
https://bugs.webkit.org/attachment.cgi?id=180320&action=review

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