Bug 78599 – Web Inspector: [refactoring] extract TimelineOverviewWindow from TimelineOverviewPanel

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

[Andrey Kosyakov]

This patch separates the logic of window selection in timeline overview panel from the rest of the overview pane.

[Andrey Kosyakov]

Created attachment 126960 [details]
Patch

[WebKit Review Bot]

Attachment 126960 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets
Using index info to reconstruct a base tree...
<stdin>:1578: trailing whitespace.
        
<stdin>:1647: trailing whitespace.
    
<stdin>:1657: trailing whitespace.
    
<stdin>:1672: trailing whitespace.
        return 0;        
<stdin>:1674: trailing whitespace.
    
warning: squelched 7 whitespace errors
warning: 12 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 168776 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebKit2/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog
Auto-merging Tools/ChangeLog
CONFLICT (content): Merge conflict in Tools/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pavel Feldman]

Comment on attachment 126960 [details]
Patch

Could you describe the responsibilities of each? What is TimelineOverviewPane and TimelineOverviewWindow now?

[Andrey Kosyakov]

(In reply to comment #3)
> (From update of attachment 126960 [details])
> Could you describe the responsibilities of each? What is TimelineOverviewPane and TimelineOverviewWindow now?

TimelineOverviewPane is a high-level class that holds together overview grid and heap graph. Overview window is essentially a control that manages window selection within an element passed as parent -- i.e. drag-n-drop resizing, scrolling etc.

[Andrey Kosyakov]

Created attachment 126964 [details]
Patch

[Pavel Feldman]

Comment on attachment 126964 [details]
Patch

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

> Source/WebCore/inspector/front-end/TimelineOverviewPane.js:196
> +    this._parent = parentElement;

this._parentElement

> Source/WebCore/inspector/front-end/TimelineOverviewPane.js:237
> +    scrollWindow: function(event)

Could you leave it in place for the sake of better diff?

> Source/WebCore/inspector/front-end/TimelineOverviewPane.js:261
> +            if (node.hasStyleClass("resources-dividers-label-bar")) {

why did this change?

[WebKit Review Bot]

Attachment 126964 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets
Using index info to reconstruct a base tree...
<stdin>:1578: trailing whitespace.
        
<stdin>:1647: trailing whitespace.
    
<stdin>:1657: trailing whitespace.
    
<stdin>:1672: trailing whitespace.
        return 0;        
<stdin>:1674: trailing whitespace.
    
warning: squelched 7 whitespace errors
warning: 12 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 168776 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebKit2/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog
Auto-merging Tools/ChangeLog
CONFLICT (content): Merge conflict in Tools/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andrey Kosyakov]

Created attachment 126965 [details]
Patch

[WebKit Review Bot]

Attachment 126965 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets
Using index info to reconstruct a base tree...
<stdin>:1578: trailing whitespace.
        
<stdin>:1647: trailing whitespace.
    
<stdin>:1657: trailing whitespace.
    
<stdin>:1672: trailing whitespace.
        return 0;        
<stdin>:1674: trailing whitespace.
    
warning: squelched 7 whitespace errors
warning: 12 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 168776 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebKit2/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog
Auto-merging Tools/ChangeLog
CONFLICT (content): Merge conflict in Tools/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pavel Feldman]

Comment on attachment 126965 [details]
Patch

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

> Source/WebCore/inspector/front-end/TimelineOverviewPane.js:255
> +            if (node.hasStyleClass("resources-dividers-label-bar")) {

Why did this change?

> Source/WebCore/inspector/front-end/TimelineOverviewPane.js:-264
> -            if (this._overviewGrid.itemsGraphsElement.offsetWidth - window.end > WebInspector.TimelineOverviewPane.MinSelectableSize)

This also might have side effects due to margins?

[Andrey Kosyakov]

(In reply to comment #6)

> > Source/WebCore/inspector/front-end/TimelineOverviewPane.js:196
> > +    this._parent = parentElement;
> this._parentElement

done

> 
> > Source/WebCore/inspector/front-end/TimelineOverviewPane.js:237
> > +    scrollWindow: function(event)
> Could you leave it in place for the sake of better diff?

done (as long as another class qualifies as "in place" :-))

> 
> > Source/WebCore/inspector/front-end/TimelineOverviewPane.js:261
> > +            if (node.hasStyleClass("resources-dividers-label-bar")) {
> 
> why did this change?

We used to peak into private field of a class defined in another file. We could as well use the style here, considering it's not 'private'.

[Andrey Kosyakov]

(In reply to comment #10)

> > Source/WebCore/inspector/front-end/TimelineOverviewPane.js:-264
> > -            if (this._overviewGrid.itemsGraphsElement.offsetWidth - window.end > WebInspector.TimelineOverviewPane.MinSelectableSize)
> 
> This also might have side effects due to margins?

The values are actually identical in our case. We do use clientWidth for the width of the parent container in quite a few places within the rest of overview window, do you see why it should be different in this particular case?

[Pavel Feldman]

> We used to peak into private field of a class defined in another file. We could as well use the style here, considering it's not 'private'.

Style class is as private as any other private field, so we are only masking the issue. Is there a way to fix it properly?

> The values are actually identical in our case. We do use clientWidth for the width of the parent container in quite a few places within the rest of overview window, do you see why it should be different in this particular case?

As you say, we decided to do differently in this case and I wonder if that might be for a reason.

[Andrey Kosyakov]

(In reply to comment #13)
> > We used to peak into private field of a class defined in another file. We could as well use the style here, considering it's not 'private'.
> 
> Style class is as private as any other private field, so we are only masking the issue. Is there a way to fix it properly?

I do not see a true fix for this yet, as least a cheap one. We do this because we'd like to handle dragging on timeline label bar differently from that on the graph area (moving window vs. re-creating it). The label bar is in TimelineGrid, which is not related to overview pane. We can probably use a designated style to mark it, perhaps add it in the TimelineOverviewPane (this will still require making the element public).

> > The values are actually identical in our case. We do use clientWidth for the width of the parent container in quite a few places within the rest of overview window, do you see why it should be different in this particular case?
> 
> As you say, we decided to do differently in this case and I wonder if that might be for a reason.

A few days that I spent with the timeline code seriously undermine my ability to presume that all the decisions there were made consciously and in the good will ;) So I rather assured there's no practical difference between these values.

[Andrey Kosyakov]

Committed r107709: <http://trac.webkit.org/changeset/107709>