[webkit-reviews] review denied: [Bug 35680] Web Inspector: Popup for Timeline panel will work in a tooltip mode : [Attachment 49948] second iteration. Just fixed the comments from the first one.

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


Pavel Feldman <pfeldman at chromium.org> has denied Ilya Tikhonovsky
<loislo at chromium.org>'s request for review:
Bug 35680: Web Inspector: Popup for Timeline panel will work in a tooltip mode
https://bugs.webkit.org/show_bug.cgi?id=35680

Attachment 49948: second iteration. Just fixed the comments from the first one.
https://bugs.webkit.org/attachment.cgi?id=49948&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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.


More information about the webkit-reviews mailing list