Closed
Bug 1077441
Opened 10 years ago
Closed 10 years ago
Create an empty "performance-dev" tool
Categories
(DevTools :: Performance Tools (Profiler/Timeline), defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 36
People
(Reporter: jsantell, Assigned: jsantell)
References
Details
Attachments
(1 file, 2 obsolete files)
26.16 KB,
patch
|
jsantell
:
review+
|
Details | Diff | Splinter Review |
For new performance tool, create a shell tool to begin adding items from the current profiler and timeline tools.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → jsantell
Assignee | ||
Comment 1•10 years ago
|
||
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)
Comment 2•10 years ago
|
||
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)
Assignee | ||
Comment 3•10 years ago
|
||
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 4•10 years ago
|
||
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-
Assignee | ||
Comment 5•10 years ago
|
||
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
Assignee | ||
Comment 6•10 years ago
|
||
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)
Assignee | ||
Comment 7•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bdd869f77ff8
Comment 8•10 years ago
|
||
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+
Assignee | ||
Comment 9•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 10•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/ac42dea3a8eb
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 11•10 years ago
|
||
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
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•