[webkit-reviews] review canceled: [Bug 31796] Web Inspector: Implement expandable compartments on timeline panel. : [Attachment 43739] [PATCH] Patch that makes it look as in latest screenshot (Image#43735)

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at hatcher.name> has canceled Pavel Feldman
<pfeldman at chromium.org>'s request for review:
Bug 31796: Web Inspector: Implement expandable compartments on timeline panel.
https://bugs.webkit.org/show_bug.cgi?id=31796

Attachment 43739: [PATCH] Patch that makes it look as in latest screenshot
(Image#43735)
https://bugs.webkit.org/attachment.cgi?id=43739&action=review

------- Additional Comments from Timothy Hatcher <timothy at hatcher.name>
> +		   graphRowElement = new
WebInspector.TimelineRecordGraphRow(this._itemsGraphsElement,
this._scheduleRefresh.bind(this, true)).element;

Maybe we should cache this _scheduleRefresh bind?


> +    this._refreshCallback = refreshCallback;
> +
>  }

Extra empty line.


> +	       this._expandElement.style.setProperty("left", left + "px");
> +	       this._expandElement.style.setProperty("width", Math.max(12,
width + 25) + "px");
> +	       if (!record.collapsed) {
> +		   this._expandElement.style.height =
(record.visibleChildrenCount + 1) * 18 + "px";

Be consistent when setting style properties. I prefer the method you used for
height over setProperty when the property name has no hyphens.


> +		   this._expandElement.style.height = "18px";
> +		  
this._expandElement.addStyleClass("timeline-expandable-collapsed");

Can this 18px be the default supplied by the timeline-expandable-collapsed
class?


> +	   this._refreshCallback();

Does the whole tree really need to refresh when expanding collapsing?


Looks fine. Should address the offset issue before this land though. Clearing
the review flag.