Bug 101749 – Web Inspector: Timeline: enhance short-records filter.

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

Currently filter could be "on" (accepting records longer than 15ms) or "off" (accepting all records).

Adding some options between 0ms and 15ms will make this feature much more useful.

Created attachment 173285 [details]
Screenshot1

Created attachment 173286 [details]
Screenshot2

Created attachment 173300 [details]
Patch

[WebKit Review Bot]

Comment on attachment 173300 [details]
Patch

Attachment 173300 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14794078

[Pavel Feldman]

Comment on attachment 173300 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173300&action=review

> Source/WebCore/ChangeLog:10
> +        will make this feature much more useful.

What is this assessment based on?

> Source/WebCore/inspector/front-end/ProfilesPanel.js:1028
>          this.profileViewStatusBarItemsContainer.style.left = Math.max(minFloatingStatusBarItemsOffset, this.splitView.sidebarWidth()) + "px";

This will result as a warning on your timeline, no? You are measuring totalOffsetLeft and applying it to the style. Is that intended?

> Source/WebCore/inspector/front-end/StatusBarButton.js:32
> + * @typedef {{element: !Element, disabled: boolean}}

We don't use typedefs, ui components are compiled together.

> Source/WebCore/inspector/front-end/StatusBarButton.js:286
> +    set disabled(value)

Please do not create new getters / setters. They are not compiler friendly.

Comment on attachment 173300 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173300&action=review

>> Source/WebCore/ChangeLog:10
>> +        will make this feature much more useful.
> 
> What is this assessment based on?

This assessment is based on experience analyzing WebGL apps.
Typically each frame is 8-30 ms long.
But it may contain a ton of short calls to graphics subsystem. It is hard to find most significant (>=1ms) among them.

>> Source/WebCore/inspector/front-end/ProfilesPanel.js:1028
>>          this.profileViewStatusBarItemsContainer.style.left = Math.max(minFloatingStatusBarItemsOffset, this.splitView.sidebarWidth()) + "px";
> 
> This will result as a warning on your timeline, no? You are measuring totalOffsetLeft and applying it to the style. Is that intended?

totalOffestLeft had been being measured before this patch.
This change make it possible to add status bar items of unspecified width as well as make changes of style easier.

Results of measuring are intentionally applied to style, because there is no CSS-friendly way to make this position binding.

Perhaps, we could throw away this logic, because it doesn't make much sense to move additional controls alongside split-view pane.

>> Source/WebCore/inspector/front-end/StatusBarButton.js:32
>> + * @typedef {{element: !Element, disabled: boolean}}
> 
> We don't use typedefs, ui components are compiled together.

I would like to use interface, but getters made this impossible.

>> Source/WebCore/inspector/front-end/StatusBarButton.js:286
>> +    set disabled(value)
> 
> Please do not create new getters / setters. They are not compiler friendly.

Yes, I know. But this getter was added because of StatusBarButton one's.
I would like to remove both, if you do not mind.

Comment on attachment 173300 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173300&action=review

>>> Source/WebCore/inspector/front-end/StatusBarButton.js:32
>>> + * @typedef {{element: !Element, disabled: boolean}}
>> 
>> We don't use typedefs, ui components are compiled together.
> 
> I would like to use interface, but getters made this impossible.

Fixed.

>>> Source/WebCore/inspector/front-end/StatusBarButton.js:286
>>> +    set disabled(value)
>> 
>> Please do not create new getters / setters. They are not compiler friendly.
> 
> Yes, I know. But this getter was added because of StatusBarButton one's.
> I would like to remove both, if you do not mind.

Removed

Created attachment 174435 [details]
Patch

Some comments on the UI:

- Select box options should not have tooltips (as shown in Screenshot1). If you want to communicate "Show all records", just make the option say "Show all records". Likewise for the other options.

- On the subject of tooltips, it's confusing when the tooltip says the inverse of what the control says: "Show records >= 15ms" vs. "Hide records < 15ms". It's not communicating extra information.

What about keeping the existing filter button, but just making it act like a select box? I will attach a screenshot.

Created attachment 175756 [details]
Filter button as select box

[Pavel Feldman]

Comment on attachment 174435 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174435&action=review

Looks almost good. I don't think you should remove the 31* piece from non-timeline panels.

> Source/WebCore/inspector/front-end/ProfilesPanel.js:1027
> +        var minFloatingStatusBarItemsOffset = lastItemElement.totalOffsetLeft() + lastItemElement.offsetWidth;

This will cause force layout.

[Eugene Klyuchnikov]

(In reply to comment #11)
> (From update of attachment 174435 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174435&action=review
> 
> Looks almost good. I don't think you should remove the 31* piece from non-timeline panels.
> 
> > Source/WebCore/inspector/front-end/ProfilesPanel.js:1027
> > +        var minFloatingStatusBarItemsOffset = lastItemElement.totalOffsetLeft() + lastItemElement.offsetWidth;
> 
> This will cause force layout.

Actually, this won't cause additional re-layouts.
I've modified patch a bit to reduce query/set clutter.

[Eugene Klyuchnikov]

Created attachment 179704 [details]
Patch

[Andrey Kosyakov]

Committed r138244: <http://trac.webkit.org/changeset/138244>