Bug 35680 - Web Inspector: Popup for Timeline panel will work in a tooltip mode
Summary: Web Inspector: Popup for Timeline panel will work in a tooltip mode
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: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-03 06:51 PST by Ilya Tikhonovsky
Modified: 2010-03-04 09:36 PST (History)
2 users (show)

See Also:


Attachments
Popup for record bar was implemented. (44.25 KB, image/png)
2010-03-03 07:05 PST, Ilya Tikhonovsky
no flags Details
initial version of patch (38.84 KB, patch)
2010-03-03 07:19 PST, Ilya Tikhonovsky
pfeldman: review-
Details | Formatted Diff | Diff
second iteration. Just fixed the comments from the first one. (38.47 KB, patch)
2010-03-03 14:20 PST, Ilya Tikhonovsky
pfeldman: review-
Details | Formatted Diff | Diff
third iteration. recent comments from pfeldman were fixed. (40.38 KB, patch)
2010-03-04 07:23 PST, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
Rebased version of third patch (40.37 KB, patch)
2010-03-04 07:37 PST, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
[PATCH] Variation of patch with extracted popover helper and fixed changelog. (41.31 KB, patch)
2010-03-04 09:35 PST, Pavel Feldman
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ilya Tikhonovsky 2010-03-03 06:51:18 PST
Just %subj%
Comment 1 Ilya Tikhonovsky 2010-03-03 07:05:07 PST
Created attachment 49902 [details]
Popup for record bar was implemented.
Comment 2 Ilya Tikhonovsky 2010-03-03 07:19:13 PST
Created attachment 49904 [details]
initial version of patch

1) Now popup for Timeline works as a tooltip:
  a) hide after timeout;
  b) keep open if mouse over popup content;
  c) keep open if moved over anchor element;
2) arrow position was adjusted;
3) title split line now thinner;
4) content is selectable.

Some methods were extracted from TimelinePanel and TimelineRecordListRow and inserted into new FormattedRow class.
Autohide functionality was implemented in Popover class.
Comment 3 Pavel Feldman 2010-03-03 07:54:55 PST
Comment on attachment 49904 [details]
initial version of patch

Great job, looks good overall. Couple of nits that need to be fixed prior to landing. r- for passing panel instance into items.

> +    this.sendRequestRecords = {};
> +    this.timerRecords = {};

these are private within the file, should remain _*

> +WebInspector.TimelinePanel.showDetailsDelay = 2000;

Should be 1.5s

> +WebInspector.TimelinePanel.hideDetailsDelay = 5000;

Should be infinity (or 10s)

> +    get recordStyles()
> +    {

_getRecordStyles?
> +        var formattedRecord = new WebInspector.TimelinePanel.FormattedRecord(record, this);

passing panel instance seems to be too much.

> -                listRowElement = new WebInspector.TimelineRecordListRow().element;

ditto.

> +                graphRowElement = new WebInspector.TimelineRecordGraphRow(this, this._itemsGraphsElement, scheduleRefreshCallback, rowHeight).element;

ditto.

> +WebInspector.TimelinePanel.FormattedRecord.prototype = {
> +_createCell: function(content, styleName)


Here and below - poor indent.
Comment 4 Ilya Tikhonovsky 2010-03-03 14:20:56 PST
Created attachment 49948 [details]
second iteration. Just fixed the comments from the first one.

1) private members were returned back;
2) timeouts for popover were adjusted;
3) recordStyles renamed to private one but I think it should be getter;
4) panel is not passed to FormattedRecord, TimelineListRow and TimelineGraphRow ;
5) idents were fixed.
Comment 5 Pavel Feldman 2010-03-03 22:16:26 PST
Comment on attachment 49948 [details]
second iteration. Just fixed the comments from the first one.

Although entries are no longer aware of panel, they still are too much involved into the tooltip business. It would be better if they were entirely decoupled. Panel is interested in providing tooltips. It adds a listener to itself and whenever mouse is hovered over some interesting element, it launches its tooltip routines. You can detect interesting elements by styles, you can put a reference to a GraphRow / Bar into the dom elements for fast access. It would allow us to minimize the number of listeners and make the tooltip code more localizer.

I am in fact thinking of a generic popover tooltipping support where one would tell some helper method that he wants a tooltip within some element (panel/view). Would provide toggle filter function and cancel filter function. I think that code can be easily extracted from SourceFrame. Otherwise your change introduces 'another version' of this stuff in popover.

> +    this.contentDiv.className = "content";

Nit: this one cold be private.

>      this._containerElement.addEventListener("scroll", this._onScroll.bind(this), false);
> +    this._containerElement.addEventListener("mousemove", this._mouseOverBackground.bind(this));

_mouseOverBackground sounds too specific. It is in fact a "_mouseMove" called on all mouse move events.

>      this._expandElement.addEventListener("click", this._onClick.bind(this));
> -    this._refreshCallback = refreshCallback;
> +    this._barElement.addEventListener("click", this._onBarElementClick.bind(this));
> +    this._barElement.addEventListener("mousemove", this._mouseOverAnchor.bind(this));
>      this._rowHeight = rowHeight;

Adding a listener per entry does not seem right to me. We could just have a listener on a panel that would handle tooltips for the sake of performance. All you do is dispatch this call back to panel. Event infrastructure would do it for you on the DOM level.

>  }
>  
> @@ -736,7 +662,19 @@ WebInspector.TimelineRecordGraphRow.prototype = {
>      _onClick: function(event)
>      {
>          this._record.collapsed = !this._record.collapsed;
> -        this._refreshCallback();
> +        this._callbacks._scheduleRefresh();
> +        this._callbacks._closeRecordDetails();

I can see that _closeRecordDetails is being called from here only. Sounds like you should have closed record details from within scheduleRefreshCallback in the old code instead. No need in fine grained _closeRecordDetails event and graph row's being aware of record details as a whole.
Comment 6 Ilya Tikhonovsky 2010-03-04 07:23:19 PST
Created attachment 50018 [details]
third iteration. recent comments from pfeldman were fixed.

Recent comment about listeners and style were fixed.
Comment 7 Ilya Tikhonovsky 2010-03-04 07:37:32 PST
Created attachment 50022 [details]
Rebased version of third patch
Comment 8 Pavel Feldman 2010-03-04 09:35:23 PST
Created attachment 50030 [details]
[PATCH] Variation of patch with extracted popover helper and fixed changelog.
Comment 9 Pavel Feldman 2010-03-04 09:36:11 PST
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/inspector/front-end/Popover.js
	M	WebCore/inspector/front-end/TimelineOverviewPane.js
	M	WebCore/inspector/front-end/TimelinePanel.js
	M	WebCore/inspector/front-end/inspector.css
Committed r55530