Bug 31784

Summary: Web Inspector: Reimplement TimelinePanel to make it scale
Product: WebKit Reporter: Pavel Feldman <pfeldman>
Component: Web Inspector (Deprecated)Assignee: Pavel Feldman <pfeldman>
Status: RESOLVED FIXED    
Severity: Normal CC: bweinstein, joepeck, rik, timothy
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
[PATCH] the patch. timothy: review+

Description Pavel Feldman 2009-11-22 04:32:37 PST
Today instrumenting 30 seconds of cnn.com or techcrunch.com makes timeline unresponsive. The reason is that several thousands or recurds result in proportional number of elements attached to the dom.

What this change does:
- Extracts grid and overview classes into separate files
- Makes timeline create only divs for visible rows
- It makes timeline really fast and scalable. We can further optimize it via bucketing events by time if that is needed (like in order to handle >100K entries).
Comment 1 Pavel Feldman 2009-11-22 04:35:53 PST
Created attachment 43675 [details]
[PATCH] the patch.
Comment 2 Timothy Hatcher 2009-11-22 07:59:30 PST
Comment on attachment 43675 [details]
[PATCH] the patch.

> +        var visibleTop = this._containerElement.scrollTop;
> +        var visibleBottom = visibleTop + this._containerElement.clientHeight;
> +        var rowHeight = 18;
> +        var startIndex = Math.max(0, Math.floor(visibleTop / rowHeight) - 1);
> +        var endIndex = Math.min(recordsInWindow.length, Math.ceil(visibleBottom / rowHeight));

Use const instead of var for these.


> +        var top = (startIndex * rowHeight) + "px";

Use const.


>  .sidebar {
>      position: absolute;
>      top: 0;
> +    min-height: 100%;
>      left: 0;
> -    bottom: 0;

Why was this needed?


>  .sidebar-resizer-vertical {
>      position: absolute;
>      top: 0;
> -    bottom: 0;
> +    min-height: 100%;

Ditto.


> +.timeline-tree-section {

Is this class uses in your patch? What is this for?


> +    color: rgb(92, 110, 129);
> +    font-family: Helvetica, Arial, sans-serif;

Why is this Helvetica? We don't use Helvetica except in a few rare places. Otherwise the font is inherited from the body.


> +    position: relative;

Why relative, so it have positioned children?


> +    text-shadow: rgba(255, 255, 255, 0.746094) 0px 1px 0px;

Just use 0.75 instead of 0.746094. The CSS parser ignores the extra precision anyway.


> +    font-family: Helvetica, Arial, sans-serif;

Why specify the font?
Comment 3 Pavel Feldman 2009-11-22 10:17:12 PST
> Use const instead of var for these.
> 

Done.

> 
> Use const.
> 

Done.

> 
> >  .sidebar {
> >      position: absolute;
> >      top: 0;
> > +    min-height: 100%;
> >      left: 0;
> > -    bottom: 0;
> 
> Why was this needed?
> 

I shared an image on IRC.

> 
> > +.timeline-tree-section {
> 
> Is this class uses in your patch? What is this for?
> 

Should not be here - from a previous version of patch. Removed.


> Why is this Helvetica? We don't use Helvetica except in a few rare places.
> Otherwise the font is inherited from the body.
> Why relative, so it have positioned children?

I was experimenting and forgot to delete. from a previous version of patch. Removed.


> > +    text-shadow: rgba(255, 255, 255, 0.746094) 0px 1px 0px;
> 
> Just use 0.75 instead of 0.746094. The CSS parser ignores the extra precision
> anyway.
> 

That was a copypaste from inspector. Fixed.
Comment 4 Pavel Feldman 2009-11-22 10:25:24 PST
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/inspector/timeline-test.js
	M	WebCore/ChangeLog
	M	WebCore/WebCore.gypi
	M	WebCore/WebCore.vcproj/WebCore.vcproj
	M	WebCore/inspector/front-end/AbstractTimelinePanel.js
	A	WebCore/inspector/front-end/TimelineGrid.js
	A	WebCore/inspector/front-end/TimelineOverviewPane.js
	M	WebCore/inspector/front-end/TimelinePanel.js
	M	WebCore/inspector/front-end/WebKit.qrc
	M	WebCore/inspector/front-end/inspector.css
	M	WebCore/inspector/front-end/inspector.html
Committed r51296