[webkit-reviews] review granted: [Bug 30752] Web Inspector: Multiple Selection on Scope Bars by default Conflicts with other behavior on OSX : [Attachment 41803] Fix + Joe's Comments

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at hatcher.name> has granted Brian Weinstein
<bweinstein at apple.com>'s request for review:
Bug 30752: Web Inspector: Multiple Selection on Scope Bars by default Conflicts
with other behavior on OSX
https://bugs.webkit.org/show_bug.cgi?id=30752

Attachment 41803: Fix + Joe's Comments
https://bugs.webkit.org/attachment.cgi?id=41803&action=review

------- Additional Comments from Timothy Hatcher <timothy at hatcher.name>
> +	   Need a short description and bug URL (OOPS!)
> +
> +	   No new tests. (OOPS!)

Remove.


> +	       // If multiple selection is off, we want to unselect everything
else
> +	       // and just select ourselves.
> +	       for (var i = 0; i < this.filterBarElement.childNodes.length;
++i) {
> +		   var child = this.filterBarElement.childNodes[i];
> +		   if (!child.category)
> +		       continue;
> +		   
> +		   child.removeStyleClass("selected");
> +
> +		   var filterClass = "filter-" + child.category.toLowerCase();
> +		   this.resourcesGraphsElement.removeStyleClass(filterClass);
> +		  
this.resourcesTreeElement.childrenListElement.removeStyleClass(filterClass);
> +	       }

You could factor this into a helper nested function that you share with the
other part of this function that also deselects all.


>	   if (target.hasStyleClass("selected")) {
> +	       // If selectMultiple is turned on, and we were selected, we just

> +	       // want to unselect ourselves.
>	       target.removeStyleClass("selected");
> -
>	       var filterClass = "filter-" + target.category.toLowerCase();

I prefer to keep the empty line that you deleted, it helps readability. You
should also factor out the var filterCalss line so you don't need it twice.
Like you did for the ConsoleView.


>	       target.addStyleClass("selected");
> -
>	       var filterClass = "filter-" + target.category.toLowerCase();

Keep that empty line.