To make code more brief, instead of: for (var i = 0; i < this._classNames.length; ++i) this._listItemNode.classList.add(this._classNames[i]); We should use: this._listItemNode.classList.add(...this._classNames); And instead of: document.body.classList.remove("docked"); document.body.classList.remove("right"); document.body.classList.remove("bottom"); We should use: document.body.classList.remove("docked", "right", "bottom");
Created attachment 251083 [details] Patch
Comment on attachment 251083 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=251083&action=review > Source/WebInspectorUI/UserInterface/Views/DetailsSection.js:35 > + this._element.classList.add(identifier, WebInspector.DetailsSection.StyleClassName); You could inline and remove the StyleClassName constant. We have started doing that in new code. > Source/WebInspectorUI/UserInterface/Views/GeneralTreeElement.js:80 > + this._listItemNode.classList.remove(...this._classNames); Nice! > Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:35 > + this.element.classList.add(styleClassName, WebInspector.ResourceContentView.StyleClassName); Ditto about StyleClassName.
(In reply to comment #2) > Comment on attachment 251083 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=251083&action=review > > > Source/WebInspectorUI/UserInterface/Views/DetailsSection.js:35 > > + this._element.classList.add(identifier, WebInspector.DetailsSection.StyleClassName); > > You could inline and remove the StyleClassName constant. We have started > doing that in new code. > > > Source/WebInspectorUI/UserInterface/Views/GeneralTreeElement.js:80 > > + this._listItemNode.classList.remove(...this._classNames); > > Nice! > > > Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:35 > > + this.element.classList.add(styleClassName, WebInspector.ResourceContentView.StyleClassName); > > Ditto about StyleClassName. Besides these two, there are dozens of constants that are used only for CSS classes. They should probably be inlined in a separate patch.
Created attachment 251107 [details] Patch
You don't/can't set r+ if you put the reviewers name in the ChangeLog. You can also set cq+ yourself since you are a commiter of you got a r+ on a previous version and there were only minor tweaks needed.
Comment on attachment 251107 [details] Patch Clearing flags on attachment: 251107 Committed r182990: <http://trac.webkit.org/changeset/182990>
All reviewed patches have been landed. Closing bug.