[Webkit-unassigned] [Bug 35680] Web Inspector: Popup for Timeline panel will work in a tooltip mode

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Mar 3 22:16:26 PST 2010


https://bugs.webkit.org/show_bug.cgi?id=35680


Pavel Feldman <pfeldman at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #49948|review?                     |review-
               Flag|                            |




--- Comment #5 from Pavel Feldman <pfeldman at chromium.org>  2010-03-03 22:16:26 PST ---
(From update of attachment 49948)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list