Closed Bug 876913 Opened 7 years ago Closed 6 years ago

New Netmonitor filters are single-selection only

Categories

(DevTools :: Netmonitor, defect, P2)

defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 30

People

(Reporter: canuckistani, Assigned: sjakthol)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
OS: Mac OS X → All
Priority: -- → P3
Hardware: x86 → All
Yeah, this is pretty annoying. Boosting priority.
Priority: P3 → P2
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.
Attachment #8369077 - Flags: review?(vporof)
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.
Attachment #8369077 - Flags: review?(vporof) → review+
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.
Attachment #8369077 - Attachment is obsolete: true
Sounds good! I'll review this shortly.
Attachment #8372791 - Flags: review?(vporof)
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.
Attachment #8372791 - Flags: review?(vporof) → review+
This looks green enough. Landing!
Hey Sami, if you want to become a Mozillian on https://mozillians.org I can vouch for you! Thanks for all the good work.
https://hg.mozilla.org/mozilla-central/rev/f5726434eedc
Assignee: nobody → sjakthol
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
Duplicate of this bug: 876914
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.