Closed Bug 1419369 Opened 7 years ago Closed 7 years ago

NetMonitor: Stop using ImmutableJS in ui 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: miller.time.baby, Mentored)

References

Details

(Whiteboard: good-first-bug)

Attachments

(1 file)

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

It should be replaced by plain JS structures.

See bug 1408182 as an example.

Honza
Mentor: odvarko
Priority: -- → P2
Whiteboard: good-first-bug
I'd like to start working on this bug
I'm curious what the preference is for replacing `Immutable.is` (as seen here: https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/RequestListItem.js#100).

If I'm reading the Immutable docs correctly, we'll need to do a deep equality check of these columns (which are now plain objects).

Lodash has an `isEqual` function that accomplishes this, but maybe we don't need the new dependency if we can leverage existing functionality?
Flags: needinfo?(odvarko)
I pushed up what I've got so far. There are some failures when I run the tests locally, but I wasn't sure how to resolve them. Looking for some guidance please :)

Thanks
Assignee: nobody → miller.time.baby
Status: NEW → ASSIGNED
Flags: needinfo?(odvarko)
Comment on attachment 8931832 [details]
Bug 1419369 - NetMonitor: Stop using ImmutableJS in ui reducer;

https://reviewboard.mozilla.org/r/202950/#review208444

Thanks for working on this!

Please see my inline comment

Also, the following STR are failing for me:
1) Open the toolbox, select the Net panel
2) Hide a columns, reopen the Toolbox
3) The column should stay hidden, it isn't in my case -> BUG

---

Try push with the current patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=63b93b194bb21a550126d32d964e6e12c51550b5

Honza

::: devtools/client/netmonitor/test/browser_net_columns_pref.js:35
(Diff revision 1)
> -    .includes("contentSize"), "Pref should be synced for contentSize");
> +    "Pref should be synced for contentSize");
>  
>    yield showColumn("status");
>  
> -  ok(Services.prefs.getCharPref("devtools.netmonitor.visibleColumns").includes("status"),
> +  ok(Services.prefs.getCharPref("devtools.netmonitor.visibleColumns").status,
> -  "Pref should be synced for status");
> +    "Pref should be synced for status");

The value returned from `getCharPref` is a string. It should be parsed (using JSON.parse) to an array and the `status` column visibility checked using `include`.

The test fails for me when running locally:

INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_columns_pref.js | Pref should be synced for status -
Stack trace:
    chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_columns_pref.js:null:36
    
But, you might use `Prefs.visibleColumns`, which is an array.
Attachment #8931832 - Flags: review?(odvarko) → review-
(In reply to Russell from comment #4)
> I pushed up what I've got so far. There are some failures when I run the
> tests locally, but I wasn't sure how to resolve them. Looking for some
> guidance please :)
See my comment related to the return value of `getCharPref` 
(it seems that the test was wrong even before)

Honza
(In reply to Russell from comment #2)
> I'm curious what the preference is for replacing `Immutable.is` (as seen
> here:
> https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/
> components/RequestListItem.js#100).
> 
> If I'm reading the Immutable docs correctly, we'll need to do a deep
> equality check of these columns (which are now plain objects).
> 
> Lodash has an `isEqual` function that accomplishes this, but maybe we don't
> need the new dependency if we can leverage existing functionality?
Yes, we don't need Lodash. Your patch is going in the right direction.

Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #6)
> See my comment related to the return value of `getCharPref` 
> (it seems that the test was wrong even before)

Thanks for the suggestion. I pushed to review with your suggestion of `Prefs.visibleColumns.includes(...)`

Can you help me figure out why a bunch of tests (browser_net_api-calls.js, browser_net_brotli.js, browser_net_cached-status.js, browser_net_cause.js) are getting the error `TypeError: target.querySelector(...) is null` ?
Flags: needinfo?(odvarko)
Comment on attachment 8931832 [details]
Bug 1419369 - NetMonitor: Stop using ImmutableJS in ui reducer;

https://reviewboard.mozilla.org/r/202950/#review208944

> Can you help me figure out why a bunch of tests (browser_net_api-calls.js,
> browser_net_brotli.js, browser_net_cached-status.js, browser_net_cause.js)
> are getting the error `TypeError: target.querySelector(...) is null` ?

See my inline comments

Honza

::: devtools/client/netmonitor/src/reducers/ui.js:53
(Diff revision 2)
>      RESPONSE_HEADERS.reduce((acc, header) => Object.assign(acc, { [header]: false }), {})
> -  )
> -);
> +  );
> +}
>  
> -const UI = I.Record({
> +function UI() {

create-store.js passes default 'initialState' into the UI() function, but I don't see any arguments here.

1) Create 'initialState' argument
2) Merge with the return value using spread operator.

::: devtools/client/netmonitor/src/reducers/ui.js:62
(Diff revision 2)
> -  networkDetailsOpen: false,
> +    networkDetailsOpen: false,
> -  persistentLogsEnabled: Services.prefs.getBoolPref("devtools.netmonitor.persistlog"),
> +    persistentLogsEnabled: Services.prefs.getBoolPref("devtools.netmonitor.persistlog"),
> -  browserCacheDisabled: Services.prefs.getBoolPref("devtools.cache.disabled"),
> +    browserCacheDisabled: Services.prefs.getBoolPref("devtools.cache.disabled"),
> -  statisticsOpen: false,
> +    statisticsOpen: false,
> -  waterfallWidth: null,
> +    waterfallWidth: null,
> -});
> +  };

