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

Status: RESOLVED FIXED

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

Description

[Jordan Santell [:jsantell] [@jsantell]]

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

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 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.

Comment 2

[Victor Porof [:vporof][:vp]]

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.

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 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

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1130200-compat-perf.patch

rebased

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/f97102233fba

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f97102233fba