Bug 160335 – Web Inspector: Expanding a sidebar causes two tab browser layouts

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Web Inspector

[Matt Baker]

Summary:
Expanding a sidebar causes two tab browser layouts. Collapsing a sidebar correctly causes a single layout.

[Radar WebKit Bug Importer]

<rdar://problem/27605633>

[Matt Baker]

Created attachment 284853 [details]
[Patch] Proposed Fix

[Blaze Burg]

Comment on attachment 284853 [details]
[Patch] Proposed Fix

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

> Source/WebInspectorUI/ChangeLog:11
> +        WidthDidChange event should only fire once.

Where does the other call to _recalculateWidth come from?

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:280
> +        if (!suppressWidthDidChange)

This seems like the wrong fix, it's overly fragile and the caller needs to know when a second call might happen. Shouldn't we just bail early if the newWidth is the same as the old width?

[Blaze Burg]

It would also be good to explain the performance implications, is this extra layout showing up in profiles when expanding and collapsing sidebars?

[Matt Baker]

Comment on attachment 284853 [details]
[Patch] Proposed Fix

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

>> Source/WebInspectorUI/ChangeLog:11
>> +        WidthDidChange event should only fire once.
> 
> Where does the other call to _recalculateWidth come from?

It's only called once. The other WidthDidChange occurs at the end of the "collapsed" setter.

>> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:280
>> +        if (!suppressWidthDidChange)
> 
> This seems like the wrong fix, it's overly fragile and the caller needs to know when a second call might happen. Shouldn't we just bail early if the newWidth is the same as the old width?

It's also overly complicated. Since _recalculateWidth is only called when the sidebar is expanded, we just need to do this at the end:

if (this._collapsed)
    this.dispatchEventToListeners(WebInspector.Sidebar.Event.WidthDidChange);

[Matt Baker]

(In reply to comment #4)
> It would also be good to explain the performance implications, is this extra
> layout showing up in profiles when expanding and collapsing sidebars?

Didn't check, I just noticed the double layout while debugging another view issue.

[Matt Baker]

And it's all moot now that we no longer recalculate the width on expand: https://bugs.webkit.org/show_bug.cgi?id=159646.

[Brady Eidson]

Comment on attachment 284853 [details]
[Patch] Proposed Fix

r-, as this has been pending review for over a year now. It is near-impossible that this patch still applies to trunk and unlikely to still be relevant in its current form.