Bug 36606 - Web Inspector: Short Records filter sould be implemented in Timeline Panel
Summary: Web Inspector: Short Records filter sould be implemented in 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: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-25 09:23 PDT by Ilya Tikhonovsky
Modified: 2010-03-26 01:05 PDT (History)
2 users (show)

See Also:


Attachments
[patch] Initial version for review. (9.11 KB, patch)
2010-03-25 09:36 PDT, Ilya Tikhonovsky
pfeldman: review-
Details | Formatted Diff | Diff
[patch] second iteration. (9.38 KB, patch)
2010-03-25 11:03 PDT, Ilya Tikhonovsky
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-25 09:23:42 PDT
%SUBJ%
Comment 1 Ilya Tikhonovsky 2010-03-25 09:36:53 PDT
Created attachment 51648 [details]
[patch] Initial version for review.

1) additional button was added to status bar. "Show small records";
2) initial state of that button is off;
3) all events that are 15ms or less will be hidden until Show small records" is off;
Comment 2 Pavel Feldman 2010-03-25 10:27:13 PDT
Comment on attachment 51648 [details]
[patch] Initial version for review.

Overall looks good. Please attach screenshot and fix comments!

> +    _toggleFilterButtonClicked: function()
> +    {
> +        this.toggleFilterButton.toggled = !this.toggleFilterButton.toggled;
> +        this._calculator._showShortEvents = this.toggleFilterButton.toggled;
> +        this._scheduleRefresh();

I think you want immediate refresh here.

>              }
> +            record = formattedRecord;
> +            while (record._isLongEvent && record.parent && !record.parent._isLongEvent) {
> +                record = record.parent;
> +                record._hasLongChildrenEvents = true;

Comment this?

> -        if (record.children.length) {
> +        if (record.visibleChildrenCount || (record.collapsed && record.children.length && (record._hasLongChildrenEvents || calculator._showShortEvents))) {

Comment would be nice. Something like: We show expand element for expanded nodes or for nodes that are collapsed and have unfiltered children, or we are not filtering.

>              this._expandElement.style.top = index * this._rowHeight + "px";
>              this._expandElement.style.left = barPosition.left + "px";
>              this._expandElement.style.width = Math.max(12, barPosition.width + 25) + "px";
> @@ -737,6 +758,12 @@ WebInspector.TimelinePanel.FormattedRecord = function(record, parentRecord, reco
>  }
>  
>  WebInspector.TimelinePanel.FormattedRecord.prototype = {
> +    get _isLongEvent()


This should be a function, not getter.

> +    {
> +        const shortEventLength = 0.015;
> +        return this._hasLongChildrenEvents || (this._lastChildEndTime - this.startTime) > shortEventLength;

No need for _hasLongChildrenEvents here.

> +    },
> +
Comment 3 Ilya Tikhonovsky 2010-03-25 11:03:50 PDT
Created attachment 51659 [details]
[patch] second iteration.

1) comment about _hasLongChildrenEvents propagation was added;
2) scheduleRefresh(true);
3) complex boolean expression was redesigned and looks much clear, no comments required I think.
Comment 4 Pavel Feldman 2010-03-25 11:10:31 PDT
Comment on attachment 51659 [details]
[patch] second iteration.

> +            while (record._isLongEvent() && record.parent && !record.parent._isLongEvent()) {
> +                record = record.parent;

No need to call _isLongEvent twice on the same record. I can optimize it while landing.

I'll land manually.
Comment 5 Pavel Feldman 2010-03-26 01:05:40 PDT
Slight variation landed as:

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/English.lproj/localizedStrings.js
	M	WebCore/inspector/front-end/TimelinePanel.js
	M	WebCore/inspector/front-end/inspector.css
Committed r56613