For setting one of the preset breakdowns.

In progress. Mainly want thoughts on how this should function. When changing breakdowns, right now it blows away stored census data so they're refetched with the new breakdown -- so if you have a few snapshots, switch breakdown, the current selected snapshot takes a new census, but the other ones wait lazily until you select them. Should we do it this way? Or do them all at once and get the costs out of the way? Also, custom breakdowns are in here, haven't tested too much -- but do a JSON.parseable string in devtools.memory.custom-breakdowns -- an object with each key being a unique string name and the property being the breakdown. Like: { "My Custom Breakdown": { "by": "objectClass", "then": { "by": "count", "count": true, "bytes": true } } } What happens if the string isn't JSON parseable? Or worse, what if it is but doesn't have a valid breakdown? Is there any validation logic for breakdown objects? Here's an example of the breakdown change behavior, and custom pref breakdowns in action, and being updated when it changes (or, well when the next state change happens, like selecting a new snapshot, can make this change automatically but im not worried about this case): http://i.imgur.com/kkuLrKW.gif

Lots of changes, but most are for tests and react validations :D https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb69b61b97a7

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #1) > Created attachment 8675212 [details] [diff] [review] > 1215397-change-breakdowns.patch > > In progress. Mainly want thoughts on how this should function. When changing > breakdowns, right now it blows away stored census data so they're refetched > with the new breakdown -- so if you have a few snapshots, switch breakdown, > the current selected snapshot takes a new census, but the other ones wait > lazily until you select them. Should we do it this way? Or do them all at > once and get the costs out of the way? The implemented behavior sounds good to me. We can always revisit it seems to be causing UX issues down the line. > Also, custom breakdowns are in here, haven't tested too much -- but do a > JSON.parseable string in devtools.memory.custom-breakdowns -- an object with > each key being a unique string name and the property being the breakdown. > Like: > > { "My Custom Breakdown": { "by": "objectClass", "then": > { "by": "count", "count": true, "bytes": > true } } } Very awesome. > What happens if the string isn't JSON parseable? We should catch any error for sure, and at minimum DTU.reportException. A warning banner thing would be even better if you want to spend the time to do that. Use reportException for now, and file a follow up for later [polish-backlog] sprints for the banner? > Or worse, what if it is but > doesn't have a valid breakdown? Is there any validation logic for breakdown > objects? We could expose a static method HeapSnapshot.isValidCensusBreakdown(breakdown) down the line, but for now taking a census with an invalid breakdown will throw an exception. Again, we should ensure that this is at least DTU.reportException'd.

Comment on attachment 8675341 [details] [diff] [review] 1215397-change-breakdowns.patch Review of attachment 8675341 [details] [diff] [review]: ----------------------------------------------------------------- r=me with a few nits ::: devtools/client/memory/actions/snapshot.js @@ +16,5 @@ > * @param {MemoryFront} > * @param {HeapAnalysesClient} > * @param {Object} > */ > const takeSnapshotAndCensus = exports.takeSnapshotAndCensus = function takeSnapshotAndCensus (front, heapWorker) { Existing nit: No need to use a named function expression here. @@ +24,5 @@ > yield dispatch(takeCensus(heapWorker, snapshot)); > }; > }; > > +const selectSnapshotAndRefresh = exports.selectSnapshotAndRefresh = function (heapWorker, snapshot) { NIt: doc comments @@ +87,5 @@ > + let census; > + let breakdown = getState().breakdown; > + > + // If breakdown hasn't changed, don't do anything > + if (breakdownEquals(breakdown, snapshot.breakdown)) { What if the snapshot is READ but not SAVED_CENSUS? Shouldn't we still take the census in this case even if the breakdown is the same? ::: devtools/client/memory/test/unit/head.js @@ +71,5 @@ > + return waitUntilState(store, predicate); > +} > + > +function isBreakdownType (census, type) { > + // Little sanity check, all censeses should have atleast a children array "censuses" I always think "censi" in my head, like "octopi", but alas tis not so... ::: devtools/client/memory/utils.js @@ +77,5 @@ > + return typeof name === "object" ? name : > + // If it's in our custom breakdowns, use it > + customBreakdowns[name] ? customBreakdowns[name] : > + // If breakdown name is in our presets, use that > + breakdowns[name] ? breakdowns[name].breakdown : Object.create(null); This is kind of unreadable to me... I think this is big enough that it is just more clear to use if/else @@ +102,2 @@ > case states.SAVING_CENSUS: > return SAVING_CENSUS_TEXT; Is this supposed to switch over every possible census state? It would be nice to add a default that does DTU.reportException so that when we add new states and forget to update this, we learn about our error eventually. @@ +144,5 @@ > + * @param {Any} x > + * @return {String} > + */ > +exports.stringify = function stringify (x) { > + return typeof x === "object" ? JSON.stringify(x) : String(x); Cu.getJSTestingFunctions().uneval(x) @@ +147,5 @@ > +exports.stringify = function stringify (x) { > + return typeof x === "object" ? JSON.stringify(x) : String(x); > +}; > + > +exports.breakdownEquals = function (obj1, obj2) { Nit: need a doc comment. @@ +148,5 @@ > + return typeof x === "object" ? JSON.stringify(x) : String(x); > +}; > + > +exports.breakdownEquals = function (obj1, obj2) { > + let type1 = typeof obj1; Could check right off the bat if obj1 === obj2 and early return true. You could avoid traversing the object if someone opened the dropdown and reselected the same breakdown.