Bug 31796 – Web Inspector: Implement expandable compartments on timeline panel.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Pavel Feldman]

I needed to add box-shadow for better clarity of what to starts / end where. See screenshots attached.

[Pavel Feldman]

Created attachment 43695 [details]
[PATCH] Original mock

[Pavel Feldman]

Created attachment 43696 [details]
[IMAGE] Proposed looks

Couple of things are different here:
1. There is a 'collapse' down arrow. Rationale: user is not ready for expand arrow to disappear
2. There is no dark background for collapsed item: rationale: borders clash when there is a top-level collapsed compartment following expanded one
3. There is a box shadow that helps understanding the z-order of things

[Pavel Feldman]

Created attachment 43697 [details]
[IMAGE] Same with no shadows (kinda unclear)

The more I look at it, the more I think that having an explicit column for these expand/collapse things could be useful - I don't like how they overlay the text.

[Pavel Feldman]

Created attachment 43699 [details]
[IMAGE] Explicit column and opacity for nesting level.

[Pavel Feldman]

Created attachment 43714 [details]
[IMAGE] Expandable compartments on timeline (the one I like most)

[Pavel Feldman]

Created attachment 43735 [details]
[IMAGE] Expandable controls on timeline, lighter UI. (Not a mock - I have a patch for this)

[Pavel Feldman]

Created attachment 43739 [details]
[PATCH] Patch that makes it look as in latest screenshot (Image#43735)

Does not yet address the initial offset of 8 pixels wrt timeline...

[Timothy Hatcher]

Comment on attachment 43739 [details]
[PATCH] Patch that makes it look as in latest screenshot (Image#43735)

> +                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.

[Pavel Feldman]

(In reply to comment #8)
> (From update of attachment 43739 [details])
> > +                graphRowElement = new WebInspector.TimelineRecordGraphRow(this._itemsGraphsElement, this._scheduleRefresh.bind(this, true)).element;
> 
> Maybe we should cache this _scheduleRefresh bind?
> 
> 

Done. It is not that critical though - we only create ~20 of them on start and then adding / removing them as necessary. Divs for rows are reused!

> 
> Extra empty line.

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

Done. Was copying from random places of frontend...

> > +                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?
> 

It does: 1) collapsed states of nested elements could change 2) we change amount of visible rows, etc. Important thing is that refresh is really inexpensive, given that divs are cached, it is somewhat closer to repaint than refresh now.

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

Done.

[Pavel Feldman]

Created attachment 43752 [details]
[IMAGE] Expand controls to the left of 0ms time.

[Pavel Feldman]

Created attachment 43753 [details]
[IMAGE] 0ms divider is only there when you move overview window all way to the left (otherwise, no 0 divider).

[Pavel Feldman]

Created attachment 43755 [details]
[PATCH] I think it is ready.

Contains a drive-by optimization of time grid - no need to re-create divs there over and over...

[Timothy Hatcher]

It bothers me that the window at the top has a different starting point than below now. Sure resizing the window changes it anyway, but a full size window no longer has horizontal alignment/width matching what is below.

[Timothy Hatcher]

Comment on attachment 43755 [details]
[PATCH] I think it is ready.

> +    right: 0px;

Just use 0, no "px".

Post a diff against this change for the overview not reporting nested fix.

[Pavel Feldman]

Created attachment 43774 [details]
[PATCH] Incremental diff with traversing all records.

[Pavel Feldman]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/inspector/timeline-test.js
	M	WebCore/ChangeLog
	M	WebCore/inspector/front-end/TimelineGrid.js
	M	WebCore/inspector/front-end/TimelineOverviewPane.js
	M	WebCore/inspector/front-end/TimelinePanel.js
	M	WebCore/inspector/front-end/inspector.css
	M	WebCore/inspector/front-end/utilities.js
Committed r51339