Bug 31796 - Web Inspector: Implement expandable compartments on timeline panel.
Summary: Web Inspector: Implement expandable compartments on timeline panel.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Pavel Feldman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-22 23:55 PST by Pavel Feldman
Modified: 2009-11-24 09:43 PST (History)
4 users (show)

See Also:


Attachments
[PATCH] Original mock (73.87 KB, image/png)
2009-11-23 00:26 PST, Pavel Feldman
no flags Details
[IMAGE] Proposed looks (173.17 KB, image/png)
2009-11-23 00:32 PST, Pavel Feldman
no flags Details
[IMAGE] Same with no shadows (kinda unclear) (146.64 KB, image/png)
2009-11-23 00:38 PST, Pavel Feldman
no flags Details
[IMAGE] Explicit column and opacity for nesting level. (148.47 KB, image/png)
2009-11-23 00:53 PST, Pavel Feldman
no flags Details
[IMAGE] Expandable compartments on timeline (the one I like most) (117.62 KB, image/png)
2009-11-23 07:08 PST, Pavel Feldman
no flags Details
[IMAGE] Expandable controls on timeline, lighter UI. (Not a mock - I have a patch for this) (219.61 KB, image/png)
2009-11-23 14:36 PST, Pavel Feldman
no flags Details
[PATCH] Patch that makes it look as in latest screenshot (Image#43735) (16.66 KB, patch)
2009-11-23 15:12 PST, Pavel Feldman
no flags Details | Formatted Diff | Diff
[IMAGE] Expand controls to the left of 0ms time. (127.12 KB, image/png)
2009-11-24 00:23 PST, Pavel Feldman
no flags Details
[IMAGE] 0ms divider is only there when you move overview window all way to the left (otherwise, no 0 divider). (125.90 KB, image/png)
2009-11-24 00:24 PST, Pavel Feldman
no flags Details
[PATCH] I think it is ready. (21.48 KB, patch)
2009-11-24 01:11 PST, Pavel Feldman
timothy: review+
Details | Formatted Diff | Diff
[PATCH] Incremental diff with traversing all records. (1.74 KB, patch)
2009-11-24 08:49 PST, Pavel Feldman
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 2009-11-22 23:55:59 PST
I needed to add box-shadow for better clarity of what to starts / end where. See screenshots attached.
Comment 1 Pavel Feldman 2009-11-23 00:26:31 PST
Created attachment 43695 [details]
[PATCH] Original mock
Comment 2 Pavel Feldman 2009-11-23 00:32:37 PST
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
Comment 3 Pavel Feldman 2009-11-23 00:38:03 PST
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.
Comment 4 Pavel Feldman 2009-11-23 00:53:10 PST
Created attachment 43699 [details]
[IMAGE] Explicit column and opacity for nesting level.
Comment 5 Pavel Feldman 2009-11-23 07:08:29 PST
Created attachment 43714 [details]
[IMAGE] Expandable compartments on timeline (the one I like most)
Comment 6 Pavel Feldman 2009-11-23 14:36:55 PST
Created attachment 43735 [details]
[IMAGE] Expandable controls on timeline, lighter UI. (Not a mock - I have a patch for this)
Comment 7 Pavel Feldman 2009-11-23 15:12:40 PST
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...
Comment 8 Timothy Hatcher 2009-11-23 16:02:06 PST
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.
Comment 9 Pavel Feldman 2009-11-24 00:22:35 PST
(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.
Comment 10 Pavel Feldman 2009-11-24 00:23:38 PST
Created attachment 43752 [details]
[IMAGE] Expand controls to the left of 0ms time.
Comment 11 Pavel Feldman 2009-11-24 00:24:31 PST
Created attachment 43753 [details]
[IMAGE] 0ms divider is only there when you move overview window all way to the left (otherwise, no 0 divider).
Comment 12 Pavel Feldman 2009-11-24 01:11:13 PST
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...
Comment 13 Timothy Hatcher 2009-11-24 07:47:20 PST
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.
Comment 14 Timothy Hatcher 2009-11-24 08:23:49 PST
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.
Comment 15 Pavel Feldman 2009-11-24 08:49:43 PST
Created attachment 43774 [details]
[PATCH] Incremental diff with traversing all records.
Comment 16 Pavel Feldman 2009-11-24 09:43:07 PST
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