1041895 - Filter Network requests by column values in Developer Tools

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect, P3)

Description

[Žilvinas]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/37.0.2062.20 Safari/537.36

Steps to reproduce:

I opened Firefox Dev Tools. Clicked on Network tab in Dev Tools. 


Actual results:

I only was able to filter out them by content type (HTML, CSS, XHR and etc.)


Expected results:

I should have been able to filter by the values in the columns such as Method, File, Domain, Size and etc.

Comment 1

[merzhin]

might be same as bug 1062851

Comment 2

[Žilvinas]

No, this is more about not having a feature to filter by column value, I can see columns properly.

Comment 3

[Matthew L Daniel]

Appears related to bug 923192

Comment 5

[Sebastian Zartner [:sebo]]

For what it's worth, the Chrome DevTools allow to search by specific options via search tags. E.g. to search for the HTTP status you enter "status-code:" and then the number.

Sebastian

Comment 6

[Sebastian Zartner [:sebo]]

With bug 862855 being implemented the UX for searching for a hidden info should be considered.
E.g. when the 'Method' column is hidden but you search for the method, the filtering might be confusing.

One way to make this obvious would be to automatically open the side panel and highlight the related information there and allow to toggle through the matches by pressing Enter similar to how it's proposed for responses in bug 1334408.

This might be done in a separate bug if needed.

Sebastian

Comment 7

[Tim Nguyen :ntim]

