876913 - New Netmonitor filters are single-selection only

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect, P2)

Description

[Jeff Griffiths (:canuckistani) (:⚡︎)]

I'd like to be able to select two different types of requests in the netmonitor, eg css, html & js, but not XHR, or XHR and images only.

Comment 1

[Victor Porof [:vporof][:vp]]

Yeah, this is pretty annoying. Boosting priority.

Comment 2

[Sami Jaktholm]

Attachment: Allow selection of multiple filters.

The patch contains following changes to the filters of netmonitor:
- Multiple filters can be simultaneously active.
- A request will be shown in the list if it matches any of the active filters.
- If an active filter is reselected, it'll be disabled (bug 876914).
  * However, 'all' doesn't toggle (though this would be quite easy to implement).
- If the last active filter is disabled, 'all' will be selected.
- When 'all' is selected other active filters will be disabled (and all request shown).

The patch also converts the old tests to fit the new functionality and adds few tests to test the new features.

Comment 3

[Victor Porof [:vporof][:vp]]

Comment on attachment 8369077 [details] [diff] [review]
Allow selection of multiple filters.

Review of attachment 8369077 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for contributing Sami! Things look very good to me, I don't have much to nitpick on :)

Please remove trailing or unnecessary whitespace introduced in this new code; if your editor supports automatic removal of trailing whitespace, it'd be great to enable it, otherwise you'll have to delete it manually. See below for a few extra comments, and please rebase this patch on fx-team tip, a few relatively large changes have landed to the network monitor, among which the introduction of an "other" filter.

Do the other tests still pass? Do you have access to the try servers? If not, I'll push the patch for you.

r+ with the comments below addressed.

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +528,5 @@
> +    if (aType !== "all" && this._activeFilters.indexOf("all") !== -1)
> +      this._disableFilter("all");    
> +  },
> +
> +  get _filterPredicate() {

Can you please also document this getter? A small comment describing its purpose will help when quickly glancing over the source.

@@ +537,5 @@
> +                            "xhr": this._onXhr,
> +                            "fonts": this._onFonts,
> +                            "images": this._onImages,
> +                            "media": this._onMedia,
> +                            "flash": this._onFlash};

Nit: please format this as:

let filterPredicates = {
  "all": () => true,
  "html": this._onHtml,
  "css": this._onCss,
  "js": this._onJs,
  "xhr": this._onXhr,
  "fonts": this._onFonts,
  "images": this._onImages,
  "media": this._onMedia,
  "flash": this._onFlash
};

to respect the code style in the rest of this particular file.

@@ +544,5 @@
> +       // The simplest case: only one filter active.
> +       return filterPredicates[this._activeFilters[0]];
> +     } else {
> +       // Multiple filters active.
> +       return (item) => {

Nit: please rename item to requestItem to make it clearer what it refers to.

::: browser/devtools/netmonitor/test/browser_net_filter-01.js
@@ +125,5 @@
> +     * Tests if a button for a filter of given type is the only one checked.
> +     *
> +     * @param string aFilterType
> +     *        The type of the filter that should be the only one checked.
> +     *

Nit: redundant extra * line.

@@ +145,5 @@
> +     * @param array aIsChecked
> +     *        An array specifying if a button at given index should have a 
> +     *        'checked' attribute. For example, if the third item of the array
> +     *        evaluates to true, the third button should be checked.
> +     *

Ditto.

Comment 4

[Sami Jaktholm]

Attachment: Allow selection of multiple filters, version 2.

An updated patch based on the fx-team branch with all comments addressed and updated to work with "other" filter too.

I don't have access to the try servers but when running the entire devtools test suite locally, there were 25 test failures with and without this patch so this should not cause any problems.

Comment 5

[Victor Porof [:vporof][:vp]]

Sounds good! I'll review this shortly.

Comment 6

[Victor Porof [:vporof][:vp]]

Comment on attachment 8372791 [details] [diff] [review]
Allow selection of multiple filters, version 2.

Review of attachment 8372791 [details] [diff] [review]:
-----------------------------------------------------------------

This looks marvelous! I only have a few very small nits that I'll just take care of myself before landing.

Thank you very much for contributing! Lots of people were wanting this.

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +625,5 @@
> +    target.setAttribute("checked", true);
> +
> +    // Check if 'all' was selected before. If so, disable it.
> +    if (aType !== "all" && this._activeFilters.indexOf("all") !== -1)
> +      this._disableFilter("all");

Nit: the prevalent style in this file is to always have curly braces.

@@ +654,5 @@
> +       // Multiple filters active.
> +       return ((requestItem) => {
> +         return this._activeFilters.some((filterName) => {
> +           return filterPredicates[filterName].call(this, requestItem);
> +         }, this);

Nit: I just saw this now, there's probably no need for a `this` argument here since you're using arrow functions.

@@ +655,5 @@
> +       return ((requestItem) => {
> +         return this._activeFilters.some((filterName) => {
> +           return filterPredicates[filterName].call(this, requestItem);
> +         }, this);
> +       })

Nit: missing a semicolon here; no worries, I'll add it before landing.

Comment 7

[Victor Porof [:vporof][:vp]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=0bded55d244a

Comment 8

[Victor Porof [:vporof][:vp]]

This looks green enough. Landing!

Comment 9

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/f5726434eedc

Comment 10

[Victor Porof [:vporof][:vp]]

Hey Sami, if you want to become a Mozillian on https://mozillians.org I can vouch for you! Thanks for all the good work.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f5726434eedc