Bug 35680 – Web Inspector: Popup for Timeline panel will work in a tooltip mode

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

[Ilya Tikhonovsky]

Just %subj%

[Ilya Tikhonovsky]

Created attachment 49902 [details]
Popup for record bar was implemented.

[Ilya Tikhonovsky]

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.

[Pavel Feldman]

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.

[Ilya Tikhonovsky]

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.

[Pavel Feldman]

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.

[Ilya Tikhonovsky]

Created attachment 50018 [details]
third iteration. recent comments from pfeldman were fixed.

Recent comment about listeners and style were fixed.

[Ilya Tikhonovsky]

Created attachment 50022 [details]
Rebased version of third patch

[Pavel Feldman]

Created attachment 50030 [details]
[PATCH] Variation of patch with extracted popover helper and fixed changelog.

[Pavel Feldman]

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