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

RESOLVED FIXED in Firefox 47

Status

DevTools
Memory
P1
normal
RESOLVED FIXED
2 years ago
10 days ago

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 47
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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
Status: NEW → ASSIGNED
Created attachment 8723787 [details] [diff] [review]
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.
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().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?
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!
Created attachment 8725486 [details] [diff] [review]
Remove the "invert tree" checkbox and make inverting a property of the "group by" selection

s/breakdownModel/displayModel/
Attachment #8725486 - Flags: review+
Attachment #8723787 - Attachment is obsolete: true

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2750be25944c
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47

Updated

10 days ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.