Summary: | Web Inspector: Single column layout for the elements panel when docked right | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Vladislav Kaznacheev <kaznacheev> | ||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | apavlov, keishi, loislo, pfeldman, pmuellr, vsevik, web-inspector-bugs, webkit.review.bot, yurys | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Vladislav Kaznacheev
2013-01-17 08:02:11 PST
Created attachment 183192 [details]
Patch
Comment on attachment 183192 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183192&action=review > Source/WebCore/inspector/front-end/DockController.js:187 > + __proto__: WebInspector.Object.prototype You should also annotate constructor with @extends {WebInspector.Object}. Check out Source/WebCore/inspector/compile-front-end.py. > Source/WebCore/inspector/front-end/ElementsPanel.js:60 > + this.splitView.setMinimumMainWidthPercent(minimumContentHeightPercent); setMinimalMainHeightPercent > Source/WebCore/inspector/front-end/ElementsPanel.js:62 > + // ElementsPanel never uses Top or Left sidebar position so it is safe to assume We don't use comments unless absolutely necessary. > Source/WebCore/inspector/front-end/ElementsPanel.js:179 > + _onDockStateChanged: function(event) { Please place { on the next line. > Source/WebCore/inspector/front-end/ElementsPanel.js:186 > + _sidebarPosition: function() { ditto > Source/WebCore/inspector/front-end/SidebarView.js:43 > + WebInspector.SplitView.call(this, vertical, sidebarWidthSettingName, defaultSidebarWidth || 200, defaultSidebarHeight); defaultSidebarHeight || 200 > Source/WebCore/inspector/front-end/SidebarView.js:72 > + get mainElement() We try to not use getters to improve compilability. > Source/WebCore/inspector/front-end/SidebarView.js:90 > + this.sidebarElement.removeStyleClass("split-view-sidebar-left"); You could use classList for batch operations. > Source/WebCore/inspector/front-end/SidebarView.js:103 > + if (this.sidebarPosition_ == sidebarPosition) We use === when comparing primitive types. > Source/WebCore/inspector/front-end/SidebarView.js:200 > + var minMainWidthPercent = this.isVertical() ? this._minimumMainWidthPercent : this._minimumMainHeightPercent; var minMainSizePercent ? > Source/WebCore/inspector/front-end/SplitView.js:37 > +WebInspector.SplitView = function(isVertical, sidebarSizeSettingName, defaultSidebarSize, defaultSidebarSize2) You should move this change into a separate bug. In fact, introducing a single "swap" method that would convert this split into another instance and simply re-style it might remove the necessity of the changes below. > Source/WebCore/inspector/front-end/SplitView.js:60 > + if (defaultSidebarSize2) { Then we can drop this comment. > Source/WebCore/inspector/front-end/SplitView.js:63 > + this._sidebarWidthSettingName = sidebarSizeSettingName + 'Width'; We only use double quotes. WebKit JS style is easiest to remember as the opposite to Google Closure's. > Source/WebCore/inspector/front-end/SplitView.js:73 > + if (this._sidebarHeightSettingName != this._sidebarWidthSettingName) { I would create settings for the given defaults. > Source/WebCore/inspector/front-end/SplitView.js:147 > + _updateLayout: function() { { on the next line. > Source/WebCore/inspector/front-end/SplitView.js:409 > + resizerElement.addEventListener("mousedown", this._onDragStart.bind(this), false); Why did this change? > Source/WebCore/inspector/front-end/SplitView.js:416 > + _onDragStart: function(event) { ditto Created attachment 183771 [details]
Patch
Comment on attachment 183771 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183771&action=review Please re-upload the the nits fixed prior to landing. > Source/WebCore/inspector/front-end/DockController.js:70 > + dockSide: function() { { goes to the next line > Source/WebCore/inspector/front-end/Settings.js:217 > + this._cleanUpSetting(); Accidental change Comment on attachment 183771 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183771&action=review > Source/WebCore/inspector/front-end/DockController.js:68 > + * @return {string} dockSide remove "dockSide" > Source/WebCore/inspector/front-end/SidebarView.js:116 > + case WebInspector.SidebarView.SidebarPosition.Left: style: "switch" and "case" should have the same alignment Created attachment 183780 [details]
Patch
Comment on attachment 183771 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183771&action=review >> Source/WebCore/inspector/front-end/DockController.js:68 >> + * @return {string} dockSide > > remove "dockSide" Done >> Source/WebCore/inspector/front-end/DockController.js:70 >> + dockSide: function() { > > { goes to the next line Done >> Source/WebCore/inspector/front-end/Settings.js:217 >> + this._cleanUpSetting(); > > Accidental change done >> Source/WebCore/inspector/front-end/SidebarView.js:116 >> + case WebInspector.SidebarView.SidebarPosition.Left: > > style: "switch" and "case" should have the same alignment Done Comment on attachment 183780 [details] Patch Clearing flags on attachment: 183780 Committed r140333: <http://trac.webkit.org/changeset/140333> All reviewed patches have been landed. Closing bug. |