(In reply to Sebastian Zartner [:sebo] from comment #6)
> With bug 862855 being implemented the UX for searching for a hidden info
> should be considered.
> E.g. when the 'Method' column is hidden but you search for the method, the
> filtering might be confusing.

I don't see how it's confusing if you explicitly write: `method:get` in the search input (this is the UI I'm planning to implement btw, to be consistent with chrome).

Comment 8

[Sebastian Zartner [:sebo]]

Yeah, you're right. It should be clear to the users when they explicitly type 'method:get' that this filters the requests the HTTP method.

Two more points:
I believe the autocompletion should be available for both, the filter and the value, like it's done in Chrome.
And do you plan to add additional filters for information that is currently not visible within the columns in this bug, like e.g. the scheme, or special ones like 'is:from-cache'? Or will that be handled in bug 859047 or a another bug?

Sebastian

Comment 9

[Tim Nguyen :ntim]

(In reply to Sebastian Zartner [:sebo] from comment #8)
> Yeah, you're right. It should be clear to the users when they explicitly
> type 'method:get' that this filters the requests the HTTP method.
> 
> Two more points:
> I believe the autocompletion should be available for both, the filter and
> the value, like it's done in Chrome.
I don't plan to implement autocompletion initially. This can happen in a different bug.

> And do you plan to add additional filters for information that is currently
> not visible within the columns in this bug, like e.g. the scheme, or special
> ones like 'is:from-cache'? Or will that be handled in bug 859047 or a
> another bug?

I don't support everything from this list:

https://github.com/ChromeDevTools/devtools-frontend/blob/16e3b8c8a824315ac2b4c07ca4e4a01fc18f2cea/front_end/network/NetworkLogView.js#L1764

Here are the unsupported fields:
- cookie-related fields
- has-response-header
- mixed-content
- priority

Comment 10

[Tim Nguyen :ntim]

Attachment: Bug 1041895 - Add support for different flags with text filtering.

Review commit: https://reviewboard.mozilla.org/r/127586/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/127586/

Comment 11

[Tim Nguyen :ntim]

Comment on attachment 8855696 [details]
Bug 1041895 - Add support for different flags with text filtering.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127586/diff/1-2/

Comment 12

[Tim Nguyen :ntim]

Comment on attachment 8855696 [details]
Bug 1041895 - Add support for different flags with text filtering.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127586/diff/2-3/

Comment 13

[Tim Nguyen :ntim]

Comment on attachment 8855696 [details]
Bug 1041895 - Add support for different flags with text filtering.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127586/diff/3-4/

Comment 15

[Sebastian Zartner [:sebo]]

(In reply to Tim Nguyen :ntim from comment #9)
> (In reply to Sebastian Zartner [:sebo] from comment #8)
> > Yeah, you're right. It should be clear to the users when they explicitly
> > type 'method:get' that this filters the requests the HTTP method.
> > 
> > Two more points:
> > I believe the autocompletion should be available for both, the filter and
> > the value, like it's done in Chrome.
> I don't plan to implement autocompletion initially. This can happen in a
> different bug.

Ok, I've filed bug 1354504 for that. Though without any autocompletion users won't find out about this feature.

> > And do you plan to add additional filters for information that is currently
> > not visible within the columns in this bug, like e.g. the scheme, or special
> > ones like 'is:from-cache'? Or will that be handled in bug 859047 or a
> > another bug?
> 
> I don't support everything from this list:
> 
> https://github.com/ChromeDevTools/devtools-frontend/blob/
> 16e3b8c8a824315ac2b4c07ca4e4a01fc18f2cea/front_end/network/NetworkLogView.
> js#L1764
> 
> Here are the unsupported fields:
> - cookie-related fields
> - has-response-header
> - mixed-content
> - priority

Should I also file a bug for those?

Sebastian

Comment 16

[Tim Nguyen :ntim]

(In reply to Sebastian Zartner [:sebo] from comment #15)
> (In reply to Tim Nguyen :ntim from comment #9)
> > (In reply to Sebastian Zartner [:sebo] from comment #8)
> > > Yeah, you're right. It should be clear to the users when they explicitly
> > > type 'method:get' that this filters the requests the HTTP method.
> > > 
> > > Two more points:
> > > I believe the autocompletion should be available for both, the filter and
> > > the value, like it's done in Chrome.
> > I don't plan to implement autocompletion initially. This can happen in a
> > different bug.
> 
> Ok, I've filed bug 1354504 for that. Though without any autocompletion users
> won't find out about this feature.
Agreed, thanks for the bug.

> > > And do you plan to add additional filters for information that is currently
> > > not visible within the columns in this bug, like e.g. the scheme, or special
> > > ones like 'is:from-cache'? Or will that be handled in bug 859047 or a
> > > another bug?
> > 
> > I don't support everything from this list:
> > 
> > https://github.com/ChromeDevTools/devtools-frontend/blob/
> > 16e3b8c8a824315ac2b4c07ca4e4a01fc18f2cea/front_end/network/NetworkLogView.
> > js#L1764
> > 
> > Here are the unsupported fields:
> > - cookie-related fields
> > - has-response-header
> > - mixed-content
> > - priority

Priority should be solved by 1354054.
Feel free to file bugs for the others though.

Comment 18

[Sebastian Zartner [:sebo]]

(In reply to Tim Nguyen :ntim from comment #16)
> Priority should be solved by 1354054.
> Feel free to file bugs for the others though.

Ok, I've filed bugs for the others. Btw. what about the 'is' filter?

Sebastian

Comment 19

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks for working on this Tim, I like this feature!

@Ricky: can you please do the review?

Just one UX comment. Could we allow spaces between the colon and a value?

For example:
This works: method:GET
This doesn't: method: GET

It took me some time to figure out that the space is not allowed.

Honza

Comment 20

[Tim Nguyen :ntim]

(In reply to Jan Honza Odvarko [:Honza] from comment #19)
> Thanks for working on this Tim, I like this feature!
> 
> @Ricky: can you please do the review?
> 
> Just one UX comment. Could we allow spaces between the colon and a value?
> 
> For example:
> This works: method:GET
> This doesn't: method: GET
> 
> It took me some time to figure out that the space is not allowed.

I think this should be more obvious with autocomplete (bug 1354504), the reason I haven't allowed spaces is that it's not consistent with chrome's UX.

Comment 21

[Tim Nguyen :ntim]

(In reply to Sebastian Zartner [:sebo] from comment #18)
> (In reply to Tim Nguyen :ntim from comment #16)
> > Priority should be solved by 1354054.
> > Feel free to file bugs for the others though.
> 
> Ok, I've filed bugs for the others. Btw. what about the 'is' filter?

That's already supported with this patch.

Comment 22

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8855696 [details]
Bug 1041895 - Add support for different flags with text filtering.

https://reviewboard.mozilla.org/r/127586/#review130656

Nice job ntim. I noticed there are lots of new feature has added recently. This one is also a good feature everyone wants.

I've verified your patch on my local machine, and it works perfectly. I don't see any big issue here. Let's ship it!

::: commit-message-0895c:1
(Diff revision 4)
> +Bug 1041895 - Add support for different flags with text filtering. r=Honza, gerv

r=rchien

::: devtools/client/netmonitor/src/components/request-list-header.js:22
(Diff revision 4)
>  const WaterfallBackground = require("../waterfall-background");
>  const RequestListHeaderContextMenu = require("../request-list-header-context-menu");
>  
>  const { div, button } = DOM;
>  
> +const { HEADERS } = require("../constants");

nit: move this line before `const { L10N } = require("../utils/l10n");`

::: devtools/client/netmonitor/src/request-list-header-context-menu.js:11
(Diff revision 4)
>  
>  const Menu = require("devtools/client/framework/menu");
>  const MenuItem = require("devtools/client/framework/menu-item");
>  const { L10N } = require("./utils/l10n");
>  
> -const stringMap = {
> +const { HEADERS } = require("./constants");

nit: move this line before `const { L10N } = require("../utils/l10n");`

::: devtools/client/netmonitor/src/request-list-header-context-menu.js:13
(Diff revision 4)
> -};
> +const stringMap = HEADERS.filter(h => h.hasOwnProperty("label"))
> +                    .reduce((acc, h) => {
> +                      acc[h.name] = h.label;
> +                      return acc;
> +                    }, {});

nit: please fix code indentation and avoid to use one character to indicate variable.

How about using Object.assgin to rewrite this part? It makes code look more elegant and functional.

```
const stringMap = HEADERS
  .filter((header) => header.hasOwnProperty("label"))
  .reduce((acc, [name, label]) => Object.assign(acc, { [name]: label }), {});
```

::: devtools/client/netmonitor/src/utils/filter-predicates.js:119
(Diff revision 4)
>    flash: isFlash,
>    ws: isWS,
>    other: isOther,
>  };
>  
> -exports.isFreetextMatch = isFreetextMatch;
> +exports.isFreetextMatch = require("./filter-text-utils").isFreetextMatch;

nit: Let's do require("./filter-text-utils") at the top of file like:

```
const { isFreetextMatch } = require("./filter-text-utils");
```

and then export all properties and methods in one single object pattern:

```
modules.exports = {
  Filters = {
    all: all,
    html: isHtml,
    css: isCss,
    js: isJs,
    xhr: isXHR,
    fonts: isFont,
    images: isImage,
    media: isMedia,
    flash: isFlash,
    ws: isWS,
    other: isOther,
  },
  isFreetextMatch,
};
```

::: devtools/client/netmonitor/src/utils/filter-text-utils.js:34
(Diff revision 4)
> +const HEADER_FILTERS = HEADERS.filter(h => h.canFilter)
> +                              .map(h => h.filterKey || h.name);

nit: indent and some code style

```
const HEADER_FILTERS = HEADERS
  .filter((header) => header.canFilter)
  .map((header) => header.filterKey || header.name);

```

::: devtools/client/netmonitor/src/utils/filter-text-utils.js:46
(Diff revision 4)
> +  "is",
> +];
> +
> +/*
> +  Modified from:
> +  https://github.com/ChromeDevTools/devtools-frontend/blob/f340aefd7ec9b702de9366a812288cfb12111fce/front_end/network/FilterSuggestionBuilder.js#L138-L163

nit: this line exceeded 90 chars

Comment 23

[Tim Nguyen :ntim]

Comment on attachment 8855696 [details]
Bug 1041895 - Add support for different flags with text filtering.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127586/diff/4-5/

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/12b510985e17
Add support for different flags with text filtering. r=rickychien

Comment 25

[Sebastian Zartner [:sebo]]

(In reply to Tim Nguyen :ntim from comment #20)
> (In reply to Jan Honza Odvarko [:Honza] from comment #19)
> > Thanks for working on this Tim, I like this feature!
> > 
> > @Ricky: can you please do the review?
> > 
> > Just one UX comment. Could we allow spaces between the colon and a value?
> > 
> > For example:
> > This works: method:GET
> > This doesn't: method: GET
> > 
> > It took me some time to figure out that the space is not allowed.
> 
> I think this should be more obvious with autocomplete (bug 1354504), the
> reason I haven't allowed spaces is that it's not consistent with chrome's UX.

I think we don't have to align to Chrome's UX here. Allowing (optional) spaces between the colon and the value would be a UX improvement for users.
Having said that, personally I would expect it to work without space, as that's how it works in other search fields like on GitHub or formally on Google Code.

Sebastian

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/12b510985e17

Comment 27

[Sebastian Zartner [:sebo]]

Described the different filter flags in https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor#Filtering_by_properties and added a developer release note to https://developer.mozilla.org/en-US/Firefox/Releases/55#Developer_Tools.

Tim, can you please have a look whether the description is ok?

Sebastian

Comment 28

[Tim Nguyen :ntim]

(In reply to Sebastian Zartner [:sebo] from comment #27)
> Described the different filter flags in
> https://developer.mozilla.org/en-US/docs/Tools/
> Network_Monitor#Filtering_by_properties and added a developer release note
> to https://developer.mozilla.org/en-US/Firefox/Releases/55#Developer_Tools.
> 
> Tim, can you please have a look whether the description is ok?

It seems to be missing some information:
- You can use "-" for negative flag filtering, so "-is:cached" matches all requests that aren't cached
- remote-ip filter is missing
- For size and transferred, it takes the size order (round(log10(number)) rather than just the size, because it's hard to remember the exact size of a file
- the "k" multiplier also works for size and transferred
- the "m" multiplier is also supported for size/transferred/larger-than


Other than that, the docs looks great, thanks!

Comment 29

[Sebastian Zartner [:sebo]]

(In reply to Tim Nguyen :ntim from comment #28)
> (In reply to Sebastian Zartner [:sebo] from comment #27)
> > Described the different filter flags in
> > https://developer.mozilla.org/en-US/docs/Tools/
> > Network_Monitor#Filtering_by_properties and added a developer release note
> > to https://developer.mozilla.org/en-US/Firefox/Releases/55#Developer_Tools.
> > 
> > Tim, can you please have a look whether the description is ok?
> 
> It seems to be missing some information:
> - You can use "-" for negative flag filtering, so "-is:cached" matches all
> requests that aren't cached
> - remote-ip filter is missing
> - For size and transferred, it takes the size order (round(log10(number))
> rather than just the size, because it's hard to remember the exact size of a
> file
> - the "k" multiplier also works for size and transferred
> - the "m" multiplier is also supported for size/transferred/larger-than
> 
> 
> Other than that, the docs looks great, thanks!

Thank you for the fast reply! I've fixed the documentation accordingly.

The UX of the size and transferred filters feels a bit strange for me. Maybe that should be changed to mean digits, i.e. size:3 means everything between 100 and 999 bytes, or be replaced by a larger-than filter that matches the transferred size instead of the decompressed size, e.g. transferred-larger-than.

Sebastian

Comment 30

[Tim Nguyen :ntim]

(In reply to Sebastian Zartner [:sebo] from comment #29)
> (In reply to Tim Nguyen :ntim from comment #28)
> > (In reply to Sebastian Zartner [:sebo] from comment #27)
> > > Described the different filter flags in
> > > https://developer.mozilla.org/en-US/docs/Tools/
> > > Network_Monitor#Filtering_by_properties and added a developer release note
> > > to https://developer.mozilla.org/en-US/Firefox/Releases/55#Developer_Tools.
> > > 
> > > Tim, can you please have a look whether the description is ok?
> > 
> > It seems to be missing some information:
> > - You can use "-" for negative flag filtering, so "-is:cached" matches all
> > requests that aren't cached
> > - remote-ip filter is missing
> > - For size and transferred, it takes the size order (round(log10(number))
> > rather than just the size, because it's hard to remember the exact size of a
> > file
> > - the "k" multiplier also works for size and transferred
> > - the "m" multiplier is also supported for size/transferred/larger-than
> > 
> > 
> > Other than that, the docs looks great, thanks!
> 
> Thank you for the fast reply! I've fixed the documentation accordingly.
> 
> The UX of the size and transferred filters feels a bit strange for me. Maybe
> that should be changed to mean digits, i.e. size:3 means everything between
> 100 and 999 bytes, or be replaced by a larger-than filter that matches the
> transferred size instead of the decompressed size, e.g.
> transferred-larger-than.

Ah, sorry for the confusion, I actually meant it matches depending on the size order.

So: size:120 would match anything from 100 to 999. Same goes for transferred size.

I'm guessing the algorithm could be tweaked a bit to match only from 100 to 200 though in this case. What do you think ?

Comment 31

[Tim Nguyen :ntim]

> be replaced by a larger-than filter that matches the transferred size instead of the decompressed size, e.g. transferred-larger-than

A transferred-larger-than filter sounds like a good idea as well.

Comment 32

[Sebastian Zartner [:sebo]]

(In reply to Tim Nguyen :ntim (not accepting requests until 1st June) from comment #30)
> (In reply to Sebastian Zartner [:sebo] from comment #29)
> > (In reply to Tim Nguyen :ntim from comment #28)
> > > (In reply to Sebastian Zartner [:sebo] from comment #27)
> > > > Described the different filter flags in
> > > > https://developer.mozilla.org/en-US/docs/Tools/
> > > > Network_Monitor#Filtering_by_properties and added a developer release note
> > > > to https://developer.mozilla.org/en-US/Firefox/Releases/55#Developer_Tools.
> > > > 
> > > > Tim, can you please have a look whether the description is ok?
> > > 
> > > It seems to be missing some information:
> > > - You can use "-" for negative flag filtering, so "-is:cached" matches all
> > > requests that aren't cached
> > > - remote-ip filter is missing
> > > - For size and transferred, it takes the size order (round(log10(number))
> > > rather than just the size, because it's hard to remember the exact size of a
> > > file
> > > - the "k" multiplier also works for size and transferred
> > > - the "m" multiplier is also supported for size/transferred/larger-than
> > > 
> > > 
> > > Other than that, the docs looks great, thanks!
> > 
> > Thank you for the fast reply! I've fixed the documentation accordingly.
> > 
> > The UX of the size and transferred filters feels a bit strange for me. Maybe
> > that should be changed to mean digits, i.e. size:3 means everything between
> > 100 and 999 bytes, or be replaced by a larger-than filter that matches the
> > transferred size instead of the decompressed size, e.g.
> > transferred-larger-than.
> 
> Ah, sorry for the confusion, I actually meant it matches depending on the
> size order.
> 
> So: size:120 would match anything from 100 to 999. Same goes for transferred
> size.
> 
> I'm guessing the algorithm could be tweaked a bit to match only from 100 to
> 200 though in this case. What do you think ?

I've filed bug 1361480 to improve the algorithm, so we can discuss this separately.

(In reply to Tim Nguyen :ntim (not accepting requests until 1st June) from comment #31)
> > be replaced by a larger-than filter that matches the transferred size instead of the decompressed size, e.g. transferred-larger-than
> 
> A transferred-larger-than filter sounds like a good idea as well.

Great! I've filed bug 1361473 for it.

Sebastian

Comment 33

[Sajedul Islam]

I have reproduced this bug with Nightly 55.0a1 (2014-07-21) on Windows 8.1 , 64 Bit ! 

This bug's fix is Verified with latest Nightly 55.0a1 !

Build   ID    20170528030206
User Agent    Mozilla/5.0 (Windows NT 6.3; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170524]