Check this out. Still gotta clean up some tests and add more tests, but this is the idea.

Comment on attachment 8547870 [details] [diff] [review] 1111004-multi-recordings.patch Review of attachment 8547870 [details] [diff] [review]: ----------------------------------------------------------------- Swag, r+ with tests and everything. Ask me again for review. ::: browser/devtools/jar.mn @@ +89,5 @@ > #ifdef MOZ_DEVTOOLS_PERFTOOLS > content/browser/devtools/performance.xul (performance/performance.xul) > content/browser/devtools/performance/performance-controller.js (performance/performance-controller.js) > content/browser/devtools/performance/performance-view.js (performance/performance-view.js) > + content/browser/devtools/performance/recording-model.js (performance/recording-model.js) I feel like this should be a module. ::: browser/devtools/performance/performance-controller.js @@ +17,5 @@ > devtools.lazyRequireGetter(this, "DevToolsUtils", > "devtools/toolkit/DevToolsUtils"); > +// Overloads ViewHelper's `Heritage`. > +devtools.lazyRequireGetter(this, "Heritage", > + "sdk/core/heritage"); Why? @@ +50,5 @@ > const EVENTS = { > + // Emitted by the PerformanceController when a recording model > + // is selected. Also emitted by the RecordingsView, which is > + // re-emitted by the PerformanceController. Usually, should only listen > + // to the PerformanceController's event here. This comment makes sounds too canadian. Just write "Emitted by the PerformanceController and PerformanceView when a recording model is selected" @@ +343,2 @@ > */ > + _getLastManualProfile: function () { Nit: _getLatest sounds more accurate. @@ +343,4 @@ > */ > + _getLastManualProfile: function () { > + for (let i = this._recordings.length - 1; i >= 0; i--) { > + if (!this._recordings[i].fromConsole()) { No need for .fromConsole for now. Should be added in a later patch. @@ +349,1 @@ > } This method doesn't always return something. Either throw or return null. ::: browser/devtools/performance/recording-model.js @@ +6,5 @@ > +/** > + * Model for a wholistic profile, containing start/stop times, profiling data, frames data, > + * timeline (marker, tick, memory) data, and methods to start/stop recording. > + */ > +const RecordingModel = Heritage.Class({ Why use Class? ::: browser/devtools/performance/views/recordings.js @@ +97,5 @@ > + > + // If a label is specified (e.g due to a call to `console.profile`), > + // then try reusing a pre-existing recording item, if there is one. > + // This is symmetrical to how `this.handleRecordingEnded` works. > + if (recording.fromConsole()) { It's probably more idiomatic to have a `hasLabel` and use it here, instead of `fromConsole`. We don't want to make the two synonymous (think GCLI, ulterior additions to the console API, GCLI, etc.). @@ +132,5 @@ > + // then try reusing a pre-existing recording item, if there is one. > + // This is symmetrical to how `this.handleRecordingStarted` works. > + if (profileLabel) { > + recordingItem = this.getItemForAttachment(e => > + e.profilerData.profileLabel == profileLabel); Why aren't we using getLabel() here? @@ +210,5 @@ > + return; > + } > + > + // TODO 1120699 > + // show appropriate empty/recording panels I feel like just one TODO comment is enough for this method. @@ +217,5 @@ > + > + // Only clear the checked state if there's nothing recording. > + if (!this.getItemForPredicate(e => e.isRecording)) { > + $("#record-button").removeAttribute("checked"); > + } Is all of this still necessary? Looks kind of ugly to control the recording button from here. @@ +220,5 @@ > + $("#record-button").removeAttribute("checked"); > + } > + > + // But don't leave it locked in any case. > + $("#record-button").removeAttribute("locked"); Ditto.

Comment on attachment 8547870 [details] [diff] [review] 1111004-multi-recordings.patch Review of attachment 8547870 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/performance/performance-controller.js @@ +17,5 @@ > devtools.lazyRequireGetter(this, "DevToolsUtils", > "devtools/toolkit/DevToolsUtils"); > +// Overloads ViewHelper's `Heritage`. > +devtools.lazyRequireGetter(this, "Heritage", > + "sdk/core/heritage"); ViewHelper's Heritage is just a piecemeal copy of the sdk/core/heritage, with comments that should be removed at some point, and this one has more functionality that's used. @@ +50,5 @@ > const EVENTS = { > + // Emitted by the PerformanceController when a recording model > + // is selected. Also emitted by the RecordingsView, which is > + // re-emitted by the PerformanceController. Usually, should only listen > + // to the PerformanceController's event here. Understood. ::: browser/devtools/performance/recording-model.js @@ +6,5 @@ > +/** > + * Model for a wholistic profile, containing start/stop times, profiling data, frames data, > + * timeline (marker, tick, memory) data, and methods to start/stop recording. > + */ > +const RecordingModel = Heritage.Class({ Originally used some properties of `Class`. No longer the case, so this can be just a basic Function. ::: browser/devtools/performance/views/recordings.js @@ +97,5 @@ > + > + // If a label is specified (e.g due to a call to `console.profile`), > + // then try reusing a pre-existing recording item, if there is one. > + // This is symmetrical to how `this.handleRecordingEnded` works. > + if (recording.fromConsole()) { Agreed @@ +132,5 @@ > + // then try reusing a pre-existing recording item, if there is one. > + // This is symmetrical to how `this.handleRecordingStarted` works. > + if (profileLabel) { > + recordingItem = this.getItemForAttachment(e => > + e.profilerData.profileLabel == profileLabel); We can -- just need to return recording model instead of recordingData everywhere, so small refactor, but should be fine (because you can easily get the recording data from the model) @@ +217,5 @@ > + > + // Only clear the checked state if there's nothing recording. > + if (!this.getItemForPredicate(e => e.isRecording)) { > + $("#record-button").removeAttribute("checked"); > + } Yeah I'm not 100% sure what this is, was legacy from the `ui-recordings.js`

Added tests -- pushed to try https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=063620213af7

Comment on attachment 8547949 [details] [diff] [review] 1111004-multi-recordings.patch Review of attachment 8547949 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/performance/modules/recording-model.js @@ +225,5 @@ > + let [markers] = data; > + Array.prototype.push.apply(this._markers, markers); > + } > + // Accumulate stack frames into an array. > + else if (eventName == "frames") { Why not a switch statement ?

Comment on attachment 8547949 [details] [diff] [review] 1111004-multi-recordings.patch Review of attachment 8547949 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/performance/modules/recording-model.js @@ +225,5 @@ > + let [markers] = data; > + Array.prototype.push.apply(this._markers, markers); > + } > + // Accumulate stack frames into an array. > + else if (eventName == "frames") { This just was here before in the main controller, no preference though, uploading new patch with a switch in it

Added switch statement, try looks good