Bug 140664 – Web Inspector: Make collapsed sidebars undraggable

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

[Nikita Vasilyev]

Created attachment 244966 [details]
Animated GIF

It's currently possible to grip on the invisible gutter on the right edge of the console and drag in an empty sidebar.

[Radar WebKit Bug Importer]

<rdar://problem/19528505>

[Nikita Vasilyev]

Created attachment 245983 [details]
Patch

[Nikita Vasilyev]

Created attachment 245984 [details]
Animated GIF with the patch applied

[Timothy Hatcher]

Comment on attachment 245983 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/Sidebar.css:83
> +    visibility: hidden;

Why visibility: hidden? display: none will prevent mouse events and other work.

[Nikita Vasilyev]

Created attachment 246241 [details]
Animated GIF: Bug with `display:none`

`.sidebar.collapsed {display: none}` introduces a bug, see the attached GIF.

offsetWidth and totalOffsetLeft properties are 0 while resizing:

    _resizerMouseMoved: function(event)
    {
        if (this._side === WebInspector.Sidebar.Sides.Left)
            var newWidth = event.pageX - this._element.totalOffsetLeft;
        else
            var newWidth = this._element.totalOffsetLeft + this._element.offsetWidth - event.pageX;

[Nikita Vasilyev]

Created attachment 246242 [details]
Patch (using display: none)

[Timothy Hatcher]

Comment on attachment 246242 [details]
Patch (using display: none)

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

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:54
> +    this._widthBeforeResize = 0;
> +    this._resizerMouseDownX = 0;

No need to create the properties here.

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:274
> +        this._widthBeforeResize = this.width;
> +        this._resizerMouseDownX = event.pageX;

They will be create as needed here.

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:305
> +        this._widthBeforeResize = 0;
> +        this._resizerMouseDownX = 0;

Nit: No need to reset them, they will be initialized again next time. I just like to delete ephemeral properties when done.

[Nikita Vasilyev]

(In reply to comment #7)
> Comment on attachment 246242 [details]
> Patch (using display: none)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=246242&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:54
> > +    this._widthBeforeResize = 0;
> > +    this._resizerMouseDownX = 0;

A long while ago I read somewhere that it’s better to define all instance properties in constructor since it will allocate a memory during allocation and also make property access a tad faster. I don’t know if it’s still relevant or applicable to JavaScriptCore.

[Nikita Vasilyev]

Created attachment 246254 [details]
Patch (using display: none)

[Nikita Vasilyev]

(In reply to comment #8)
> (In reply to comment #7)
> > Comment on attachment 246242 [details]
> > Patch (using display: none)
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=246242&action=review
> > 
> > > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:54
> > > +    this._widthBeforeResize = 0;
> > > +    this._resizerMouseDownX = 0;
> 
> A long while ago I read somewhere that it’s better to define all instance
> properties in constructor since it will allocate a memory during allocation
> and also make property access a tad faster. I don’t know if it’s still
> relevant or applicable to JavaScriptCore.

s/during allocation/during instantiation/

[Brian Burg]

(In reply to comment #10)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Comment on attachment 246242 [details]
> > > Patch (using display: none)
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=246242&action=review
> > > 
> > > > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:54
> > > > +    this._widthBeforeResize = 0;
> > > > +    this._resizerMouseDownX = 0;
> > 
> > A long while ago I read somewhere that it’s better to define all instance
> > properties in constructor since it will allocate a memory during allocation
> > and also make property access a tad faster. I don’t know if it’s still
> > relevant or applicable to JavaScriptCore.
> 
> s/during allocation/during instantiation/

I think the rationale for current style is that it's easier to read. It may be marginally faster to unconditionally define the property, but the inspector's performance is 99% dominated by layout and painting. So I wouldn't clutter the constructors unless there's evidence it's a performance problem.

[WebKit Commit Bot]

Comment on attachment 246254 [details]
Patch (using display: none)

Clearing flags on attachment: 246254

Committed r179889: <http://trac.webkit.org/changeset/179889>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.