Bug 33803 - Web Inspector: PanelEnablerView._windowResized is slow, called even when the view is hidden
Summary: Web Inspector: PanelEnablerView._windowResized is slow, called even when the ...
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: 2010-01-18 09:29 PST by Alexander Pavlov (apavlov)
Modified: 2010-01-20 05:58 PST (History)
8 users (show)

See Also:


Attachments
patch (6.85 KB, patch)
2010-01-18 10:40 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
patch (comments followed) (13.92 KB, patch)
2010-01-19 07:20 PST, Alexander Pavlov (apavlov)
timothy: review+
Details | Formatted Diff | Diff
patch (DataGrid column resizing fixed) (14.16 KB, patch)
2010-01-19 09:59 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Pavlov (apavlov) 2010-01-18 09:29:51 PST
PanelEnablerView._windowResized is invoked on EVERY inspector window resize. It makes the resizing slow - the method sometimes takes as much as 20% of the time profile.
Comment 1 Timothy Hatcher 2010-01-18 09:34:06 PST
Nice find. We can make the owner Panel update the size when Panel.resize is called.

Some panels implement resize already, and that shares a global listener and only goes to the active panel.
Comment 2 Alexander Pavlov (apavlov) 2010-01-18 10:40:17 PST
Created attachment 46833 [details]
patch
Comment 3 Alexander Pavlov (apavlov) 2010-01-18 10:53:53 PST
Comment on attachment 46833 [details]
patch

EnablerView and WelcomeView are not actual panel views (visibleView)
Comment 4 Timothy Hatcher 2010-01-18 10:57:27 PST
Comment on attachment 46833 [details]
patch


>      resize: function()
>      {
>          this.updateGraphDividersIfNeeded();
> +        WebInspector.Panel.prototype.resize.call(this);
>      },

We should call the prototype first if there isn't a good reason to call it last. And I like to seperate them by a blank line to distinguish it.


>          this.updateSidebarWidth();
> +
> +        if (this._delayedResize) {
> +            this._delayedResize = false;
> +            this.resize();
> +        }

There isn't a need for the _delayedResize code, since resize is only called on the currentPanel. And updateSidebarWidth does a resize.

>          this.updateMainViewWidth(width);
>
> -        var visibleView = this.visibleView;
> -        if (visibleView && "resize" in visibleView)
> -            visibleView.resize();
> +        WebInspector.Panel.prototype.resize.call(this);

I think we should add the visibleView resize code to updateMainViewWidth, and just make Panel.prototype.resize call updateMainViewWidth.

Then this code wouldn't be needed, since updateMainViewWidth is called right before it.

We want subclasses of updateMainViewWidth to call the Panel prototype then.


> +    resize: function()
> +    {
> +        if (!this.visible) {
> +            this._delayedResize = true;
> +            return;
> +        }

I don't think the delayed resize code is needed.


Is the Welcome view and Enabler view the visibleView? Where is that handled? I don't think it is. The visibleView is only used by the data views.
Comment 5 Alexander Pavlov (apavlov) 2010-01-19 07:18:59 PST
(In reply to comment #4)
> (From update of attachment 46833 [details])
> 
> >      resize: function()
> >      {
> >          this.updateGraphDividersIfNeeded();
> > +        WebInspector.Panel.prototype.resize.call(this);
> >      },
> 
> We should call the prototype first if there isn't a good reason to call it
> last. And I like to seperate them by a blank line to distinguish it.

Looks fine here, although if we want to handle the resize event in the general case, it should be done AFTER all "left" and "width" properties have been set (which is usually done in subclasses), otherwise there is no "resize" when the splitter is dragged.

I believe a better alternative would be for the Panel ancestors to invoke resize() manually from updateMainViewWidth() if need be (i.e. if the view is using absolute positioning).

> 
> >          this.updateSidebarWidth();
> > +
> > +        if (this._delayedResize) {
> > +            this._delayedResize = false;
> > +            this.resize();
> > +        }
> 
> There isn't a need for the _delayedResize code, since resize is only called on
> the currentPanel. And updateSidebarWidth does a resize.

Done.

> >          this.updateMainViewWidth(width);
> >
> > -        var visibleView = this.visibleView;
> > -        if (visibleView && "resize" in visibleView)
> > -            visibleView.resize();
> > +        WebInspector.Panel.prototype.resize.call(this);
> 
> I think we should add the visibleView resize code to updateMainViewWidth, and
> just make Panel.prototype.resize call updateMainViewWidth.

What should be the "width" argument to updateMainViewWidth in this case? this._currentSidebarWidth?
Also, it becomes sooooo slooow... Consider DataGrid.updateWidths() which sets all column widths in percent, yet it needs to reiterate that on every sidebar resize which, due to the slowness, makes the resizing process extremely abrupt  -> poor usability.

> Then this code wouldn't be needed, since updateMainViewWidth is called right
> before it.

Done.

> We want subclasses of updateMainViewWidth to call the Panel prototype then.

Done.

> > +    resize: function()
> > +    {
> > +        if (!this.visible) {
> > +            this._delayedResize = true;
> > +            return;
> > +        }
> 
> I don't think the delayed resize code is needed.
 
Done.

> Is the Welcome view and Enabler view the visibleView? Where is that handled? I
> don't think it is. The visibleView is only used by the data views.

Yes, I cancelled the review due to this.
Comment 6 Alexander Pavlov (apavlov) 2010-01-19 07:20:16 PST
Created attachment 46906 [details]
patch (comments followed)

Includes a drive-by fix to remove an unused setting
Comment 7 Timothy Hatcher 2010-01-19 08:48:46 PST
Comment on attachment 46906 [details]
patch (comments followed)

Not resizing things like column widths when the window resizes sounds like a bug.

I still think all panels should have all their resize logic in updateMainViewWidth, and none would impement a version of resize. Only Panel would have resize that calls updateMainViewWidth.

THe width passed to updateMainViewWidth in that case can be the current offsetWidth.
Comment 8 Alexander Pavlov (apavlov) 2010-01-19 09:59:28 PST
Created attachment 46920 [details]
patch (DataGrid column resizing fixed)
Comment 9 WebKit Commit Bot 2010-01-20 05:58:28 PST
Comment on attachment 46920 [details]
patch (DataGrid column resizing fixed)

Clearing flags on attachment: 46920

Committed r53545: <http://trac.webkit.org/changeset/53545>
Comment 10 WebKit Commit Bot 2010-01-20 05:58:34 PST
All reviewed patches have been landed.  Closing bug.