[webkit-reviews] review denied: [Bug 36606] Web Inspector: Short Records filter sould be implemented in Timeline Panel : [Attachment 51648] [patch] Initial version for review.

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Ilya Tikhonovsky
<loislo at chromium.org>'s request for review:
Bug 36606: Web Inspector: Short Records filter sould be implemented in Timeline
Panel
https://bugs.webkit.org/show_bug.cgi?id=36606

Attachment 51648: [patch] Initial version for review.
https://bugs.webkit.org/attachment.cgi?id=51648&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
Overall looks good. Please attach screenshot and fix comments!

> +    _toggleFilterButtonClicked: function()
> +    {
> +	   this.toggleFilterButton.toggled = !this.toggleFilterButton.toggled;
> +	   this._calculator._showShortEvents = this.toggleFilterButton.toggled;

> +	   this._scheduleRefresh();

I think you want immediate refresh here.

>	       }
> +	       record = formattedRecord;
> +	       while (record._isLongEvent && record.parent &&
!record.parent._isLongEvent) {
> +		   record = record.parent;
> +		   record._hasLongChildrenEvents = true;

Comment this?

> -	   if (record.children.length) {
> +	   if (record.visibleChildrenCount || (record.collapsed &&
record.children.length && (record._hasLongChildrenEvents ||
calculator._showShortEvents))) {

Comment would be nice. Something like: We show expand element for expanded
nodes or for nodes that are collapsed and have unfiltered children, or we are
not filtering.

>	       this._expandElement.style.top = index * this._rowHeight + "px";
>	       this._expandElement.style.left = barPosition.left + "px";
>	       this._expandElement.style.width = Math.max(12, barPosition.width
+ 25) + "px";
> @@ -737,6 +758,12 @@ WebInspector.TimelinePanel.FormattedRecord =
function(record, parentRecord, reco
>  }
>  
>  WebInspector.TimelinePanel.FormattedRecord.prototype = {
> +    get _isLongEvent()


This should be a function, not getter.

> +    {
> +	   const shortEventLength = 0.015;
> +	   return this._hasLongChildrenEvents || (this._lastChildEndTime -
this.startTime) > shortEventLength;

No need for _hasLongChildrenEvents here.

> +    },
> +