Žilvinas Reporter
	Description • 10 years ago

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.

	merzhin
	Comment 1 • 10 years ago

might be same as bug 1062851

	Žilvinas Reporter
	Comment 2 • 10 years ago

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

	Matthew L Daniel
	Comment 3 • 9 years ago

Appears related to bug 923192

	Sebastian Zartner [:sebo]
	Comment 5 • 7 years ago

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

	Sebastian Zartner [:sebo]
	Comment 6 • 7 years ago

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

	Tim Nguyen :ntim Assignee
	Comment 7 • 7 years ago

(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).

	Sebastian Zartner [:sebo]
	Comment 8 • 7 years ago

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

	Tim Nguyen :ntim Assignee
	Comment 9 • 7 years ago

(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

	Sebastian Zartner [:sebo]
	Comment 15 • 7 years ago

(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

	Tim Nguyen :ntim Assignee
	Comment 16 • 7 years ago

(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.

	Sebastian Zartner [:sebo]
	Comment 18 • 7 years ago

(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

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 19 • 7 years ago

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

	Tim Nguyen :ntim Assignee
	Comment 20 • 7 years ago

(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.

	Tim Nguyen :ntim Assignee
	Comment 21 • 7 years ago

(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.

	Ricky Chien [:rickychien] (inactive)
	Comment 22 • 7 years ago
mozreview-review

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

	Pulsebot
	Comment 24 • 7 years ago

Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/12b510985e17
Add support for different flags with text filtering. r=rickychien

	Sebastian Zartner [:sebo]
	Comment 25 • 7 years ago

(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

	Carsten Book [:Tomcat]
	Comment 26 • 7 years ago
bugherder

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

	Sebastian Zartner [:sebo]
	Comment 27 • 7 years ago

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

	Tim Nguyen :ntim Assignee
	Comment 28 • 7 years ago

(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!

	Sebastian Zartner [:sebo]
	Comment 29 • 7 years ago

(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

	Tim Nguyen :ntim Assignee
	Comment 30 • 7 years ago

(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 ?

	Tim Nguyen :ntim Assignee
	Comment 31 • 7 years ago

> 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.

	Sebastian Zartner [:sebo]
	Comment 32 • 7 years ago

(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

	Sajedul Islam
	Comment 33 • 7 years ago

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]