Closed Bug 1249779 Opened 4 years ago Closed 4 years ago

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


(DevTools :: Memory, defect, P1)



(firefox47 fixed)

Firefox 47
Tracking Status
firefox47 --- fixed


(Reporter: fitzgen, Assigned: fitzgen)


(Blocks 1 open bug)



(1 file, 1 obsolete file)

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".
Has STR: --- → irrelevant
Priority: -- → P1
Depends on: 1249789
Depends on: 1249791
Depends on: 1249792
Depends on: 1249796
Depends on: 1249798
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
Summary: Simplify "group by" choices → Remove "invert tree" checkbox, make inversion a property of the selected "group by" option
Assignee: nobody → nfitzgerald
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.
Attachment #8723787 - Flags: review?(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(), "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 = "";

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?
Attachment #8723787 - Flags: review?(jsantell) → review+
I'm thinking what is currently the "breakdown" model should become the "display" model. Then thing.breakdown.breakdown becomes thing.display.breakdown, etc.
That sounds better!
Blocks: 1248988
Attachment #8723787 - Attachment is obsolete: true
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.