1249779 - Remove "invert tree" checkbox, make inversion a property of the selected "group by" option

Status: RESOLVED FIXED

(DevTools :: Memory, defect, P1)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

We should simplify the breakdowns into:

* Coarse type, always inverted. Renamed to "Group by: 'Type'"
* Allocation stack, with inversion toggle-able. Renamed to "Call Stack".
  * Should this be two options: "Call Stack" and "Inverted Call Stack"? Then
    we could get rid of the invert checkbox permanently.

and maybe

* Internal type, iff "show-gecko-platform-data" pref is set?

We should get rid of "object class" as it is a subset of "coarse type".

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Split all the renaming and option removing into the blocking bugs. Will leave this bug for removing inverting checkbox and making that part of the selected dropdown.

New options will be:

Group by:
  * Type
  * Call Stack
  * Inverted Call Stack

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Remove the "invert tree" checkbox and make inverting a property of the "group by" selection

Effectively, this moves `inverted` from a global and per-census state, to a
per-`breakdownModel` state. Additionally, a `breakdownModel` is not just a
breakdown spec anymore, but also some extra associated metadata.

----------------------------

This ended up being way bigger than I expected. *Sigh*, turns out that (a) it
would be nice to have a compiler point out all the new type errors when moving
properties, and (b) you have to change a lot of code when you change fairly base
relation/ownership assumptions throughout your codebase.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ab552043217

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8723787 [details] [diff] [review]
Remove the "invert tree" checkbox and make inverting a property of the "group by" selection

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

Well this is quite a bit for such a small change, but makes sense -- nice! One thing that seemed weird (and usually a code smell) is the `x.breakdown.breakdown` naming, resulting in not sure if I'm looking at a breakdown model (which has the breakdown spec, inverted status, name, etc.) or a breakdown spec when looking at code. The updated comments helps, but for understanding it faster to fresh eyes, I think calling the breakdown model explicitly `breakdownModel` (or something else) would help (breakdownModel.breakdown) when naming variables.

::: devtools/client/memory/test/browser/browser_memory_allocationStackBreakdown_01.js
@@ +18,5 @@
>    const { getState, dispatch } = panel.panelWin.gStore;
>    const doc = panel.panelWin.document;
>  
> +  dispatch(breakdownActions.setBreakdown(breakdowns.invertedAllocationStack));
> +  is(getState().breakdown.breakdown.by, "allocationStack");

Shouldn't also check `is(getState().breakdown.inverted, true)` here too?

::: devtools/client/memory/test/browser/browser_memory_no_auto_expand.js
@@ +11,1 @@
>  const TEST_URL = "http://example.com/browser/devtools/client/memory/test/browser/doc_steady_allocation.html";

Is this test still useful?

::: devtools/client/memory/test/unit/test_action-toggle-inverted-and-refresh-01.js
@@ +1,1 @@
>  /* Any copyright is dedicated to the Public Domain.

Should update the filename for this test, as well as the *-02 version

::: devtools/client/memory/test/unit/test_action-toggle-inverted.js
@@ +1,1 @@
>  /* Any copyright is dedicated to the Public Domain.

Update file name here too

::: devtools/client/memory/utils.js
@@ +312,5 @@
>   * @param {censusModel} census
>   *
>   * @returns {Boolean}
>   */
> +exports.censusIsUpToDate = function (filter, breakdown, census) {

Shouldn't we be testing the breakdownModel here, rather than the breakdown, so we also check the inverted status?

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I'm thinking what is currently the "breakdown" model should become the "display" model. Then thing.breakdown.breakdown becomes thing.display.breakdown, etc.

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

That sounds better!

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2750be25944c

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Remove the "invert tree" checkbox and make inverting a property of the "group by" selection

s/breakdownModel/displayModel/

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2750be25944c