Closed Bug 1408124 Opened 7 years ago Closed 7 years ago

Create a new recording panel actor

Categories

(DevTools :: Performance Tools (Profiler/Timeline), enhancement, P2)

Product:
DevTools
Component:
Performance Tools (Profiler/Timeline)
Type:
enhancement

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: gregtatum, Assigned: gregtatum)

References

Details

Attachments

(1 file, 5 obsolete files)

Bug 1408124 - Create a new perf actor and recording panel;
59 bytes, text/x-review-board-request
julienw
: review+
Details 
In order to support a new recording panel we need a new performance actor. Rather than use the existing actors, we should research what we want to migrate over and start with a blank slate. The goal of the actor is to start the Gecko Profiler on a page, and associate that page's information withe profile captured. After the capture, the profile should be dumped into the perf-html.io, or a configurable url (e.g. localhost:4242). The actor might also be the correct place to perform the source mapping.
Attachment #8922809 - Attachment is obsolete: true
Attachment #8924278 - Attachment is obsolete: true
Attachment #8925705 - Attachment is obsolete: true
Attachment #8926105 - Attachment is obsolete: true
Attachment #8926499 - Attachment is obsolete: true
Assignee: nobody → gtatum
Comment on attachment 8917961 [details]
Bug 1408124 - Create a new perf actor and recording panel;

https://reviewboard.mozilla.org/r/188854/#review204944

Thanks for this work, this is very complete! Note you need to rebase as there are conflicts :)

My main comments are:
* I think we should put the files elsewhere than in a `new` subdirectory. I know the debugger folks do that but I think it's bad.
* In the same vein, because this is a very different tab than the previous one, we should handle it like a complete new tab, and not just one new version of the existing tab.
* Let's not use already obsolete React API when the new ones are available.
* I gave suggestions to rewrite very small part of code into more readable versions.

This is a good start but we have more things to do in subsequent bugs. From the top of my head:
* put the tab in green when recording
* identify which tab(s) is (are) important so that we don't display useless things at first
* of course, switch to JS-only view
* support pseudo-frames in JS-only view

We need bugs for these tasks (and maybe others ?)

::: commit-message-19b32:1
(Diff revision 5)
> +Bug 1408124 - Create a new perf actor and recording panel; r?julienw

Please add the new pref in the commit message.

::: devtools/client/definitions.js:22
(Diff revision 5)
>  loader.lazyGetter(this, "ShaderEditorPanel", () => require("devtools/client/shadereditor/panel").ShaderEditorPanel);
>  loader.lazyGetter(this, "CanvasDebuggerPanel", () => require("devtools/client/canvasdebugger/panel").CanvasDebuggerPanel);
>  loader.lazyGetter(this, "WebAudioEditorPanel", () => require("devtools/client/webaudioeditor/panel").WebAudioEditorPanel);
>  loader.lazyGetter(this, "MemoryPanel", () => require("devtools/client/memory/panel").MemoryPanel);
>  loader.lazyGetter(this, "PerformancePanel", () => require("devtools/client/performance/panel").PerformancePanel);
> +loader.lazyGetter(this, "PerfPanel", () => require("devtools/client/performance/new/panel").PerfPanel);

Can we name it something better than PerfPanel ? Maybe NewPerformancePanel or PerformancePanel2 ?

::: devtools/client/definitions.js:273
(Diff revision 5)
>    },
>    accesskey: l10n("performance.accesskey"),
>    inMenu: true,
> -
> -  isTargetSupported: function (target) {
> -    return target.hasActor("performance");
> +};
> +function switchPerformancePanel() {
> +  if (Services.prefs.getBoolPref("devtools.performance.new-panel-enabled")) {

devtools.performance.new-panel.enabled ?

::: devtools/client/definitions.js:299
(Diff revision 5)
> +switchPerformancePanel();
> +
> +Services.prefs.addObserver(
> +  "devtools.performance.new-panel-enabled",
> +  { observe: switchPerformancePanel }
> +);

Maybe it's simpler to have 2 completely distinct Tools properties (which they actually are), both controlled by 2 distinct prefs ? I believe the property "visibilitySwitch" would control this neatly.

It would also make it easier to remove the old one once we want to do it.

::: devtools/client/framework/toolbox-options.js:335
(Diff revision 5)
>        parentId: "debugger-options"
> +    }, {
> +      pref: "devtools.performance.new-panel-enabled",
> +      label: "Enable new performance recorder",
> +      id: "devtools-new-performance",
> +      parentId: "context-options"

See, it wouldn't be very clear that we need 2 prefs for this :)

::: devtools/client/framework/toolbox-options.js:335
(Diff revision 5)
>        parentId: "debugger-options"
> +    }, {
> +      pref: "devtools.performance.new-panel-enabled",
> +      label: "Enable new performance recorder",
> +      id: "devtools-new-performance",
> +      parentId: "context-options"

See, it wouldn't be very clear that we need 2 prefs for this :)

::: devtools/client/performance/moz.build:9
(Diff revision 5)
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  DIRS += [
>      'components',
>      'modules',
> +    'new',

I'm not a big fan of having a subdirectory "new". What happens when we remove the old one ? What happens when, in the future, we'll have a "newer" ?

IMO we should keep things simple and just have a "devtools/client/performance2" directory.

::: devtools/client/performance/new/components/Perf.js:6
(Diff revision 5)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +"use strict";
> +
> +const { PropTypes, Component, DOM } = require("devtools/client/shared/vendor/react");

I see we have `react-prop-types.js` in `shared/vendor`, I believe we should use it instead of relying on an already deprecated API.

::: devtools/client/performance/new/components/Perf.js:7
(Diff revision 5)
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +"use strict";
> +
> +const { PropTypes, Component, DOM } = require("devtools/client/shared/vendor/react");
> +const { div, button } = DOM;

From the React source code:
```
// React.DOM factories are deprecated. Wrap these methods so that              
// invocations of the React.DOM namespace and alert users to switch            
// to the `react-dom-factories` package. 
```

Turns out the react-dom-factories package is also in `shared/vendor` :)

::: devtools/client/performance/new/components/Perf.js:29
(Diff revision 5)
> +// The profiler notified us that our request to start it actually started it.
> +const RECORDING = "recording";
> +// Some other code with access to the profiler started it.
> +const OTHER_IS_RECORDING = "other-is-recording";
> +// Profiling is not available when in private browsing mode.
> +const LOCKED_FOR_PRIVATE_BROWSING = "locked-for-private-browsing";

Nit, I know, but shouldn't this be "locked by private browsing"? Not that I care enough to force you to change it, moreover I see the event from the profiler is named like this already.

::: devtools/client/performance/new/components/Perf.js:107
(Diff revision 5)
> +        // We couldn't have started it yet, so it must have been someone
> +        // else. (fallthrough)
> +      case AVAILABLE_TO_RECORD:
> +        // We aren't recording, someone else started it up. (fallthrough)
> +      case REQUEST_TO_STOP_PROFILER:
> +        // We aren't recording, someone else started it up. (fallthrough)

is it the right comment ?

::: devtools/client/performance/new/components/Perf.js:220
(Diff revision 5)
> +    this.setState({
> +      recordingState: REQUEST_TO_START_RECORDING,
> +      // Reset this error state since it's no longer valid.
> +      recordingUnexpectedlyStopped: false,
> +    });
> +    this.props.perfFront.startProfiler();



::: devtools/client/performance/new/components/Perf.js:231
(Diff revision 5)
> +    this.setState({ recordingState: AVAILABLE_TO_RECORD });
> +    console.log("getProfileAndStopProfiler");
> +    this.props.receiveProfile(profile);
> +  }
> +
> +  async stopProfilerAndDiscardProfile() {

async, but no await ?

::: devtools/client/performance/new/components/Perf.js:243
(Diff revision 5)
> +
> +    // Handle the cases of platform support.
> +    switch (isSupportedPlatform) {
> +      case null:
> +        // We don't know yet if this is a supported platform, wait for a response.
> +        return null;

Maybe not for this patch, but do you think we should display something here ? A default layout or something, for perceived performance ?

::: devtools/client/performance/new/components/Perf.js:256
(Diff revision 5)
> +      case true:
> +        // Continue on and render the panel.
> +        break;
> +    }
> +
> +    // TODO - L10N all of the messages.

Please file a separate bug and add it to this TODO line.

::: devtools/client/performance/new/components/Perf.js:266
(Diff revision 5)
> +      case AVAILABLE_TO_RECORD:
> +        return renderButton({
> +          onClick: this.startRecording,
> +          label: "Start recording",
> +          additionalMessage: this.state.recordingUnexpectedlyStopped
> +            ? div({}, "The recording was stopped by another tool.")

Do you know if we can pass `null` or `undefined` instead of an empty object? I don't know myself.

::: devtools/client/performance/new/components/Perf.js:294
(Diff revision 5)
> +
> +      case OTHER_IS_RECORDING:
> +        return renderButton({
> +          label: "Stop and discard the other recording",
> +          onClick: this.stopProfilerAndDiscardProfile,
> +          disabled: this.state.recordingState === REQUEST_TO_START_RECORDING,

if I'm not wrong, `disabled` will always be false here ?

::: devtools/client/performance/new/components/Perf.js:304
(Diff revision 5)
> +        return renderButton({
> +          label: "Start recording",
> +          disabled: true,
> +          additionalMessage: "The profiler is disabled when Private Browsing is " +
> +                             "enabled. Close all Private Windows to re-enable the " +
> +                             "profiler."

Can't we use template strings instead of concatenating stuff? Or is the issue that we don't have the handy `common-tags` npm package to remove indentation?

::: devtools/client/performance/new/frame-script.js:19
(Diff revision 5)
> +let gProfile = null;
> +
> +addMessageListener(TRANSFER_EVENT, e => {
> +  gProfile = e.data;
> +  connectToPage();
> +  addEventListener("DOMContentLoaded", connectToPage);

nit: please add a comment explaining why we call `connectToPage` twice.

Now that I look at it closer, I feel like this works by chance: we're not sure the page's scripts actually created `connectToGeckoProfiler` at `DOMContentLoaded`... Not the scope of this patch though, but we'll need to look at it. Instead or the addon/the devtools panel calling a function from perf.html, I think the addon/the devtools panel should inject an object to the page (it can do it very early) that perf.html could call.

::: devtools/client/performance/new/frame-script.js:38
(Diff revision 5)
> + * For now, do not try to symbolicate. Reject any attempt.
> + */
> +function getSymbolTable(debugName, breakpadId) {
> +  return new Promise((resolve, reject) => {
> +    reject();
> +  });

`return Promise.reject()` ?
Also please pass an error with the rejection.

::: devtools/client/performance/new/frame-script.js:52
(Diff revision 5)
> +  function funThatClonesObjects(resolve, reject) {
> +    return fun(result => resolve(Components.utils.cloneInto(result, contentGlobal)),
> +               error => reject(Components.utils.cloneInto(error, contentGlobal)));
> +  }
> +  return new contentGlobal.Promise(Components.utils.exportFunction(funThatClonesObjects,
> +                                                                   contentGlobal));

Not sure that "funThatClonesObjects" needs to be exported. Isn't it executed in this context?

::: devtools/client/performance/new/frame-script.js:53
(Diff revision 5)
> +    return fun(result => resolve(Components.utils.cloneInto(result, contentGlobal)),
> +               error => reject(Components.utils.cloneInto(error, contentGlobal)));
> +  }
> +  return new contentGlobal.Promise(Components.utils.exportFunction(funThatClonesObjects,
> +                                                                   contentGlobal));
> +}

I think the function could be simpler by taking a promise as parameter:

```
function createPromiseInPage(promise, contentGlobal) {
  return new contentGlobal.Promise((resolve, reject) => {
    promise.then(
      result => resolve(cloneInto(result, contentGlobal)),
      error => reject(cloneInto(error, contentGlobal))
    );
  });
};
```
(`cloneInto` should be imported in the current scope for a greater readability.)

Then you can use it below like this; `return createPromiseInPage(result, contentGlobal)`

::: devtools/client/performance/new/frame-script.js:79
(Diff revision 5)
> +/**
> + * Pass a simple object containing values that are objects or functions.
> + * The objects or functions are wrapped in such a way that they can be
> + * consumed by the page.
> + */
> +function makeAccessibleToPage(obj, contentGlobal) {

I believe you could just use `cloneInto` with the option `cloneFunctions` ?

See https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Language_Bindings/Components.utils.cloneInto#Cloning_objects_that_have_functions

`cloneInto` already does a structured clone so you don't need to worry about cloning object properties, and usinf this `cloneFunctions` you don't need to worry about it either.

Now I think we still need wrapFunction for the promise, right ? (You should file a bug for this, this is painful). Then you can use wrapFunction directly line 26 and 27.

Actually, I believe you can just get rid of this complex `makeAccessibleToPage` function by using directly `cloneInto` and `wrapFunction` line 25. You don't need a complex function to expose a 2-property object :)

::: devtools/client/performance/new/frame-script.js:86
(Diff revision 5)
> +  for (let field in obj) {
> +    switch (typeof obj[field]) {
> +      case "function":
> +        Components.utils.exportFunction(
> +          wrapFunction(obj[field], contentGlobal), result, { defineAs: field });
> +        break;

I believe you can avoid that by calling cl

::: devtools/client/performance/new/initializer.js:10
(Diff revision 5)
> +
> +/* exported gInit */
> +
> +const { utils: Cu } = Components;
> +const BrowserLoaderModule = {};
> +Cu.import("resource://devtools/client/shared/browser-loader.js", BrowserLoaderModule);

nit: just use `Component.utils.import` and remove line 8

::: devtools/client/performance/new/initializer.js:34
(Diff revision 5)
> +      const browser = top.gBrowser;
> +      const tab = browser.addTab("https://perf-html.io/from-addon");
> +      browser.selectedTab = tab;
> +      const mm = tab.linkedBrowser.messageManager;
> +      mm.loadFrameScript("chrome://devtools/content/performance/new/frame-script.js",
> +                         false);

Is this the indentation we should use in Gecko ?

Unless it's mandatory, please do something like
```
mm.loadFrameScript(
  "chrome://...",
  false
);
```

::: devtools/client/performance/new/panel.js:9
(Diff revision 5)
> +"use strict";
> +
> +const { PerfFront } = require("devtools/shared/fronts/perf");
> +
> +loader.lazyRequireGetter(this, "EventEmitter",
> +  "devtools/shared/old-event-emitter");

Is there a new one we could use? Or just the devtools code use this one and we should use it?

::: devtools/client/performance/new/panel.js:37
(Diff revision 5)
> +
> +      this.isReady = true;
> +      this.emit("ready");
> +      this.panelWin.gInit(perfFront);
> +      resolve(this);
> +    });

I think this would work the same and be easier to read:
```
this.panelWin.gToolbox = this.toolbox;
this.panelWin.gTarget = this.target;

this._opening = (async () => {
  const rootForm = await this.target.root;
  const perfFront = new PerfFront(this.target.client, rootForm);

  this.isReady = true;
  this.emit("ready");
  this.panelWin.gInit(perfFront);
  return this;
})();
```

Basically, the logic is I don't see the need to use `new Promise` when the async function already returns a promise.

It may be even more readable by move=ing it to a `async _doOpen()` method.

::: devtools/client/performance/new/panel.js:47
(Diff revision 5)
> +
> +  get target() {
> +    return this.toolbox.target;
> +  }
> +
> +  async destroy() {

nit: if nothing is asynchronous in this method, just `return Promise.resolve()`. Although I don't mind if you prefer this way.

::: devtools/client/performance/new/test/chrome/head.js:93
(Diff revision 5)
> +  flushAsyncQueue() {
> +    const pending = this._asyncQueue;
> +    this._asyncQueue = [];
> +    pending.forEach(fn => fn());
> +    // Wait for the next frame before continuing
> +    return new Promise(requestAnimationFrame);

maybe `setTimeout` would be better here ? If I understand correctly, what you really want is waiting that all promises are run, and `setTimeout` should do it as well but would still run the callback faster than `requestAnimationFrame`.

Although that IMO none of these techniques is really foolproof: you don't know what the functions' callers will do.

I guess this works here because our promise chains aren't really asynchronous.

::: devtools/client/performance/new/test/chrome/head.js:98
(Diff revision 5)
> +    return new Promise(requestAnimationFrame);
> +  }
> +
> +  startProfiler() {
> +    this._isActive = true;
> +    // Defer this so it doesn't happen immediately.

I don't understand these comments about "defer" in these methods. Where do we defer anything in this code?

::: devtools/server/main.js:461
(Diff revision 5)
>        constructor: "HeapSnapshotFileActor",
>        type: { global: true }
>      });
> +    // Always register this as a global module, even while there is a pref turning
> +    // on and off the other performance actor. This actor shouldn't conflict with
> +    // the other one.

why don't we trigger it with a pref too ? And why don't we look for `nsIProfiler` too ?
> * I think we should put the files elsewhere than in a `new` subdirectory. I know the debugger folks do that but I think it's bad.

My preference would be to follow what the debugger and webconsole are already doing (although the webconsole has new-console-output) as their directory. My thought was once we're ready to swap them out, we rewrite the directories and swap in the new one with a bunch of file moves. I'm not seeing a disadvantage here, as once we've completed our work there will be a clean swap.

I'd also like to bring up a point on the "PerfPanel" comment. The actors are namespace via a string prefix that the messaging keys off of. The prefix "performance" was already taken. I decided to be consistent on all new work with the word "perf". To me this has a nice symmetry to "perf.html", as well as the added benefit of being able to be distinct from the existing performance DevTools work, as they did not use this abbreviation. I found it useful when searching through the codebase.

> * In the same vein, because this is a very different tab than the previous one, we should handle it like a complete new tab, and not just one new version of the existing tab.

I disagree on this as well. The tab is a replacement, as once we are done with the work, we will remove the old one. The two tools are mutually exclusive, as they both need to work with the Gecko Profiler, so it's not possible to have them both on at the same time. The current performance devtool ends up fighting for control of the Gecko Profiler Add-on as it is.

I believe I agree with most everything else on the review and I'll start addressing the changes. Thanks for taking the time to look through this huge patch, and it might be helpful to chat tomorrow on Slack about it a bit to reach consensus.
> My preference would be to follow what the debugger and webconsole are already doing (although the webconsole has new-console-output) as their directory. My thought was once we're ready to swap them out, we rewrite the directories and swap in the new one with a bunch of file moves. I'm not seeing a disadvantage here, as once we've completed our work there will be a clean swap.

But it will be even easier with a different top-level directory. I don't see the disavantage of having a different top-level directory, but I clearly see it having a messy "new" subdirectory.


> The tab is a replacement, as once we are done with the work, we will remove the old one. The two tools are mutually exclusive, as they both need to work with the Gecko Profiler, so it's not possible to have them both on at the same time.

I agree with the "mutually exclusive" argument. But I think this could be handled through a clever use of how we set prefs. But codewise, we can still handle them as separate tabs instead of having a sort of a factory that makes things more complex and makes the old tab more difficult to remove once we want to do it.
Comment on attachment 8917961 [details]
Bug 1408124 - Create a new perf actor and recording panel;

https://reviewboard.mozilla.org/r/188854/#review204944

> Can we name it something better than PerfPanel ? Maybe NewPerformancePanel or PerformancePanel2 ?

Ok, I've come around. NewPerformancePanel it is.

> devtools.performance.new-panel.enabled ?

I was following the other panels here:
"devtools.webconsole.new-frontend-enabled"
"devtools.debugger.new-debugger-frontend"

Although I named this one "new-panel-enabled", since it's not just the frontend. I'm not planning on prefixing prefs under "new-panel". I'm dropping this issue, but if you want it I can re-add it again.

> Maybe it's simpler to have 2 completely distinct Tools properties (which they actually are), both controlled by 2 distinct prefs ? I believe the property "visibilitySwitch" would control this neatly.
> 
> It would also make it easier to remove the old one once we want to do it.

I'm putting them both on Tools.performance and doing a single `if` check at startup. It is much simpler this way.

> I'm not a big fan of having a subdirectory "new". What happens when we remove the old one ? What happens when, in the future, we'll have a "newer" ?
> 
> IMO we should keep things simple and just have a "devtools/client/performance2" directory.

I opted for `performance-new` so it's a bit more explicit.

> I see we have `react-prop-types.js` in `shared/vendor`, I believe we should use it instead of relying on an already deprecated API.

Nice catch, thanks! I need to rebase again to get those.

> From the React source code:
> ```
> // React.DOM factories are deprecated. Wrap these methods so that              
> // invocations of the React.DOM namespace and alert users to switch            
> // to the `react-dom-factories` package. 
> ```
> 
> Turns out the react-dom-factories package is also in `shared/vendor` :)

Will pull in with a rebase.

> Nit, I know, but shouldn't this be "locked by private browsing"? Not that I care enough to force you to change it, moreover I see the event from the profiler is named like this already.

Seems reasonable. I created all the events so I can rename them all.

> is it the right comment ?

Nope

> async, but no await ?

It was probably left over from some intermediate state. Cleaning it up.

> Maybe not for this patch, but do you think we should display something here ? A default layout or something, for perceived performance ?

Let's follow up on this, but I didn't perceive this waiting when I was workin on it.
Blocks: devtools-perfhtml
Comment on attachment 8917961 [details]
Bug 1408124 - Create a new perf actor and recording panel;

https://reviewboard.mozilla.org/r/188854/#review204944

> Do you know if we can pass `null` or `undefined` instead of an empty object? I don't know myself.

Looks like yes!

> Can't we use template strings instead of concatenating stuff? Or is the issue that we don't have the handy `common-tags` npm package to remove indentation?

I don't think we have common tags, although this is HTML so whitespace shouldn't matter if I switch to template strings.

> nit: please add a comment explaining why we call `connectToPage` twice.
> 
> Now that I look at it closer, I feel like this works by chance: we're not sure the page's scripts actually created `connectToGeckoProfiler` at `DOMContentLoaded`... Not the scope of this patch though, but we'll need to look at it. Instead or the addon/the devtools panel calling a function from perf.html, I think the addon/the devtools panel should inject an object to the page (it can do it very early) that perf.html could call.

Yeah, that's a good question. This is all taken from the addon.

> `return Promise.reject()` ?
> Also please pass an error with the rejection.

Good call!

> Not sure that "funThatClonesObjects" needs to be exported. Isn't it executed in this context?

Everything following `// The following functions handle the security of cloning the object into the page.` was taken directly from the Gecko Profile Add-on using the old add-on SDK.

I'd really prefer not to touch the cloneInto code as I'm afraid of getting it wrong, and my attempt to rewrite it initially failed. I put in a more detailed explanation of the origin of the code as a comment. I'd prefer not to address these points, but I'm going to leave the issues open until I get your feedback.

```
// The following functions handle the security of cloning the object into the page.
// The code was taken from the original Gecko Profiler Add-on to maintain
// compatibility with the existing profile importing mechanism:
// See: https://github.com/devtools-html/Gecko-Profiler-Addon/blob/78138190b42565f54ce4022a5b28583406489ed2/data/tab-framescript.js
```

> I believe you can avoid that by calling cl

This comment is cut off

> nit: just use `Component.utils.import` and remove line 8

Copy pasta programming. I agree!

> Is this the indentation we should use in Gecko ?
> 
> Unless it's mandatory, please do something like
> ```
> mm.loadFrameScript(
>   "chrome://...",
>   false
> );
> ```

Most of Gecko does that awkward style, although I'd be happy to not follow it :)

> Is there a new one we could use? Or just the devtools code use this one and we should use it?

I think this is part of the API of the actors, as they are consumed by other devtools code, so we need to use the old one.

See: https://searchfox.org/mozilla-central/search?q=emitter&case=false&regexp=false&path=panel.js

> I think this would work the same and be easier to read:
> ```
> this.panelWin.gToolbox = this.toolbox;
> this.panelWin.gTarget = this.target;
> 
> this._opening = (async () => {
>   const rootForm = await this.target.root;
>   const perfFront = new PerfFront(this.target.client, rootForm);
> 
>   this.isReady = true;
>   this.emit("ready");
>   this.panelWin.gInit(perfFront);
>   return this;
> })();
> ```
> 
> Basically, the logic is I don't see the need to use `new Promise` when the async function already returns a promise.
> 
> It may be even more readable by move=ing it to a `async _doOpen()` method.

Went to a method, and simplified it a bit more.

> nit: if nothing is asynchronous in this method, just `return Promise.resolve()`. Although I don't mind if you prefer this way.

I think it doesn't read as clear in this case, but normally I would agree.

> I don't understand these comments about "defer" in these methods. Where do we defer anything in this code?

Ah, these are wrong now. Earlier they all had manual control of the async behavior. Now they are all wrapped in the flushable async queue.

> why don't we trigger it with a pref too ? And why don't we look for `nsIProfiler` too ?

I think it lowers the complexity of getting this landed to always have it on, and not have to fiddle preferences for it with the tests. Global actors are lazily initialized, so I don't believe there is a perf impact to have it always registered. It also will not conflict with the existing performance panel.

For the nsIProfiler check, I wrapped up the supported platform checks into the actor abstraction itself. Global actors are lazily initialized, so I can't ask if the server has the actor without initializing all of the actors. I moved the question into the actor, so I can explicitly initialize it, then ask.

```
// devtools/server/actors/perf.js

  isSupportedPlatform() {
    return IS_SUPPORTED_PLATFORM;
  },
```
Comment on attachment 8917961 [details]
Bug 1408124 - Create a new perf actor and recording panel;

https://reviewboard.mozilla.org/r/188854/#review204944

> maybe `setTimeout` would be better here ? If I understand correctly, what you really want is waiting that all promises are run, and `setTimeout` should do it as well but would still run the callback faster than `requestAnimationFrame`.
> 
> Although that IMO none of these techniques is really foolproof: you don't know what the functions' callers will do.
> 
> I guess this works here because our promise chains aren't really asynchronous.

Yeah, I was mainly wanting all Promises to run out, and ensure a bit of asynchronous behavior in the test so that the follow-up code would be in the next event loop. I changed it to a setTimeout of 0 seconds.
Ok julien, I've done the follow-ups here, all except the frame script ones (see my comments). I did another try run, but haven't seen the results yet.
> all except the frame script ones (see my comments).

It's fine, if this works we can look at it later.
Comment on attachment 8917961 [details]
Bug 1408124 - Create a new perf actor and recording panel;

https://reviewboard.mozilla.org/r/188854/#review207438

This looks good, but please fix the 2 remaining issues and make sure the tests are green :)

::: devtools/client/definitions.js:258
(Diff revisions 6 - 7)
>    }
>  };
>  
> +if (Services.prefs.getBoolPref("devtools.performance.new-panel-enabled")) {
> -Tools.performance = {
> +  Tools.performance = {
> -  id: "performance",
> +    id: "performance",

nit: the old one gets the id "perf" while the new one has the id "performance". Maybe just use the same one, or use "perf" for the new one and "performance" for the old one.

I believe this is what makes the tests fail: we request to open the tab "performance" which links to the new one, which doesn't exist without the new pref.

::: devtools/client/performance-new/frame-script.js:44
(Diff revision 7)
> +  // Errors will not properly clone into the content page as they bring privileged
> +  // stacks information into the page. In this case provide a mock object to maintain
> +  // the Error type object shape.
> +  const error = {
> +    message: `The DevTools' "perf" actor does not support symbolication.`
> +  };

Why not:

new Error(`The DevTools' "perf" actor does not support symbolication.`)

?

::: devtools/client/performance-new/test/chrome/test_perf-state-01.html:1
(Diff revision 7)
> +<!DOCTYPE HTML>

I miss a test that would check that the tab is properly added with the right preference.

OK to do it in a separate bug though.
Attachment #8917961 - Flags: review?(felash) → review+
BTW Alexandre Poirot told me we should be using RequestBulk and streaming the result, not to be able of displaying continuously, but to properly support the mobile use case. Food for thought.
Depends on: 1420952
No longer depends on: 1420952
Pushed by gtatum@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/36b3f7fb7d31
Create a new perf actor and recording panel; r=julienw
Backout by toros@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f7ee3e63c528
Backed out changeset 36b3f7fb7d31 for chrome failures in devtools test devtools/shared/security/tests/chrome/test_websocket-transport.html r=backout on a CLOSED TREE
Backed out for for chrome failures in devtools test devtools/shared/security/tests/chrome/test_websocket-transport.html
Push with failures - https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=36b3f7fb7d313122a1294fd37c98c86334815798

Failure: https://treeherder.mozilla.org/logviewer.html#?job_id=148238695&repo=autoland&lineNumber=2511

[task 2017-11-28T19:02:58.200Z] 19:02:58     INFO -  1 INFO TEST-START | devtools/shared/security/tests/chrome/test_websocket-transport.html
2508
[task 2017-11-28T19:02:58.201Z] 19:02:58     INFO -  Buffered messages logged at 19:02:53
2509
[task 2017-11-28T19:02:58.201Z] 19:02:58     INFO -  2 INFO SpawnTask.js | Entering test
2510
[task 2017-11-28T19:02:58.201Z] 19:02:58     INFO -  Buffered messages finished
2511
[task 2017-11-28T19:02:58.202Z] 19:02:58     INFO -  3 INFO TEST-UNEXPECTED-FAIL | devtools/shared/security/tests/chrome/test_websocket-transport.html | [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js :: _addTabActors :: line 546"  data: no] - Should not throw any errors
2512
[task 2017-11-28T19:02:58.202Z] 19:02:58     INFO -  _addTabActors@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:546:10
2513
[task 2017-11-28T19:02:58.203Z] 19:02:58     INFO -  registerActors@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:275:7
2514
[task 2017-11-28T19:02:58.203Z] 19:02:58     INFO -  registerAllActors@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:283:5
2515
[task 2017-11-28T19:02:58.203Z] 19:02:58     INFO -  window.onload/<@chrome://mochitests/content/chrome/devtools/shared/security/tests/chrome/test_websocket-transport.html:30:5
2516
[task 2017-11-28T19:02:58.203Z] 19:02:58     INFO -  onFulfilled@chrome://mochikit/content/tests/SimpleTest/SpawnTask.js:69:15
2517
[task 2017-11-28T19:02:58.203Z] 19:02:58     INFO -  co/<@chrome://mochikit/content/tests/SimpleTest/SpawnTask.js:58:5
2518
[task 2017-11-28T19:02:58.203Z] 19:02:58     INFO -  co@chrome://mochikit/content/tests/SimpleTest/SpawnTask.js:54:10
2519
[task 2017-11-28T19:02:58.203Z] 19:02:58     INFO -  toPromise@chrome://mochikit/content/tests/SimpleTest/SpawnTask.js:122:60
2520
[task 2017-11-28T19:02:58.203Z] 19:02:58     INFO -  next@chrome://mochikit/content/tests/SimpleTest/SpawnTask.js:103:19
2521
[task 2017-11-28T19:02:58.203Z] 19:02:58     INFO -  onFulfilled@chrome://mochikit/content/tests/SimpleTest/SpawnTask.js:73:7
2522
[task 2017-11-28T19:02:58.204Z] 19:02:58     INFO -  co/<@chrome://mochikit/content/tests/SimpleTest/SpawnTask.js:58:5
2523
[task 2017-11-28T19:02:58.204Z] 19:02:58     INFO -  co@chrome://mochikit/content/tests/SimpleTest/SpawnTask.js:54:10
2524
[task 2017-11-28T19:02:58.204Z] 19:02:58     INFO -  add_task/<@chrome://mochikit/content/tests/SimpleTest/SpawnTask.js:272:9
2525
[task 2017-11-28T19:02:58.204Z] 19:02:58     INFO -  4 INFO TEST-OK | devtools/shared/security/tests/chrome/test_websocket-transport.html | took 4290ms

Failure: https://treeherder.mozilla.org/logviewer.html#?job_id=148237495&repo=autoland&lineNumber=2174

[task 2017-11-28T18:51:45.049Z] 18:51:45     INFO -  6 INFO None7 INFO TEST-START | mobile/android/tests/browser/chrome/test_debugger_server.html
2171
[task 2017-11-28T18:51:55.267Z] 18:51:55     INFO -  Buffered messages logged at 18:51:44
2172
[task 2017-11-28T18:51:55.267Z] 18:51:55     INFO -  8 INFO TEST-PASS | mobile/android/tests/browser/chrome/test_debugger_server.html | initialized
2173
[task 2017-11-28T18:51:55.267Z] 18:51:55     INFO -  Buffered messages finished
2174
[task 2017-11-28T18:51:55.267Z] 18:51:55     INFO -  9 INFO TEST-UNEXPECTED-FAIL | mobile/android/tests/browser/chrome/test_debugger_server.html | 1 listening socket - got +0, expected 1
2175
[task 2017-11-28T18:51:55.267Z] 18:51:55     INFO -  SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:312:5
2176
[task 2017-11-28T18:51:55.268Z] 18:51:55     INFO -  @chrome://mochitests/content/chrome/mobile/android/tests/browser/chrome/test_debugger_server.html:38:3
2177
[task 2017-11-28T18:51:55.268Z] 18:51:55     INFO -  10 INFO TEST-OK | mobile/android/tests/browser/chrome/test_debugger_server.html | took 7485ms
2178
[task 2017-11-28T18:51:55.269Z] 18:51:55     INFO -  11 INFO None12 INFO TEST-START | mobile/android/tests/browser/chrome/test_desktop_useragent.html
2179
[task 2017-11-28T18:52:15.756Z] 18:52:15     INFO -  13 INFO TEST-OK | mobile/android/tests/browser/chrome/test_desktop_useragent.html | took 19582ms
Flags: needinfo?(gtatum)
Pushed by gtatum@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/48af4f2082c7
Create a new perf actor and recording panel; r=julienw
https://hg.mozilla.org/mozilla-central/rev/48af4f2082c7
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Flags: needinfo?(gtatum)
What's the plan for making this feature localizable?

This shows up in prefs hard-coded in English, but also the tool itself has hard-coded labels.
https://searchfox.org/mozilla-central/rev/33cc9e0331da8d9ff3750f1e68d72d61201176cb/devtools/client/framework/toolbox-options.js#333
(In reply to Francesco Lodolo [:flod] from comment #39)
> What's the plan for making this feature localizable?
> 
> This shows up in prefs hard-coded in English, but also the tool itself has
> hard-coded labels.
> https://searchfox.org/mozilla-central/rev/
> 33cc9e0331da8d9ff3750f1e68d72d61201176cb/devtools/client/framework/toolbox-
> options.js#333

Uh, never mind. I missed the dependency, and found bug 1418056
@flod: we also have https://github.com/devtools-html/perf.html/issues/676 to localize perf.html itself.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: