Closed Bug 892229 Opened 7 years ago Closed 5 years ago

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

Categories

(DevTools :: Netmonitor, defect, P1)

defect

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: fitzgen, Assigned: agraham)

References

(Blocks 1 open bug)

Details

(Whiteboard: [polish-backlog])

Attachments

(1 file, 1 obsolete file)

We should have a search box that allows you to find and filter the requests visible in the network monitor.
Depends on: 859047
OS: Mac OS X → All
Priority: -- → P3
Hardware: x86 → All
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.
Attached patch network_text_filter.patch (obsolete) — Splinter Review
Flagging Victor to take a look.
Flags: needinfo?(vporof)
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.
Flags: needinfo?(vporof)
Attachment #8584537 - Flags: review+
Aaron, can you update the patch with the comments addressed so we can land this?
Thanks!
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.
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!
Attachment #8584537 - Attachment is obsolete: true
Attachment #8588791 - Flags: review?(vporof)
Comment on attachment 8588791 [details] [diff] [review]
Bug892229-v2.patch

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

Land away!
Attachment #8588791 - Flags: review?(vporof) → review+
Try run looks good to me...  I'd say it's safe to land.
Keywords: checkin-needed
Priority: P3 → P1
https://hg.mozilla.org/integration/fx-team/rev/30eef7948c54
Assignee: nobody → agraham19
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [devedition-40] → [devedition-40][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/30eef7948c54
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [devedition-40][fixed-in-fx-team] → [devedition-40]
Target Milestone: --- → Firefox 40
Whiteboard: [devedition-40] → [polish-backlog]
No longer depends on: 859047
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.