Closed Bug 1128859 Opened 7 years ago Closed 7 years ago

Memory allocations views should be dependent on `enable-memory` pref

Categories

(DevTools :: Performance Tools (Profiler/Timeline), defect)

37 Branch
x86
macOS
defect
Not set
normal

Tracking

(firefox38 fixed)

RESOLVED FIXED
Firefox 38
Tracking Status
firefox38 --- fixed

People

(Reporter: jsantell, Assigned: jsantell)

References

Details

Attachments

(1 file, 3 obsolete files)

No description provided.
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Attachment #8559457 - Flags: review?(vporof)
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.
Attachment #8559457 - Flags: review?(vporof) → review+
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
Attachment #8559457 - Attachment is obsolete: true
Attachment #8559482 - Flags: review+
rebased
Attachment #8559482 - Attachment is obsolete: true
Attachment #8559904 - Flags: review+
Depends on: 1129960
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.
Flags: needinfo?(jsantell)
Removed instances where stopRecording takes arguments
Attachment #8559904 - Attachment is obsolete: true
Flags: needinfo?(jsantell)
Attachment #8560278 - Flags: review+
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!
Flags: needinfo?(jsantell)
Keywords: checkin-needed
Rebased and landed myself: https://hg.mozilla.org/integration/fx-team/rev/4cff6f1458dd
Flags: needinfo?(jsantell)
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/4cff6f1458dd
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
No longer depends on: 1129960
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.