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.
<rdar://problem/19528505>
Created attachment 245983 [details] Patch
Created attachment 245984 [details] Animated GIF with the patch applied
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.
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;
Created attachment 246242 [details] Patch (using display: none)
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.
(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.
Created attachment 246254 [details] Patch (using display: none)
(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/
(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.
Comment on attachment 246254 [details] Patch (using display: none) Clearing flags on attachment: 246254 Committed r179889: <http://trac.webkit.org/changeset/179889>
All reviewed patches have been landed. Closing bug.