Performance++ should work with Fx2.0/Fx2.1

RESOLVED FIXED in Firefox 38

Status

()

Firefox
Developer Tools: Performance Tools (Profiler/Timeline)
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jsantell, Assigned: jsantell)

Tracking

37 Branch
Firefox 38
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Gecko 32/34 (Fx2.0, Fx2.1):
Mocks: MemoryActor, TimelineActor
Features: should have working JS CallTree view.
(Assignee)

Updated

3 years ago
Blocks: 1077476
(Assignee)

Updated

3 years ago
Depends on: 1130204
(Assignee)

Updated

3 years ago
Summary: Performance++ should work with Fx2.1/Fx2.2 → Performance++ should work with Fx2.0/Fx2.1
Created attachment 8560158 [details] [diff] [review]
1130200-compat-perf.patch

This fixes issues to allow this to work in fx2.0/2.1. Granted, it's only the JS call tree view.
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Attachment #8560158 - Flags: review?(vporof)
Comment on attachment 8560158 [details] [diff] [review]
1130200-compat-perf.patch

Review of attachment 8560158 [details] [diff] [review]:
-----------------------------------------------------------------

Very nice

::: browser/devtools/performance/modules/compatibility.js
@@ +45,5 @@
> +exports.MockTimelineFront = MockTimelineFront;
> +
> +MockMemoryFront.prototype = MockTimelineFront.prototype = MockFront.prototype = {
> +  initialize: function() {},
> +  destroy: function() {}

Could just set the initialize and destroy methods in the MockFront constructor, to avoid having to define a prototype here.

@@ +68,5 @@
> + * Takes a TabTarget, and checks through all methods that are needed
> + * on the server's memory actor to determine if a mock or real MemoryActor
> + * should be used. The last of the methods added to MemoryActor
> + * landed in Gecko 35, so previous versions will fail this. Setting the `target`'s
> + * MOCK_MEMORY_ACTOR property to true w

w?

@@ +74,5 @@
> + * @param {TabTarget} target
> + * @return {Boolean}
> + */
> +function* memoryActorSupported (target) {
> +  // This `target` property is used only in tests.

Could elaborate a bit as to why.

@@ +75,5 @@
> + * @return {Boolean}
> + */
> +function* memoryActorSupported (target) {
> +  // This `target` property is used only in tests.
> +  if (target.MOCK_MEMORY_ACTOR) {

Maybe renaming this to TEST_MOCK_MEMORY_ACTOR would be safer.

@@ +97,5 @@
> + * @return {Boolean}
> + */
> +function* timelineActorSupported(target) {
> +  // This `target` property is used only in tests.
> +  if (target.MOCK_TIMELINE_ACTOR) {

Ditto here on the more elaborated comment and TEST_ prefix.

::: browser/devtools/performance/modules/front.js
@@ +7,5 @@
>  const { Task } = require("resource://gre/modules/Task.jsm");
>  const { extend } = require("sdk/util/object");
> +const {
> +  MockMemoryFront, MockTimelineFront, memoryActorSupported, timelineActorSupported
> +} = require("./compatibility");

Should lazyRequre all of these, since the front is imported in the toolbox, and doesn't need all of these right away.

@@ +387,5 @@
> +
> +  /**
> +   * Returns an object indicating if mock actors are being used or not.
> +   *
> +   * @return object

Nit:

"Returns an object"
"@return object"

A bit redundant.

@@ +389,5 @@
> +   * Returns an object indicating if mock actors are being used or not.
> +   *
> +   * @return object
> +   */
> +  mocksInUse: function () {

Should make this a getter, or rename to getMocksInUse.

::: browser/devtools/performance/test/browser_perf-compatibility-01.js
@@ +54,5 @@
> +    "The timelineEndTime is the same as timelineStartTime.");
> +  is(memoryEndTime, memoryStartTime,
> +    "The memoryEndTime is the same as memoryStartTime.");
> +
> +  yield removeTab(target.tab);

Would be nice to test a bunch more stuff here, like everything in getAllData of a recording model.
Attachment #8560158 - Flags: review?(vporof) → review+
Blocks: 1075567
Blocks: 1123815
Created attachment 8560588 [details] [diff] [review]
1130200-compat-perf.patch

Lazily load modules, rename mock property to TEST_*, add another test that tests the front directly, checking that recording model has empty arrays and duration, etc.
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=1dd182fb4787
Attachment #8560158 - Attachment is obsolete: true
Attachment #8560588 - Flags: review+
(Assignee)

Updated

3 years ago
No longer depends on: 1130204
Created attachment 8561595 [details] [diff] [review]
1130200-compat-perf.patch

rebased
Attachment #8560588 - Attachment is obsolete: true
Attachment #8561595 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/f97102233fba
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/f97102233fba
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox38: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
You need to log in before you can comment on or make changes to this bug.