Bug 143914

Summary: Web Inspector: Pass multiple arguments to classList.add and classList.remove
Product: WebKit Reporter: Nikita Vasilyev <nvasilyev>
Component: Web InspectorAssignee: Nikita Vasilyev <nvasilyev>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, graouts, joepeck, jonowells, mattbaker, nvasilyev, timothy, webkit-bug-importer
Priority: P2 Keywords: DoNotImportToRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
timothy: review+
Patch none

Description Nikita Vasilyev 2015-04-17 22:17:35 PDT
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");
Comment 1 Nikita Vasilyev 2015-04-17 22:22:19 PDT
Created attachment 251083 [details]
Patch
Comment 2 Timothy Hatcher 2015-04-18 04:53:08 PDT
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.
Comment 3 Nikita Vasilyev 2015-04-18 19:23:39 PDT
(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.
Comment 4 Nikita Vasilyev 2015-04-18 19:25:32 PDT
Created attachment 251107 [details]
Patch
Comment 5 Timothy Hatcher 2015-04-18 20:29:20 PDT
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 6 WebKit Commit Bot 2015-04-18 21:21:51 PDT
Comment on attachment 251107 [details]
Patch

Clearing flags on attachment: 251107

Committed r182990: <http://trac.webkit.org/changeset/182990>
Comment 7 WebKit Commit Bot 2015-04-18 21:21:56 PDT
All reviewed patches have been landed.  Closing bug.