Closed Bug 1077441 Opened 10 years ago Closed 10 years ago

Create an empty "performance-dev" tool

Categories

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

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 36

People

(Reporter: jsantell, Assigned: jsantell)

References

Details

Attachments

(1 file, 2 obsolete files)

For new performance tool, create a shell tool to begin adding items from the current profiler and timeline tools.
Assignee: nobody → jsantell
Blocks: 1077442
Do you have a specific way in mind of having this a "build only" option? We can add it to ./browser/devtools/main as a tool, just commented out, for testing, or should we be more sophisticated?
Flags: needinfo?(paul)
See how we landed WebIDE: https://hg.mozilla.org/mozilla-central/rev/85c60372f2bf

Interesting files:
https://hg.mozilla.org/mozilla-central/diff/85c60372f2bf/configure.in
https://hg.mozilla.org/mozilla-central/diff/85c60372f2bf/browser/app/profile/firefox.js
https://hg.mozilla.org/mozilla-central/diff/85c60372f2bf/browser/devtools/moz.build

In mozconfig, we just needed to add --enable-webide. Eventually, we removed this code once we were happy with tests, UI and l10n.
Flags: needinfo?(paul)
When building FF, add `ac_add_options --enable-devtools-perf` to make the "Performance++" panel appear. This creates an option `devtools.performance_dev.enable` and sets `MOZ_DEVTOOLS_PERFTOOLS` to be consumed elsewhere.

The performance tools only get built if this build option is set.