Merge here:

```
  waterfallWidth: null,
  ...initialState,
};
```

::: devtools/client/netmonitor/src/utils/create-store.js:66
(Diff revision 2)
> -    columns = columns.withMutations((state) => {
> -      state.set(col, visibleColumns.includes(col));
> +  for (let col in columns) {
> +    state[col] = visibleColumns.includes(col);
> -    });
>    }
>  
>    return columns;

You should return `state`
Attachment #8931832 - Flags: review?(odvarko) → review-
Flags: needinfo?(odvarko)
Thanks! Nice catch, I totally missed both of those things :(
(In reply to Russell from comment #12)
> Thanks! Nice catch, I totally missed both of those things :(

Thanks for the patch update!

There are still some test failures:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=345abb6fda6d&selectedJob=148216445
Can you please look at that?

Honza
Flags: needinfo?(miller.time.baby)
(In reply to Jan Honza Odvarko [:Honza] from comment #13)
> There are still some test failures:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=345abb6fda6d&selectedJob=148216445
> Can you please look at that?

Ah, I see what I did wrong here... I tried to remove what looked like a duplicate call to `store.getState().ui.columns` but that broke the test. As far as I can tell, this test is going through each column one by one and hiding it.

Unfortunately, for some reason now the browser_net_columns_pref.js test is failing again. :( It seems like `Prefs.visibleColumns` isn't getting updated after hiding the "status" column? Is there a way to refresh that?
Flags: needinfo?(miller.time.baby) → needinfo?(odvarko)
(In reply to Russell from comment #15)
> Unfortunately, for some reason now the browser_net_columns_pref.js test is
> failing again. :( It seems like `Prefs.visibleColumns` isn't getting updated
> after hiding the "status" column? Is there a way to refresh that?

What happens if you use (properly) Services.prefs.getCharPref("devtools.netmonitor.visibleColumns")?

Honza
Flags: needinfo?(odvarko)
(In reply to Jan Honza Odvarko [:Honza] from comment #16)
> What happens if you use (properly)
> Services.prefs.getCharPref("devtools.netmonitor.visibleColumns")?

Oh yeah! I forgot you had mentioned:

> The value returned from `getCharPref` is a string. It should be parsed (using JSON.parse) to an array and the `status` column visibility checked using `include`.
> But, you might use `Prefs.visibleColumns`, which is an array.

Sure enough the first option was the right one :)
Comment on attachment 8931832 [details]
Bug 1419369 - NetMonitor: Stop using ImmutableJS in ui reducer;

https://reviewboard.mozilla.org/r/202950/#review210098

Looks good to me, just one inline comment.

R+

Honza

::: devtools/client/netmonitor/test/browser_net_columns_pref.js:18
(Diff revision 5)
>  
>    let { monitor } = yield initNetMonitor(SIMPLE_URL);
>    info("Starting test... ");
>  
> -  let { document, parent } = monitor.panelWin;
> +  let { document, parent, windowRequire } = monitor.panelWin;
> +  let { Prefs } = windowRequire("devtools/client/netmonitor/src/utils/prefs");

Prefs is never used, please remove it.
Attachment #8931832 - Flags: review?(odvarko) → review+
(In reply to Jan Honza Odvarko [:Honza] from comment #19)
> Prefs is never used, please remove it.

Whoops! I ran eslint a couple days ago and saw that, amended my commit, and then forgot to push :( But it's pushed now!
Attachment #8931832 - Flags: review+ → review?(odvarko)
Comment on attachment 8931832 [details]
Bug 1419369 - NetMonitor: Stop using ImmutableJS in ui reducer;

https://reviewboard.mozilla.org/r/202950/#review210282

::: devtools/client/netmonitor/src/components/RequestListItem.js:99
(Diff revision 6)
>    }
>  
>    shouldComponentUpdate(nextProps) {
>      return !propertiesEqual(UPDATED_REQ_ITEM_PROPS, this.props.item, nextProps.item) ||
>        !propertiesEqual(UPDATED_REQ_PROPS, this.props, nextProps) ||
> -      !I.is(this.props.columns, nextProps.columns);
> +      this.props.columns !== nextProps.columns;

Objects are compared by reference, not by value. So this will not work (it will always return true).

You can create a function that checks whether all elements in the object are the same as all the elements in the other object.
Attachment #8931832 - Flags: review-
(In reply to Tim Nguyen :ntim from comment #22)
> Comment on attachment 8931832 [details]
> Bug 1419369 - NetMonitor: Stop using ImmutableJS in ui reducer;
> 
> https://reviewboard.mozilla.org/r/202950/#review210282
> 
> ::: devtools/client/netmonitor/src/components/RequestListItem.js:99
> (Diff revision 6)
> >    }
> >  
> >    shouldComponentUpdate(nextProps) {
> >      return !propertiesEqual(UPDATED_REQ_ITEM_PROPS, this.props.item, nextProps.item) ||
> >        !propertiesEqual(UPDATED_REQ_PROPS, this.props, nextProps) ||
> > -      !I.is(this.props.columns, nextProps.columns);
> > +      this.props.columns !== nextProps.columns;
> 
> Objects are compared by reference, not by value. So this will not work (it
> will always return true).
> 
> You can create a function that checks whether all elements in the object are
> the same as all the elements in the other object.

The columns array is treated as immutable, so a new array object is created (and returned from reducer) any time the array is modified. Check out implementation of the UI reducer.
So, the 'shallow' comparison is what we want here and it should work fine.

Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #23)
> (In reply to Tim Nguyen :ntim from comment #22)
> > Comment on attachment 8931832 [details]
> > Bug 1419369 - NetMonitor: Stop using ImmutableJS in ui reducer;
> > 
> > https://reviewboard.mozilla.org/r/202950/#review210282
> > 
> > ::: devtools/client/netmonitor/src/components/RequestListItem.js:99
> > (Diff revision 6)
> > >    }
> > >  
> > >    shouldComponentUpdate(nextProps) {
> > >      return !propertiesEqual(UPDATED_REQ_ITEM_PROPS, this.props.item, nextProps.item) ||
> > >        !propertiesEqual(UPDATED_REQ_PROPS, this.props, nextProps) ||
> > > -      !I.is(this.props.columns, nextProps.columns);
> > > +      this.props.columns !== nextProps.columns;
> > 
> > Objects are compared by reference, not by value. So this will not work (it
> > will always return true).
> > 
> > You can create a function that checks whether all elements in the object are
> > the same as all the elements in the other object.
> 
> The columns array is treated as immutable, so a new array object is created
> (and returned from reducer) any time the array is modified. Check out
> implementation of the UI reducer.
> So, the 'shallow' comparison is what we want here and it should work fine.
> 
> Honza

Hmm, you're right. Looks like the object ref doesn't change in case there is nothing to change. :)
Attachment #8931832 - Flags: review-
Comment on attachment 8931832 [details]
Bug 1419369 - NetMonitor: Stop using ImmutableJS in ui reducer;

https://reviewboard.mozilla.org/r/202950/#review210556

R+, assuming try is green.

Thanks Russell!
Honza
Attachment #8931832 - Flags: review?(odvarko) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 8b85e5782dc6 -d b1ba2dcb91f1: rebasing 437214:8b85e5782dc6 "Bug 1419369 - NetMonitor: Stop using ImmutableJS in ui reducer; r=Honza" (tip)
merging devtools/client/netmonitor/src/components/RequestListItem.js
warning: conflicts while merging devtools/client/netmonitor/src/components/RequestListItem.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Needs rebase.

Honza
Flags: needinfo?(miller.time.baby)
(In reply to Jan Honza Odvarko [:Honza] from comment #27)
> Needs rebase.
> 
> Honza

Rebased :)
Flags: needinfo?(miller.time.baby)
Attachment #8931832 - Flags: review+ → review?(odvarko)
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ebd3b45ce2f4
NetMonitor: Stop using ImmutableJS in ui reducer; r=Honza
Comment on attachment 8931832 [details]
Bug 1419369 - NetMonitor: Stop using ImmutableJS in ui reducer;

https://reviewboard.mozilla.org/r/202950/#review211076

Thanks!

Honza
Attachment #8931832 - Flags: review?(odvarko) → review+
https://hg.mozilla.org/mozilla-central/rev/ebd3b45ce2f4
Status: ASSIGNED → RESOLVED
Closed: 7 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.

Attachment

General

Creator:
Created:
Updated:
Size: