Bug 78599

Summary: Web Inspector: [refactoring] extract TimelineOverviewWindow from TimelineOverviewPanel
Product: WebKit Reporter: Andrey Kosyakov <caseq>
Component: Web Inspector (Deprecated)Assignee: Andrey Kosyakov <caseq>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch pfeldman: review+

Description Andrey Kosyakov 2012-02-14 04:55:02 PST 
This patch separates the logic of window selection in timeline overview panel from the rest of the overview pane.
Comment 1 Andrey Kosyakov 2012-02-14 05:02:11 PST 
Created attachment 126960 [details]
Patch
Comment 2 WebKit Review Bot 2012-02-14 05:06:28 PST 
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.
Comment 3 Pavel Feldman 2012-02-14 05:16:34 PST 
Comment on attachment 126960 [details]
Patch

Could you describe the responsibilities of each? What is TimelineOverviewPane and TimelineOverviewWindow now?
Comment 4 Andrey Kosyakov 2012-02-14 05:23:25 PST 
(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.
Comment 5 Andrey Kosyakov 2012-02-14 05:29:29 PST 
Created attachment 126964 [details]
Patch
Comment 6 Pavel Feldman 2012-02-14 05:32:07 PST 
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?
Comment 7 WebKit Review Bot 2012-02-14 05:32:10 PST 
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.
Comment 8 Andrey Kosyakov 2012-02-14 05:41:05 PST 
Created attachment 126965 [details]
Patch
Comment 9 WebKit Review Bot 2012-02-14 05:43:33 PST 
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.
Comment 10 Pavel Feldman 2012-02-14 05:44:29 PST 
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?
Comment 11 Andrey Kosyakov 2012-02-14 05:45:35 PST 
(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'.
Comment 12 Andrey Kosyakov 2012-02-14 05:48:52 PST 
(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?
Comment 13 Pavel Feldman 2012-02-14 05:51:13 PST 
> 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.
Comment 14 Andrey Kosyakov 2012-02-14 06:05:32 PST 
(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.
Comment 15 Andrey Kosyakov 2012-02-14 06:25:31 PST 
Committed r107709: <http://trac.webkit.org/changeset/107709>