Closed Bug 1419366 Opened 5 years ago Closed 5 years ago

NetMonitor: Stop using ImmutableJS in filters reducer

Categories

(DevTools :: Netmonitor, enhancement, P2)

Product:
DevTools
Component:
Netmonitor
Type:
enhancement

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: Honza, Assigned: abhinav.koppula, Mentored, NeedInfo)

References

Details

(Keywords: good-first-bug)

Attachments

(1 file)

Bug 1419366 - Stop using ImmutableJS in filters reducer.
59 bytes, text/x-review-board-request
Honza
: review+
Details 
ImmutableJS is slow and we should stop using it in NetMonitor's filters reducer.
devtools/client/netmonitor/src/reducers/filters.js 

It should be replaced by plain JS structures.

See bug 1408182 as an example.

Honza
Mentor: odvarko
Keywords: good-first-bug
Priority: -- → P2
Hi Jan, I'm free to take a look at this.
Cool! If you need any help, please let us know!
Assignee: nobody → mandercs3
Another example showing how to remove ImmutableJS is here: bug 1419369

Honza
@mandercs: Hi! Any progress on this bug?
Do you need help? It would be great to have this fixed
(it's the last piece related to ImmutableJS removal in the Net monitor)

Honza
@mandercs: Still interested in this bug?
Honza
Flags: needinfo?(mandercs3)
I am interested in this one. Can I go ahead ?
Flags: needinfo?(odvarko)
I was just changed I.Record in filters.js. but 
TypeError: this.panelWin.Netmonitor is undefined
Stack trace:
open@resource://devtools/shared/base-loader.js -> resource://devtools/client/netmonitor/panel.js:17:5
async*onLoad@resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js:1667:19
  Promise-backend.js:925

Error is thrown at console. So I reverted my changes and still this error is present. I checked hg status also, it does not show any changes regarding net-monitor. 

What mistake am I doing ? Is net-monitor already broken in current builds ? 

Thanks,
Avi
Flags: needinfo?(odvarko)
(In reply to avi.mathur.engg+github from comment #7)
> I was just changed I.Record in filters.js. but 
> TypeError: this.panelWin.Netmonitor is undefined
> Stack trace:
> open@resource://devtools/shared/base-loader.js ->
> resource://devtools/client/netmonitor/panel.js:17:5
> async*onLoad@resource://devtools/shared/base-loader.js ->
> resource://devtools/client/framework/toolbox.js:1667:19
>   Promise-backend.js:925
> 
> Error is thrown at console. So I reverted my changes and still this error is
> present. I checked hg status also, it does not show any changes regarding
> net-monitor. 
> 
> What mistake am I doing ? Is net-monitor already broken in current builds ? 
No, the Network panel is working properly.

Honza
I am trying to adapt it without immutable, but I am having a bit of trouble with understanding why "let { filter }" has the curly  brackets. Could you help me with that one?
Flags: needinfo?(odvarko)
Are you sure the original file is even working? It gives me a lot of error when ran and on top of that the last function should be  running at all. It has a predefined parameter as the first parameter. As far as I know that should trow an error, the predefined parameters need to be in the end of the function declaration.
(In reply to Kristiyan from comment #9)
> I am trying to adapt it without immutable, but I am having a bit of trouble
> with understanding why "let { filter }" has the curly  brackets. Could you
> help me with that one?

See this MDN page:
https://developer.mozilla.org/cs/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment

(In reply to Kristiyan from comment #10)
> Are you sure the original file is even working? It gives me a lot of error
> when ran and on top of that the last function should be  running at all. It
> has a predefined parameter as the first parameter. As far as I know that
> should trow an error, the predefined parameters need to be in the end of the
> function declaration.

Can you attach the error you are seeing?
Can you provide some more details about how do you run/build the net panel?

Honza
Flags: needinfo?(odvarko)
Hello, I would like to participate in this issue. On which line(s) of code are the errors present?
Flags: needinfo?(odvarko)
Hey Nathan, i'm trying to do it my self, but you are free to try as well. It's not about which lines, but rather to adapt filters.js in a way that it doesn't use immutable. We can collaborate on it if you wish, after i catch up on Honza's feedback i will upload my progress so you can take a look at it. Message me for more info if you need :).
Sorry, I am quite new to this. How do I run the net panel? I am not really sure how I am supposed to test.
Could you as well take a look at the last function with the default parameter, I can't see a way of this working, I spend a good 2 hours on trying to find if it's possible to have a default parameter and then an empty one, I dot see this(function filters(state = new Filters(), action)) happening and i don't want to switch them in case it works somehow and I mess it up.
@Kristiyan,
This is the net-monitor - https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor
How you run it - Open Firefox Nightly, press F12 and select "Network" tab
In the above link - check the "Filtering requests" section.

So after making changes to the filters reducer, you can check if filtering works fine even after your changes.
Hi Honza,
I have taken a stab at this issue. Please let me know if this looks fine.
Comment on attachment 8937364 [details]
Bug 1419366 - Stop using ImmutableJS in filters reducer.

https://reviewboard.mozilla.org/r/208044/#review214146

Thanks for working on this, please see my inline comments.

Honza

::: devtools/client/netmonitor/src/reducers/filters.js:21
(Diff revision 1)
> -  .reduce((o, tag) => Object.assign(o, { [tag]: false }), {})
> -);
> +  overrideParams = Object.keys(overrideParams)
> +    .filter(key => allFilterTypes.includes(key))
> +    .reduce((obj, key) => {
> +      obj[key] = overrideParams[key];
> +      return obj;
> +    }, {});

I don't see the filtering as part of the previous implementation, or was it done as part of ImmutableJS impl?

Is it kind of a safety-check for filtering-out obsolete filter IDs (coming from old prefs)? Can you creat some explanatory comments?

::: devtools/client/netmonitor/src/reducers/filters.js:23
(Diff revision 1)
> +    .reduce((obj, key) => {
> +      obj[key] = overrideParams[key];
> +      return obj;
> +    }, {});
> +  let filterTypes = allFilterTypes
> +  .reduce((o, tag) => Object.assign(o, { [tag]: false }), {});

This line needs indentation.

Also, why you are using an empty object as the third argument?

::: devtools/client/netmonitor/src/reducers/filters.js:56
(Diff revision 1)
> -  if (!newState.includes(true)) {
> +  for (const key in newState) {
> +    includesTrue = includesTrue || newState[key] === true;
> +  }
> +  if (!includesTrue) {
>      newState = new FilterTypes({ all: true });
>    }

Could we simplify the code above?

Remove the for-loop and use something like: `if (!newState.values().includes(true))` ...

::: devtools/client/netmonitor/src/selectors/requests.js:45
(Diff revision 1)
>    filters => r => {
> -    const matchesType = filters.requestFilterTypes.some((enabled, filter) => {
> -      return enabled && Filters[filter] && Filters[filter](r);
> -    });
> +    let matchesType = false;
> +    for (const key in filters.requestFilterTypes) {
> +      let enabled = filters.requestFilterTypes[key];
> +      matchesType = matchesType || (enabled && Filters[key] && Filters[key](r));
> +    }

Can we avoid for-loop and use rather something like: `filters.requestFilterTypes.values().some(...)`

::: devtools/client/netmonitor/src/selectors/requests.js:55
(Diff revision 1)
>  const getTypeFilterFn = createSelector(
>    state => state.filters,
>    filters => r => {
> -    const matchesType = filters.requestFilterTypes.some((enabled, filter) => {
> -      return enabled && Filters[filter] && Filters[filter](r);
> -    });
> +    let matchesType = false;
> +    for (const key in filters.requestFilterTypes) {
> +      let enabled = filters.requestFilterTypes[key];

The same as above, can we avoid for-loop...
Attachment #8937364 - Flags: review?(odvarko) → review-
(In reply to Nathan Zengamambu from comment #12)
> Hello, I would like to participate in this issue. On which line(s) of code
> are the errors present?
Nathan, Kristiyan, I am assigning this to Abhinav since he submitted a patch already, but ping me (email) if you are interested in another bug and I'll find something for you!
Honza
Assignee: mandercs3 → abhinav.koppula
Flags: needinfo?(odvarko)
Comment on attachment 8937364 [details]
Bug 1419366 - Stop using ImmutableJS in filters reducer.

https://reviewboard.mozilla.org/r/208044/#review214146

> I don't see the filtering as part of the previous implementation, or was it done as part of ImmutableJS impl?
> 
> Is it kind of a safety-check for filtering-out obsolete filter IDs (coming from old prefs)? Can you creat some explanatory comments?

Yes, this was taken care of as part of ImmutableJS impl, since I had to add this piece of code as the below test was failing with the following failures:

TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_filter-04.js | The  button should not have a 'checked' class. - Got true, expected false
TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_filter-04.js | The  button should set 'aria-pressed' = false. - Got true, expected false
TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_filter-04.js | The  button should not have a 'checked' class. - Got true, expected false
TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_filter-04.js | The  button should set 'aria-pressed' = false. - Got true, expected false
TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_filter-04.js | The filters preferences were saved directly after the click and only with the valid. - Got ["html","js","bogus","alsobogus"], expected ["html","js"]

The above piece of code was added more specifically to ensure that this test passes as before.
Comment on attachment 8937364 [details]
Bug 1419366 - Stop using ImmutableJS in filters reducer.

https://reviewboard.mozilla.org/r/208044/#review214146

> This line needs indentation.
> 
> Also, why you are using an empty object as the third argument?

Reg the need for the empty object as third argument: 
So I was keeping it similar to what it was before. Before my change we had:
const FilterTypes = I.Record(["all"]
  .concat(FILTER_TAGS)
  .reduce((o, tag) => Object.assign(o, { [tag]: false }), {})
);
Also, if I omit the third parameter of empty object, it results in this -> https://snag.gy/HEePxT.jpg
Your thoughts on this?
Comment on attachment 8937364 [details]
Bug 1419366 - Stop using ImmutableJS in filters reducer.

https://reviewboard.mozilla.org/r/208044/#review214448

(In reply to Abhinav Koppula from comment #22)
> Reg the need for the empty object as third argument: 
> So I was keeping it similar to what it was before. Before my change we had:
> const FilterTypes = I.Record(["all"]
>   .concat(FILTER_TAGS)
>   .reduce((o, tag) => Object.assign(o, { [tag]: false }), {})
> );
> Also, if I omit the third parameter of empty object, it results in this ->
> https://snag.gy/HEePxT.jpg
> Your thoughts on this?

Ah, I overlooked a bracket, it's the initial value for `reduce` accumulator,
not third arg for `Object.assign`, so it's correct!

R+

Thanks!
Honza
Attachment #8937364 - Flags: review?(odvarko) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/de10180be095
Stop using ImmutableJS in filters reducer. r=Honza
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/de10180be095
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.