Bug 61468 – Web Inspector: Timeline panel improvements for managing current selection

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

Timeline panel needs some improvements for moving between result data:

- Current selection window should be draggable on the x-axis on the devices that support it(for example multitouch trackpads).
- Double click should reset the selection window to default
- Smooth labels repositioning when scrolling up and down(flickers currently).

(patch to follow)

Created attachment 94858 [details]
Patch

Also fixes:
When selection is dragged with mouse slightly(1-2px) to the right, the selection first starts moving to the left(caused by the negative offset of the resizer).

[Yury Semikhatsky]

Comment on attachment 94858 [details]
Patch

Patch looks good overall but there is a problem with double click handler: it will clear column labels in the overview pane(see screenshot). I'd try calling _setWindowPosition instead of .reset() r- for now.

[Yury Semikhatsky]

Created attachment 94969 [details]
Screenshot with cleared column labels

Created attachment 95257 [details]
Patch

Fixed the issue with double click on the labels.

[Pavel Feldman]

Comment on attachment 95257 [details]
Patch

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

A couple of tiny nits and a transition question. Otherwise looks good.

> Source/WebCore/inspector/front-end/TimelineOverviewPane.js:343
> +        if(event.wheelDeltaX != 0){

if (event.wheelDeltaX !== 0) {
}

(missing spaces and != -> !==

> Source/WebCore/inspector/front-end/TimelineOverviewPane.js:372
> +    ResizerOffset: 3.5, // half pixel because offset values are not rounded but ceiled

We don't define constants on prototypes, typically they are just WebInspector.TimelineOverviewPane.ResizerOffset.

> Source/WebCore/inspector/front-end/TimelineOverviewPane.js:374
> +    WindowScrollSpeedFactor: .3

ditto

> Source/WebCore/inspector/front-end/inspector.css:2965
> +    -webkit-transition: top 0.3s ease-out 0.2s;

Is this one smoothing labels flickering?  When I implemented it originally, labels were not flickering at all (see Safari 5 and earlier Chrome builds). It just regressed at some point. I'd rather find out why.

(In reply to comment #5)

> > Source/WebCore/inspector/front-end/TimelineOverviewPane.js:372
> > +    ResizerOffset: 3.5, // half pixel because offset values are not rounded but ceiled
> 
> We don't define constants on prototypes, typically they are just WebInspector.TimelineOverviewPane.ResizerOffset.
> 

Strange because datagrid.js that I previously changed had variables with the same meaning done this way. To me it would make more sense to define it in prototype if its meaning is for object's internal use only(to save typing and lookup cost) and to use normal constants when it has some meaning for other objects. 

For example in timelineoverview, the resizer is private so why should any other object care about ResizerOffset constant.

> > Source/WebCore/inspector/front-end/inspector.css:2965
> > +    -webkit-transition: top 0.3s ease-out 0.2s;
> 
> Is this one smoothing labels flickering?  When I implemented it originally, labels were not flickering at all (see Safari 5 and earlier Chrome builds). It just regressed at some point. I'd rather find out why.

It doesn't indeed happen in Safari5(but happens on Chrome stable). I'll look more into it later because it is definitely better this way than the workaround with transition. I though it isn't possible to keep it from moving without position:fixed but I guess I was wrong.

[Pavel Feldman]

> Strange because datagrid.js that I previously changed had variables with the same meaning done this way. To me it would make more sense to define it in prototype if its meaning is for object's internal use only(to save typing and lookup cost) and to use normal constants when it has some meaning for other objects. 
> 
> For example in timelineoverview, the resizer is private so why should any other object care about ResizerOffset constant.
> 

We would normally do either 
- WebInspector.TimelineOverviewPane._ResizerOffset or 
- this._resizerOffset initialized in the constructor for private values.

I don't think lookup cost is significant in the first case, but I agree that typing is a bit painful. I do realize the benefits of your approach, but I think that consistency (where prototypes only have methods) is at least as important. I think DataGrid is more of an exception / oversight.

(In reply to comment #5)

> > Source/WebCore/inspector/front-end/inspector.css:2965
> > +    -webkit-transition: top 0.3s ease-out 0.2s;
> 
> Is this one smoothing labels flickering?  When I implemented it originally, labels were not flickering at all (see Safari 5 and earlier Chrome builds). It just regressed at some point. I'd rather find out why.

I tracked it down to http://trac.webkit.org/changeset/75555/ that is non-inspector change that makes scrolling events fire async. 

So I guess my options are:
- leave this part out of the patch
- still use the transition for better but not ideal solution.
- try to move labels outside the element with the overflow:auto. This will not be pretty as this element is outside of TimelineGrid.
- any other?

[Pavel Feldman]

(In reply to comment #8)
> (In reply to comment #5)
> 
> > > Source/WebCore/inspector/front-end/inspector.css:2965
> > > +    -webkit-transition: top 0.3s ease-out 0.2s;
> > 
> > Is this one smoothing labels flickering?  When I implemented it originally, labels were not flickering at all (see Safari 5 and earlier Chrome builds). It just regressed at some point. I'd rather find out why.
> 
> I tracked it down to http://trac.webkit.org/changeset/75555/ that is non-inspector change that makes scrolling events fire async. 
> 

Makes sense, thanks.

> So I guess my options are:
> - leave this part out of the patch

I'd do this.

> - still use the transition for better but not ideal solution.
> - try to move labels outside the element with the overflow:auto. This will not be pretty as this element is outside of TimelineGrid.

Either this or we could try position: fixed and see if it actually slows down the scrolling. But it should probably be done in a separate patch anyways.

> - any other?

Created attachment 95471 [details]
Patch

Removed transition. Fixed style issues.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 95471 [details]:

http/tests/websocket/tests/frame-length-longer-than-buffer.html bug 61837 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 95471 [details]
Patch

Clearing flags on attachment: 95471

Committed r87776: <http://trac.webkit.org/changeset/87776>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.