892229 - Ctrl+F / Cmd+F should search/filter requests

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

We should have a search box that allows you to find and filter the requests visible in the network monitor.

Comment 1

[Aaron Graham [:agraham]]

I really wanted this feature so I thought I would take a crack at it. 

I've never contributed before so I expect there will be a lot of changes to what I have done so far. I would love to keep working on it though if somebody is able to review what I have done and point me in the right direction. 

My questions so far are: 

* Is my test coverage good enough?
* Is there a better way of working around the UI time out other than calling the callbacks manually
* Is the 0 width flex spacer okay? I couldn't find a better way of drawing the dividing line in the UI

Thank you in advance for you time taken to look at this.

Comment 2

[Aaron Graham [:agraham]]

Attachment: network_text_filter.patch

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Flagging Victor to take a look.

Comment 4

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

Comment on attachment 8584537 [details] [diff] [review]
network_text_filter.patch

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

This is very clean! Wonderful job!

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +691,5 @@
> +
> +    if (this._currentFreetextFilter.length === 0) {
> +      this.freetextFilterBox.removeAttribute("filled");
> +    } else {
> +      this.freetextFilterBox.setAttribute("filled", true);

I think there might be a better way to check if there's text available inside a filter box, directly in css, possibly using the "value" attribute, but I'm not sure. Less code is better, so might be worth looking into this.

@@ +694,5 @@
> +    } else {
> +      this.freetextFilterBox.setAttribute("filled", true);
> +    }
> +
> +    this.userInputTimer.initWithCallback(this.reFilterRequests, FREETEXT_FILTER_SEARCH_DELAY, Ci.nsITimer.TYPE_ONE_SHOT)

Nit: add a ;

@@ +812,5 @@
> +      // The simplest case: only one filter active.
> +      return requestItem => {
> +        return filterPredicates[this._activeFilters[0]].call(this, requestItem) &&
> +                filterPredicates["freetext"].call(this, requestItem, currentFreetextFilter);
> +      }

Nit: add a ;

@@ +821,5 @@
> +          return filterPredicates[filterName].call(this, requestItem) &&
> +                  filterPredicates["freetext"].call(this, requestItem, currentFreetextFilter);
> +        });
> +      };
> +    }

I think both these if/else conditional branches could be merged into a single one. Why special case the "one filter active" branch?

@@ +997,5 @@
>    isOther: function(e)
>      !this.isHtml(e) && !this.isCss(e) && !this.isJs(e) && !this.isXHR(e) &&
>      !this.isFont(e) && !this.isImage(e) && !this.isMedia(e) && !this.isFlash(e),
>  
> +  isFreetext: function({ attachment: { url } }, text) //no text is a positive match

Nit: isFreetextMatch might be more descriptive.

::: browser/devtools/netmonitor/netmonitor.xul
@@ +770,5 @@
> +        <textbox id="requests-menu-filter-freetext-text"
> +                 class="requests-menu-footer-textbox devtools-searchinput"
> +                 type="search"
> +                 placeholder="&netmonitorUI.footer.filterFreetext.label;"
> +            />

Nit: weirdly placed />

::: browser/themes/shared/devtools/netmonitor.inc.css
@@ +701,5 @@
> +#requests-menu-filter-freetext-text {
> +    transition-property: max-width, -moz-padding-end, -moz-padding-start;
> +    transition-duration: 250ms;
> +    transition-timing-function: ease;
> +}

Nit: css has 2 spaces indentation.

Comment 5

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

Related uservoice: https://ffdevtools.uservoice.com/forums/246087-firefox-developer-tools-ideas/suggestions/6656793-ability-to-filter-network-tab-data

Comment 6

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

Aaron, can you update the patch with the comments addressed so we can land this?
Thanks!

Comment 7

[Aaron Graham [:agraham]]

Sorry, I have been away this bank holiday weekend without access to my laptop. I should be able to get this cleaned up and submit an updated patch on Monday. Thanks.

Comment 8

[Aaron Graham [:agraham]]

Attachment: Bug892229-v2.patch

I have made the changes suggested, except for discovering if the box is empty or not in CSS. As far as I can find out it is not possible to do this. I have tried searching the project to see if I can find something Firefox specific there but cannot. 

Would love to be proved wrong though if you have an example? 

I have attached an updated patch. 

Thanks again for your time on this. I would be very excited to get my first patch in!

Comment 9

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

Comment on attachment 8588791 [details] [diff] [review]
Bug892229-v2.patch

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

Land away!

Comment 10

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

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=be748c9a6fe0

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Try run looks good to me...  I'd say it's safe to land.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/30eef7948c54