Bug 61468 - Web Inspector: Timeline panel improvements for managing current selection
Summary: Web Inspector: Timeline panel improvements for managing current selection
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: 2011-05-25 14:09 PDT by tonistiigi
Modified: 2011-05-31 23:26 PDT (History)
11 users (show)

See Also:


Attachments
Patch (4.97 KB, patch)
2011-05-25 14:25 PDT, tonistiigi
yurys: review-
Details | Formatted Diff | Diff
Screenshot with cleared column labels (123.76 KB, image/png)
2011-05-26 07:05 PDT, Yury Semikhatsky
no flags Details
Patch (5.44 KB, patch)
2011-05-28 03:24 PDT, tonistiigi
pfeldman: review-
Details | Formatted Diff | Diff
Patch (7.13 KB, patch)
2011-05-31 13:28 PDT, tonistiigi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description tonistiigi 2011-05-25 14:09:25 PDT
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)
Comment 1 tonistiigi 2011-05-25 14:25:55 PDT
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).
Comment 2 Yury Semikhatsky 2011-05-26 07:04:38 PDT
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.
Comment 3 Yury Semikhatsky 2011-05-26 07:05:13 PDT
Created attachment 94969 [details]
Screenshot with cleared column labels
Comment 4 tonistiigi 2011-05-28 03:24:05 PDT
Created attachment 95257 [details]
Patch

Fixed the issue with double click on the labels.
Comment 5 Pavel Feldman 2011-05-28 09:03:04 PDT
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.
Comment 6 tonistiigi 2011-05-28 10:49:07 PDT
(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.
Comment 7 Pavel Feldman 2011-05-28 11:34:26 PDT
> 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.
Comment 8 tonistiigi 2011-05-30 08:13:33 PDT
(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?
Comment 9 Pavel Feldman 2011-05-30 09:05:19 PDT
(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?
Comment 10 tonistiigi 2011-05-31 13:28:01 PDT
Created attachment 95471 [details]
Patch

Removed transition. Fixed style issues.
Comment 11 WebKit Commit Bot 2011-05-31 23:24:23 PDT
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: abarth@webkit.org)
The commit-queue is continuing to process your patch.
Comment 12 WebKit Commit Bot 2011-05-31 23:25:58 PDT
Comment on attachment 95471 [details]
Patch

Clearing flags on attachment: 95471

Committed r87776: <http://trac.webkit.org/changeset/87776>
Comment 13 WebKit Commit Bot 2011-05-31 23:26:04 PDT
All reviewed patches have been landed.  Closing bug.