Bug 33803 – Web Inspector: PanelEnablerView._windowResized is slow, called even when the view is hidden

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

[Alexander Pavlov (apavlov)]

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.

[Timothy Hatcher]

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.

[Alexander Pavlov (apavlov)]

Created attachment 46833 [details]
patch

[Alexander Pavlov (apavlov)]

Comment on attachment 46833 [details]
patch

EnablerView and WelcomeView are not actual panel views (visibleView)

[Timothy Hatcher]

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.

[Alexander Pavlov (apavlov)]

(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.

[Alexander Pavlov (apavlov)]

Created attachment 46906 [details]
patch (comments followed)

Includes a drive-by fix to remove an unused setting

[Timothy Hatcher]

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.

[Alexander Pavlov (apavlov)]

Created attachment 46920 [details]
patch (DataGrid column resizing fixed)

[WebKit Commit Bot]

Comment on attachment 46920 [details]
patch (DataGrid column resizing fixed)

Clearing flags on attachment: 46920

Committed r53545: <http://trac.webkit.org/changeset/53545>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.