The styling and locale (which is using profiler's locale so we dont have to change strings for the time being) will always be built however, and I don't think that'll be an issue.

Lots of things commented out in the actual tool, because I had to pull out other things I was working out during the work week, but those will be cleaned up with the performance front wrapper we discussed.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1cad819f6ba8
Attachment #8504360 - Flags: review?(vporof)
Comment on attachment 8504360 [details] [diff] [review]
1077441-skeleton-perf-tools.patch

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

Perfect start, but see comments.

::: browser/devtools/main.js
@@ +278,5 @@
> +Tools.performance = {
> +  id: "performance",
> +  accesskey: l10n("profiler.accesskey", profilerStrings),
> +  key: l10n("profiler.commandkey2", profilerStrings),
> +  ordinal: 19,

Peculiar ordinal, but whatever works?

@@ +284,5 @@
> +  icon: "chrome://browser/skin/devtools/tool-profiler.svg",
> +  invertIconForLightTheme: true,
> +  url: "chrome://browser/content/devtools/performance.xul",
> +  label: "Performance++", //l10n("profiler.label2", profilerStrings),
> +  panelLabel: "Performance++", //l10n("profiler.panelLabel2", profilerStrings),

Should file a bug for localizing these, and block the meta.

::: browser/devtools/performance/panel.js
@@ +17,5 @@
> +loader.lazyRequireGetter(this, "getPerformanceConnection",
> +  "devtools/performance/shared", true);
> +loader.lazyRequireGetter(this, "PerformanceFront",
> +  "devtools/performance/shared", true);
> +*/

Let's not land dead code. Remove this.

@@ +38,5 @@
> +   */
> +  open: Task.async(function*() {
> +    // let connection = getPerformanceConnection(this._toolbox);
> +    // yield connection.open();
> +

Ditto.

@@ +45,5 @@
> +
> +    // Mock Front for now
> +    let gFront = {};
> +    EventEmitter.decorate(gFront);
> +    // this.panelWin.gFront = new PerformanceFront(connection);

Ditto.

::: browser/devtools/performance/performance.js
@@ +41,5 @@
> +  "resource:///modules/devtools/Graphs.jsm");
> +devtools.lazyImporter(this, "CanvasGraphUtils",
> +  "resource:///modules/devtools/Graphs.jsm");
> +devtools.lazyImporter(this, "SideMenuWidget",
> +  "resource:///modules/devtools/SideMenuWidget.jsm");

I don't think all of these imports are used. It will make more sense to add them as the tool is being implemented. It'll also be useful to avoid landing dead code.

@@ +50,5 @@
> +const CATEGORIES_GRAPH_HEIGHT = 60; // px
> +const CATEGORIES_GRAPH_MIN_BARS_WIDTH = 3; // px
> +const CALL_VIEW_FOCUS_EVENTS_DRAIN = 10; // ms
> +const GRAPH_SCROLL_EVENTS_DRAIN = 50; // ms
> +const GRAPH_ZOOM_MIN_TIMESPAN = 20; // ms

Ditto for the consts.
Actually, ditto for everything else.

@@ +104,5 @@
> +  yield promise.all([
> +    PrefObserver.register(),
> +    EventsHandler.initialize(),
> +    //RecordingsListView.initialize(),
> +    //ProfileView.initialize()

Ditto. You get the idea.

::: browser/devtools/performance/performance.xul
@@ +11,5 @@
> +  <!ENTITY % profilerDTD SYSTEM "chrome://browser/locale/devtools/profiler.dtd">
> +  %profilerDTD;
> +]>
> +
> +<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">

There's waaay too much xul in here for a panel that does nothing. Let's land all of this in a different bug, this should be an *empty* perf tool.

::: browser/locales/en-US/chrome/browser/devtools/profiler.dtd
@@ +42,5 @@
> +<!ENTITY profilerUI.overview.selectTimeView "View by time">
> +
> +<!-- LOCALIZATION NOTE (profilerUI.timeilne.selectFrameView): This string is
> +  -  displayed on a button that selects the "frame" view in the overview panel. -->
> +<!ENTITY profilerUI.overview.selectFrameView "View by frame">

Should we land this strings here? Don't think so.

::: browser/themes/linux/devtools/performance.css
@@ +4,5 @@
> +
> +%include ../../shared/devtools/performance.inc.css
> +
> +#profile-content tab label {
> +  margin-bottom: 4px;

Not used yet. Remove.

::: browser/themes/linux/jar.mn
@@ +255,5 @@
> +* skin/classic/browser/devtools/performance.css       (devtools/performance.css)
> +* skin/classic/browser/devtools/timeline.css          (devtools/timeline.css)
> +* skin/classic/browser/devtools/scratchpad.css        (devtools/scratchpad.css)
> +* skin/classic/browser/devtools/shadereditor.css      (devtools/shadereditor.css)
> +* skin/classic/browser/devtools/splitview.css         (../shared/devtools/splitview.css)

What's splitview doing here?

@@ +258,5 @@
> +* skin/classic/browser/devtools/shadereditor.css      (devtools/shadereditor.css)
> +* skin/classic/browser/devtools/splitview.css         (../shared/devtools/splitview.css)
> +  skin/classic/browser/devtools/styleeditor.css       (../shared/devtools/styleeditor.css)
> +  skin/classic/browser/devtools/storage.css           (../shared/devtools/storage.css)
> +* skin/classic/browser/devtools/webaudioeditor.css    (devtools/webaudioeditor.css)

Or this?

::: browser/themes/shared/devtools/performance.inc.css
@@ +1,4 @@
> +/* vim:set ts=2 sw=2 sts=2 et: */
> +/* 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/. */

So. Much. Unused. CSS.
Attachment #8504360 - Flags: review?(vporof) → review-
Comment on attachment 8504360 [details] [diff] [review]
1077441-skeleton-perf-tools.patch

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

::: browser/devtools/main.js
@@ +278,5 @@
> +Tools.performance = {
> +  id: "performance",
> +  accesskey: l10n("profiler.accesskey", profilerStrings),
> +  key: l10n("profiler.commandkey2", profilerStrings),
> +  ordinal: 19,

Number.MAX_VALUE!?

@@ +284,5 @@
> +  icon: "chrome://browser/skin/devtools/tool-profiler.svg",
> +  invertIconForLightTheme: true,
> +  url: "chrome://browser/content/devtools/performance.xul",
> +  label: "Performance++", //l10n("profiler.label2", profilerStrings),
> +  panelLabel: "Performance++", //l10n("profiler.panelLabel2", profilerStrings),

These labels should be the same currently ("Performance" is what profiler is using), but the panelLabel should probably be updated. I'll make a meta "locales" strings for the big components.

::: browser/devtools/performance/performance.xul
@@ +11,5 @@
> +  <!ENTITY % profilerDTD SYSTEM "chrome://browser/locale/devtools/profiler.dtd">
> +  %profilerDTD;
> +]>
> +
> +<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">

Removed most XUL

::: browser/locales/en-US/chrome/browser/devtools/profiler.dtd
@@ +42,5 @@
> +<!ENTITY profilerUI.overview.selectTimeView "View by time">
> +
> +<!-- LOCALIZATION NOTE (profilerUI.timeilne.selectFrameView): This string is
> +  -  displayed on a button that selects the "frame" view in the overview panel. -->
> +<!ENTITY profilerUI.overview.selectFrameView "View by frame">

removed

::: browser/themes/linux/jar.mn
@@ +258,5 @@
> +* skin/classic/browser/devtools/shadereditor.css      (devtools/shadereditor.css)
> +* skin/classic/browser/devtools/splitview.css         (../shared/devtools/splitview.css)
> +  skin/classic/browser/devtools/styleeditor.css       (../shared/devtools/styleeditor.css)
> +  skin/classic/browser/devtools/storage.css           (../shared/devtools/storage.css)
> +* skin/classic/browser/devtools/webaudioeditor.css    (devtools/webaudioeditor.css)

well this was a terrible copy/paste job

::: browser/themes/shared/devtools/performance.inc.css
@@ +1,4 @@
> +/* vim:set ts=2 sw=2 sts=2 et: */
> +/* 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/. */

Nuked 300 lines
Removed a lot a lot of things, tree closed and will retry and try later (although previous one worked fine)
Attachment #8504360 - Attachment is obsolete: true
Attachment #8504837 - Flags: review?(vporof)
Comment on attachment 8504837 [details] [diff] [review]
1077441-skeleton-perf-tools.patch

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

r+ with the small changes below and possibly adding a missing file to the patch.

Build fails. Pastebin: https://pastebin.mozilla.org/6791794

::: browser/devtools/main.js
@@ +281,5 @@
> +  icon: "chrome://browser/skin/devtools/tool-profiler.svg",
> +  invertIconForLightTheme: true,
> +  url: "chrome://browser/content/devtools/performance.xul",
> +  label: "Performance++", //l10n("profiler.label2", profilerStrings),
> +  panelLabel: "Performance++", //l10n("profiler.panelLabel2", profilerStrings),

Once you file the bug l10n, add it here.

::: browser/devtools/performance/performance.js
@@ +78,5 @@
> +/**
> + * Shortcuts for accessing various profiler preferences.
> + */
> +const Prefs = new ViewHelpers.Prefs("devtools.profiler", {
> +  showPlatformData: ["Bool", "ui.show-platform-data"]

Should remove this line for now.

::: browser/devtools/performance/performance.xul
@@ +17,5 @@
> +  <script type="application/javascript" src="performance.js"/>
> +
> +  <vbox class="theme-body" flex="1">
> +    <toolbar id="performance-toolbar" class="devtools-toolbar">
> +      <hbox id="performance-toolbar-controls-profiles" class="devtools-toolbarbutton-group">

I'd rename this id to performance-toolbar-controls-recordings, since we're not only dealing with profiles.

@@ +20,5 @@
> +    <toolbar id="performance-toolbar" class="devtools-toolbar">
> +      <hbox id="performance-toolbar-controls-profiles" class="devtools-toolbarbutton-group">
> +        <toolbarbutton id="record-button"
> +                       class="devtools-toolbarbutton"
> +                       oncommand="RecordingsListView._onRecordButtonClick()"

Should remove the 'oncommand' attributes from all these buttons, since there's no corresponding controller logic yet.

@@ +38,5 @@
> +    </toolbar>
> +    <splitter class="devtools-horizontal-splitter" />
> +    <box id="overview-pane"
> +      class="devtools-responsive-container"
> +      flex="1">

Let's keep the attributes alignment consistent.
   id
   class
   flex
vs.
   id
 class
 flex

@@ +41,5 @@
> +      class="devtools-responsive-container"
> +      flex="1">
> +    </box>
> +    <splitter class="devtools-horizontal-splitter" />
> +    <box id="profile-pane"

Should rename this id to "details pane" to better fit our vocabulary.

::: browser/themes/shared/devtools/performance.inc.css
@@ +20,5 @@
> +}
> +
> +/* Overview Panel */
> +
> +#overview-controls {

This is dead css afaict. Remove it.
Attachment #8504837 - Flags: review?(vporof) → review+
Fixed all the above comments! Removed the test directory in moz.build until we have tests.

Quick smoke screen try run to ensure builds:  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c173744bf8c5
Attachment #8504837 - Attachment is obsolete: true
Attachment #8505561 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/ac42dea3a8eb
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.