Closed Bug 1309192 Opened 8 years ago Closed 8 years ago

Implement search filter in Net Panel Toolbar

Categories

(DevTools :: Netmonitor, defect, P1)

Product:
DevTools
Component:
Netmonitor
Type:
defect

Tracking

(firefox52 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 52
Iteration:
52.3 - Nov 14
Tracking Flags:
Tracking Status
firefox52 --- verified

People

(Reporter: steveck, Assigned: rickychien)

References

(Blocks 1 open bug)

Details

(Whiteboard: [netmonitor])

Attachments

(1 file)

Bug 1309192 - Implement search filter in Net Panel Toolbar
58 bytes, text/x-review-board-request
jsnajdr
: review+
Details 
A subitem for Bug #1308426 that need to implement search filter in Net Panel Toolbar
Whiteboard: [netmonitor]
Flags: qe-verify+
QA Contact: ciprian.georgiu
Note that bug 1154789 will introduce shared Search box React component and using this component in the Netmonitor (this report) should fix bug 1268444.

Honza
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Priority: -- → P1
Assignee: gasolin → rchien
Iteration: --- → 52.3 - Nov 7
Update: I rebased bug 1309191 and have been working on this for a while. 

I feel confortable to use shared Search box React component, it's really easy to migrate old filter input to react. And WIP patch is ready in my local environment, everything works great!

Attach try log for WIP patch:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4c0dcd8a5f69d03008f182d2f126b86e51ac1e2
I rebased to m-c and fixed conflicts. 

Jarda, please take a look for that patch. I renamed the action types for distinct `filter type` and `filter url`.
Comment on attachment 8806587 [details]
Bug 1309192 - Implement search filter in Net Panel Toolbar

https://reviewboard.mozilla.org/r/89988/#review89512

Looks good! I just have a few style and naming nits.

Warning: you'll need to rebase this patch on top of bug 1309193 (sidebar toggle button) before landing. At this moment, the sidebar patch is landed in autoland, but not yet merged to m-c.

::: devtools/client/netmonitor/components/search-box.js:13
(Diff revision 2)
> +const { L10N } = require("../l10n");
> +const Actions = require("../actions/index");
> +
> +module.exports = connect(
> +  (state) => ({
> +    delay: 200,

Define a constant (FREETEXT_FILTER_SEARCH_DELAY).

::: devtools/client/netmonitor/constants.js:9
(Diff revision 2)
>  "use strict";
>  
>  const actionTypes = {
> -  TOGGLE_FILTER: "TOGGLE_FILTER",
> -  ENABLE_FILTER_ONLY: "ENABLE_FILTER_ONLY",
> +  TOGGLE_FILTER_TYPE: "TOGGLE_FILTER_TYPE",
> +  ENABLE_FILTER_TYPE_ONLY: "ENABLE_FILTER_TYPE_ONLY",
> +  UPDATE_FILTER_URL: "UPDATE_FILTER_URL",

Nit: I'd call this UPDATE_FILTER_TEXT (or SET_FILTER_TEXT). Right now, one can filter only by the URL text, but in near future, we might have a more rich syntax for text searching. Like "is:inprogress" etc. Chrome can already do it. So let's use more general name.

::: devtools/client/netmonitor/reducers/filters.js:13
(Diff revision 2)
> -  TOGGLE_FILTER,
> -  ENABLE_FILTER_ONLY,
> +  TOGGLE_FILTER_TYPE,
> +  ENABLE_FILTER_TYPE_ONLY,
> +  UPDATE_FILTER_URL,
>  } = require("../constants");
>  
>  const FiltersTypes = I.Record({

Nit: I'd rename this to "FilterTypes", to have more consistent naming. I think Honza mentioned this somewhere, too.

::: devtools/client/netmonitor/reducers/filters.js:68
(Diff revision 2)
>  
>    return new FiltersTypes({ [filter]: true });
>  }
>  
>  function filters(state = new Filters(), action) {
>    let types;

Style nit: This is a variable local to a given "case" block, and yet it's defined for the whole function. Consider using a block:

case "X": {
  let types = ...
  ...
}

or getting rid of the variable:

return state.set("types", toggle(...));

::: devtools/client/netmonitor/test/browser_net_filter-01.js:35
(Diff revision 2)
>      // Set the text and manually call all callbacks synchronously to avoid the timeout
> -    RequestsMenu.freetextFilterBox.value = value;
> +    RequestsMenu._currentFreetextFilter = value;
> -    RequestsMenu.requestsFreetextFilterEvent();
> -    RequestsMenu.userInputTimer.cancel();
>      RequestsMenu.reFilterRequests();

Or better, just dispatch the UPDATE_FILTER_TEXT action here. "reFilterRequests" will be done automatically as a response to the state change.

The comment about calling calbacks sync etc is no longer relevant.

::: devtools/client/shared/components/search-box.js:47
(Diff revision 2)
>        this.refs.input.focus();
>      });
>    },
>  
>    componentWillUnmount() {
> -    this.shortcuts.destroy();
> +    this.shortcuts && this.shortcuts.destroy();

Good catch!
Attachment #8806587 - Flags: review?(jsnajdr) → review+
Comment on attachment 8806587 [details]
Bug 1309192 - Implement search filter in Net Panel Toolbar

https://reviewboard.mozilla.org/r/89988/#review89678

::: devtools/client/shared/components/search-box.js:47
(Diff revision 3)
>        this.refs.input.focus();
>      });
>    },
>  
>    componentWillUnmount() {
> -    this.shortcuts.destroy();
> +    this.shortcuts && this.shortcuts.destroy();

Thanks for fixing this!

Can you use 
if (this.shortcuts) {
  this.shortcuts.destroy();
}
instead so it's more obvious what we're doing here?
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/62d86ba4add9
Implement search filter in Net Panel Toolbar r=jsnajdr
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/62d86ba4add9
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Blocks: 1309496
Depends on: 1315224
Depends on: 1315635
This issue is verified fixed on latest Nightly 52.0a1 (Build ID 20161108030212) under the following OSes:
- Windows 10 x64
- Mac OS X 10.11
- Ubuntu 14.04 x86 LTS
I confirm that Search filter in Net Panel Toolbar is working as expected. Marking here accordingly.
Status: RESOLVED → VERIFIED
status-firefox52: fixed → verified
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: