Jordan Santell [:jsantell] [@jsantell] Reporter
	Description • 10 years ago

This will enable the performance tool when we're ready to ship it for (expected) Fx38. Things we need to do for this:

* Disable ability to run either profiler or timeline
* Remove build flags to enable performance tool
* Remove `devtools.performance_dev.enabled` and change to `devtools.performance.enabled`
* Change toolbox options 'show-platform-data' from `devtools.profiler.ui.show-platform-data` to `devtools.performance.ui.show-platform-data`

	Victor Porof [:vporof][:vp] Assignee
	Comment 1 • 10 years ago

* Adapt all existing profiler and timeline tests to the new perf tool
* Move all shared files into the devtools/performance folder
* Remove all dead code

	Tim Nguyen :ntim
	Comment 2 • 10 years ago

Just added dependency on bugs that I judged necessary to fix before enabling the new tool and before removing the current ones.

	Tim Nguyen :ntim
	Comment 3 • 10 years ago

Feel free to remove/add some if you disagree though.

	Victor Porof [:vporof][:vp] Assignee
	Comment 4 • 10 years ago

Enabled tests on gum: https://treeherder.mozilla.org/#/jobs?repo=gum

There's two types of failures afaict:
1. Shared profiler connections conflicting, and making browser_toolbox_* tests fail, because they try to open both the timeline and the new perf tool.
2. Crashes, like bug 1130669

I suggest we land a patch on gum that disables the old timeline and profiler (not just the tests, but the tools too, because we want to avoid the toolbox opening both of them and the new perf tool at the same time), and see what still fails.

	Jordan Santell [:jsantell] [@jsantell] Reporter
	Comment 5 • 10 years ago

gDevToolsBrowser._connectToProfiler will have to be changed over to use new profiler front.

Have a patch for this switch, disabling timeline/profiler and their tests. Thoughts on moving them to browser/devtools/deprecated/{timeline|profiler}? Or just removing them? Would like easy access to any remaining tests we want to copy over, etc.

Waiting to wrap up bug 1101235 for this as well.

	Jordan Santell [:jsantell] [@jsantell] Reporter
	Comment 6 • 10 years ago

This is a patch that does the following:

* enables perf++ by default, no longer needing build flag
* No longer builds profiler/timeline tools, and tests will not run
* Convert over remaining prefs to performance (ui-show-platform)
* framework uses new front from perf rather than profiler

NOT TO BE LANDED IN FX-TEAM YET

	Victor Porof [:vporof][:vp] Assignee
	Comment 7 • 10 years ago

Comment on attachment 8563038 [details] [diff] [review]
1123815-turn-on-perf.patch do not land on fx-team yet

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

::: browser/app/profile/firefox.js
@@ -1407,5 @@
>  pref("devtools.debugger.ui.variables-sorting-enabled", true);
>  pref("devtools.debugger.ui.variables-only-enum-visible", false);
>  pref("devtools.debugger.ui.variables-searchbox-visible", false);
>  
> -// Enable the Profiler

Keep the "Enable the ..." comment.

::: browser/devtools/framework/gDevTools.jsm
@@ +1170,5 @@
>     * Connects to the SPS profiler when the developer tools are open. This is
>     * necessary because of the WebConsole's `profile` and `profileEnd` methods.
>     */
>    _connectToProfiler: function DT_connectToProfiler(event, toolbox) {
> +    let ProfilerFront = devtools.require("devtools/performance/front");

You're not really importing the "ProfilerFront". Maybe rename to SharedPerformanceUtils?

::: browser/devtools/main.js
@@ -45,5 @@
>  const styleEditorProps = "chrome://browser/locale/devtools/styleeditor.properties";
>  const shaderEditorProps = "chrome://browser/locale/devtools/shadereditor.properties";
>  const canvasDebuggerProps = "chrome://browser/locale/devtools/canvasdebugger.properties";
>  const webAudioEditorProps = "chrome://browser/locale/devtools/webaudioeditor.properties";
>  const profilerProps = "chrome://browser/locale/devtools/profiler.properties";

File a bug to rename profiler.properties/dtd to performance.properties/dtd and all the variables here.

@@ +248,3 @@
>  Tools.performance = {
>    id: "performance",
>    ordinal: 19,

Don't forget to change the ordinals. Keep the performance ordinal where the old profiler used to be.

@@ +374,3 @@
>    Tools.netMonitor,
>    Tools.storage,
> +  Tools.performance,

Performance before netMonitor.

	Jordan Santell [:jsantell] [@jsantell] Reporter
	Comment 8 • 10 years ago

Fixed nits. We have an audit of localized strings in bug 1082695, but renaming this would cause all previous strings to have to be re-localized, I believe, so that's something we should investigate if its worth renaming or not

	Victor Porof [:vporof][:vp] Assignee
	Comment 9 • 10 years ago

https://hg.mozilla.org/projects/gum/rev/a03794f349b5

	Victor Porof [:vporof][:vp] Assignee
	Comment 10 • 10 years ago

For the record, these are the changesets that are currently on gum *BUT NOT* on fx-team:

1. Build Performance++ tool on gum only: 
https://hg.mozilla.org/projects/gum/rev/70f69456c140

2. Enable performance tools by default, disable profiler and timeline tools/tests:
https://hg.mozilla.org/projects/gum/rev/a03794f349b5

	Victor Porof [:vporof][:vp] Assignee
	Comment 13 • 10 years ago

Landed on gum. Current patches on gum but not fx-team:

1. Build Performance++ tool on gum only: 
https://hg.mozilla.org/projects/gum/rev/70f69456c140

2. Enable performance tools by default, disable profiler and timeline tools/tests:
https://hg.mozilla.org/projects/gum/rev/a03794f349b5

3. Use `devtools.performance.enabled` instead of `devtools.profiler.enabled`
https://hg.mozilla.org/projects/gum/rev/9332647ea6ac

4. Use `performance` instead of `jsprofiler` as the tool id in some tests
https://hg.mozilla.org/projects/gum/rev/c7e286c5c6a4

	Victor Porof [:vporof][:vp] Assignee
	Comment 14 • 10 years ago

Comment on attachment 8563052 [details] [diff] [review]
1123815-turn-on-perf.patch [do not land on fx-team yet]

Marking as obsolete because bugzilla-todos is bugging me.

	Jordan Santell [:jsantell] [@jsantell] Reporter
	Comment 15 • 10 years ago

Should note that those 3 patches should be ultimately landed on fx-team when we're ready (the initial patch with build flag is not needed as the subsequent patch removes that)

Attachment 8563052 [details] [diff]
Attachment 8563402 [details] [diff]
Attachment 8563405 [details] [diff]

	Victor Porof [:vporof][:vp] Assignee
	Comment 16 • 10 years ago

For the record, here is the diff between fx-team and gum.

	Andrei Petcu
	Comment 17 • 10 years ago

Hi! I want to try out this new performance tool. I have the latest nightly and I set "devtools.performance_dev.enabled;true" but the Performance tab shows "Firefox can't find the file at chrome://browser/content/devtools/performance.xul." What am I doing wrong? From what I can understand from this discussion, the new performance tool ca be used by meer mortals (like me) on a Nightly build :P

	Victor Porof [:vporof][:vp] Assignee
	Comment 18 • 10 years ago

The new perf tool is currently behind a build flag, so you'll either need to build firefox yourself using `ac_add_options --enable-devtools-perf`, or grab a gum build from https://treeherder.mozilla.org/#/jobs?repo=gum. Alternatively, wait for another week or so and hopefully this will be in Nightly prefed on by default.

	Andrei Petcu
	Comment 19 • 10 years ago

Thank you! I'll wait for a week :D

	Victor Porof [:vporof][:vp] Assignee
	Comment 20 • 10 years ago

Gum is green. Let's land this.

	Victor Porof [:vporof][:vp] Assignee
	Comment 21 • 10 years ago

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

	Tim Nguyen :ntim
	Comment 22 • 10 years ago

\o/

	Phil Ringnalda (:philor)
	Comment 23 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/2ae4dac8f094

	Tim Nguyen :ntim
	Comment 24 • 10 years ago

Release Note Request (optional, but appreciated)
[Why is this notable]: New performance tools
[Suggested wording]: Developer Tools : New performance tools
[Links (documentation, blog post, etc)]: Possibly hacks.mo.org in the future.

	Brian Grinstead [:bgrins]
	Comment 25 • 10 years ago

(In reply to Tim Nguyen [:ntim] from comment #24)
> Release Note Request (optional, but appreciated)
> [Why is this notable]: New performance tools
> [Suggested wording]: Developer Tools : New performance tools
> [Links (documentation, blog post, etc)]: Possibly hacks.mo.org in the future.

I think we want to wait until 40 to announce this so there is more time to bugfix, add features, etc.

	Victor Porof [:vporof][:vp] Assignee
	Comment 26 • 10 years ago

If anything, the relnote should be fore bug 1075567

	Victor Porof [:vporof][:vp] Assignee
	Comment 27 • 10 years ago

s/fore/for/