Closed Bug 1323933 Opened 5 years ago Closed 5 years ago

rename filter to requestFilter

Categories

(DevTools :: Netmonitor, defect, P1)

defect

Tracking

(firefox53 fixed)

RESOLVED FIXED
Firefox 53
Iteration:
53.4 - Jan 9
Tracking Status
firefox53 --- fixed

People

(Reporter: gasolin, Assigned: gasolin, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [netmonitor])

Attachments

(1 file)

(quote from bug 1317649 comment 25)
After introducing a new cookies filter in filters.js, I think it makes sense to change the generic filter name to requestFilter since it would be more easy to distinguish.

setFilterText -> setRequestFilterText
toggleFilterType -> toggleRequestFilterType
enableFilterTypeOnly -> enableRequestFilterTypeOnly
FilterTypes -> RequestFilterTypes
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Comment on attachment 8819265 [details]
Bug 1323933 - rename filter to requestFilter;

https://reviewboard.mozilla.org/r/99114/#review99422

::: devtools/client/netmonitor/reducers/filters.js:29
(Diff revision 1)
>    ws: false,
>    other: false,
>  });
>  
>  const Filters = I.Record({
> -  types: new FilterTypes({ all: true }),
> +  requestTypes: new FilterTypes({ all: true }),

nit: requestFilterTypes

::: devtools/client/netmonitor/reducers/filters.js:30
(Diff revision 1)
>    other: false,
>  });
>  
>  const Filters = I.Record({
> -  types: new FilterTypes({ all: true }),
> -  text: "",
> +  requestTypes: new FilterTypes({ all: true }),
> +  requestText: "",

nit: requestFilterText
Whiteboard: [netmonitor] → [netmonitor] [triage]
Comment on attachment 8819265 [details]
Bug 1323933 - rename filter to requestFilter;

https://reviewboard.mozilla.org/r/99114/#review99646

LGTM for naming changes. Only one comment needs to be addressed, please fix it and assume try is green.

::: devtools/client/netmonitor/reducers/filters.js:14
(Diff revision 2)
> -  TOGGLE_FILTER_TYPE,
> -  ENABLE_FILTER_TYPE_ONLY,
> -  SET_FILTER_TEXT,
> +  TOGGLE_REQUEST_FILTER_TYPE,
> +  ENABLE_REQUEST_FILTER_TYPE_ONLY,
> +  SET_REQUEST_FILTER_TEXT,
>  } = require("../constants");
>  
>  const FilterTypes = I.Record({

FilterTypes -> RequestFilterTypes
Attachment #8819265 - Flags: review?(rchien) → review+
Iteration: --- → 53.3 - Dec 26
Flags: qe-verify-
Priority: -- → P1
Whiteboard: [netmonitor] [triage] → [netmonitor]
Comment on attachment 8819265 [details]
Bug 1323933 - rename filter to requestFilter;

https://reviewboard.mozilla.org/r/99114/#review99862

Looks good, r+ if try is green.

::: devtools/client/netmonitor/components/filter-buttons.js:15
(Diff revision 2)
> -  filterTypes,
> +  requestFilterTypes,
>    triggerFilterType,

The "triggerFilterType" name looks like a typo - shouldn't it be "toggleFilterType"? Not really related to this bug, but would be a very useful drive-by fix.

Or "toggleRequestFilterType", to be consistent.

Or maybe the names in this component don't need the explicit "request" prefix and can remain "filterTypes" and "toggleFilterType". You decide.
Attachment #8819265 - Flags: review?(jsnajdr) → review+
renamed to `toggleRequestFilterType`, try is green

thanks!
there seems to be some issues in mozreview, can you fix this so that we can use the autolander, thanks!
Flags: needinfo?(gasolin)
Keywords: checkin-needed
Thanks for notice. I've rebased PR to inbound and mark issues as fixed
Flags: needinfo?(gasolin)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/188866c51253
rename filter to requestFilter; r=jsnajdr,rickychien
Keywords: checkin-needed
Iteration: 53.3 - Dec 26 → 53.4 - Jan 9
https://hg.mozilla.org/mozilla-central/rev/188866c51253
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.