Comment on attachment 8559457 [details] [diff] [review] 1128859-enable-allocations-pref.patch Review of attachment 8559457 [details] [diff] [review]: ----------------------------------------------------------------- Sweet ::: browser/devtools/performance/modules/recording-model.js @@ +19,5 @@ > const RecordingModel = function (options={}) { > this._front = options.front; > this._performance = options.performance; > this._label = options.label || ""; > + Nit: ws. ::: browser/devtools/performance/performance-controller.js @@ -57,5 @@ > > // Events emitted by various objects in the panel. > const EVENTS = { > // Fired by the OptionsView when a preference changes. > - PREF_CHANGED: "Preformance:PrefChanged", Lolol @@ +234,4 @@ > > this.emit(EVENTS.RECORDING_WILL_STOP, recording); > + > + yield recording.stopRecording({ withAllocations }); Can't `stopRecording` just read from its own model configuration, instead of expecting an argument? ::: browser/devtools/performance/test/browser_perf-details-03.js @@ +9,5 @@ > + */ > +function spawnTest () { > + let { panel } = yield initPerformance(SIMPLE_URL); > + let { EVENTS, DetailsView } = panel.panelWin; > + let { $, WaterfallView, MemoryCallTreeView } = panel.panelWin; Would be better if we made sure everything works when both the MemoryCallTreeView and the MemoryFlameGraphView are toggled, not just the call tree. ::: browser/devtools/performance/test/browser_perf-options-enable-memory-02.js @@ +18,5 @@ > + Services.prefs.setBoolPref(MEMORY_PREF, true); > + yield stopRecording(panel); > + > + is(PerformanceController.getCurrentRecording().getConfiguration().withAllocations, false, > + "The recording finished without tracking memory."); Should check `withMemory` here as well. @@ +25,5 @@ > + yield startRecording(panel); > + Services.prefs.setBoolPref(MEMORY_PREF, false); > + yield stopRecording(panel); > + is(PerformanceController.getCurrentRecording().getConfiguration().withAllocations, true, > + "The recording finished with tracking memory."); Ditto. ::: browser/devtools/performance/test/browser_perf-options-invert-call-tree-02.js @@ +1,5 @@ > /* Any copyright is dedicated to the Public Domain. > http://creativecommons.org/publicdomain/zero/1.0/ */ > > const INVERT_PREF = "devtools.performance.ui.invert-call-tree"; > +const MEMORY_PREF = "devtools.performance.ui.enable-memory"; Maybe move all these consts the head.js? ::: browser/devtools/performance/views/details.js @@ +68,5 @@ > + if (!pref) { > + continue; > + } > + let value = PerformanceController.getPref(pref); > + // Hide the button that selects the view Nit: somewhat redundant comment.

Comment on attachment 8559457 [details] [diff] [review] 1128859-enable-allocations-pref.patch Review of attachment 8559457 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/performance/performance-controller.js @@ +234,4 @@ > > this.emit(EVENTS.RECORDING_WILL_STOP, recording); > + > + yield recording.stopRecording({ withAllocations }); Yeah I like that, if we continue with the assumption that these things can't change ::: browser/devtools/performance/test/browser_perf-details-03.js @@ +9,5 @@ > + */ > +function spawnTest () { > + let { panel } = yield initPerformance(SIMPLE_URL); > + let { EVENTS, DetailsView } = panel.panelWin; > + let { $, WaterfallView, MemoryCallTreeView } = panel.panelWin; +1 ::: browser/devtools/performance/test/browser_perf-options-invert-call-tree-02.js @@ +1,5 @@ > /* Any copyright is dedicated to the Public Domain. > http://creativecommons.org/publicdomain/zero/1.0/ */ > > const INVERT_PREF = "devtools.performance.ui.invert-call-tree"; > +const MEMORY_PREF = "devtools.performance.ui.enable-memory"; +1

rebased

There's a couple tests where stopRecording is used with a param, when that's not necessary anymore. Should clean those up, and there's a new case added in bug 1130200

needinfo? to make sure you see comment 8.

Removed instances where stopRecording takes arguments

Hi, this patch failed to apply cleanly: applying 1128859-enable-allocations-pref.patch patching file browser/devtools/performance/performance-controller.js Hunk #3 FAILED at 291 1 out of 3 hunks FAILED -- saving rejects to file browser/devtools/performance/performance-controller.js.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh 1128859-enable-allocations-pref.patch could you take a look? thanks!

Rebased and landed myself: https://hg.mozilla.org/integration/fx-team/rev/4cff6f1458dd