Bug 143914 - Web Inspector: Pass multiple arguments to classList.add and classList.remove
Summary: Web Inspector: Pass multiple arguments to classList.add and classList.remove
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: DoNotImportToRadar
Depends on:
Blocks:
 
Reported: 2015-04-17 22:17 PDT by Nikita Vasilyev
Modified: 2015-04-18 21:21 PDT (History)
8 users (show)

See Also:


Attachments
Patch (16.27 KB, patch)
2015-04-17 22:22 PDT, Nikita Vasilyev
timothy: review+
Details | Formatted Diff | Diff
Patch (16.89 KB, patch)
2015-04-18 19:25 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.