1721072 - Some cleanup, round 2

Status: RESOLVED FIXED

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

Description

[Markus Stange [:mstange]]

The Performance devtools panel, and the profiler settings (about:profiling), use the same redux state shape, actions, and reducers.
However, some parts of the state are only needed for the devtools panel and not for about:profiling, specifically everything that's related to starting/stopping the recording.

This causes some confusion in the about:profiling initializer, which still has to deal with these unneeded pieces of machinery.

In this bug I'll try to untangle that situation, and sneak some other cleanup in between.

Comment 1

[Markus Stange [:mstange]]

Attachment: Bug 1721072 - Rename recordingPrefs to recordingSettings. r=canaltinova

Sometimes these settings come from a preset, and other times they come from Firefox about:config preferences.
Calling them settings removes the ambiguity with Firefox prefs.

Other parts of the codebase call these ProfilerInitParams.

Depends on D120034

Comment 2

[Markus Stange [:mstange]]

Attachment: Bug 1721072 - Rename RecordingStateFromPreferences to RecordingSettings. r=canaltinova

This reduces confusion with the RecordingState type, which describes whether
we are currently recording.

Depends on D120143

Comment 3

[Markus Stange [:mstange]]

Attachment: Bug 1721072 - Remove recording settings translation functions. r=canaltinova

We were using the same type for settings with interval in milliseconds and
settings with interval in microseconds.
There was no type checking to ensure that we do the translation in the right places.
Now the interval is always in milliseconds, is documented as such in the type,
and the translation happens only where we read from and write to prefs.
The "preference-management" module is removed.

Depends on D120144

Comment 4

[Markus Stange [:mstange]]

Attachment: Bug 1721072 - Add getRecordingSettingsFromPrefs and tweak prefPostfix handling. r=canaltinova

Depends on D120145

Comment 5

[Markus Stange [:mstange]]

Attachment: Bug 1721072 - Remove unnecessary callback to get the objdir list. r=canaltinova

This callback was always called directly afterwards. Calling it directly
slightly changes the order, but doesn't affect anything, and the call still
happens when the profile is obtained, and not before.

Depends on D120146

Comment 6

[Markus Stange [:mstange]]

Attachment: Bug 1721072 - Move receiveProfile and symbol table functions out of the redux state, and use a react prop for it. r=canaltinova

This functionality is only needed by the devtools panel, so it doesn't make
sense to store it in the state object whose type is also used for about:profiling.
And now it's more like a component event listener, which looks more natural than
a callback function in the redux state.

Depends on D120147

Comment 7

[Markus Stange [:mstange]]

Attachment: Bug 1721072 - Rename receiveProfile to openProfilerAndDisplayProfile. r=canaltinova

Depends on D120148

Comment 8

[Markus Stange [:mstange]]

Attachment: Bug 1721072 - Move openAboutProfiling out of the redux state. r=canaltinova

I think expressing this as a react prop feels more natural. This also stops it
from polluting the state type, which is shared between both the devtools panel
and about:profiling itself.

Unfortunately, there is a small hole in the typescript coverage: If I don't pass
any OwnProps to DevToolsPresetSelection, i.e. use DevToolsPresetSelection() in
the parent render method, then the type system does not detect the missing
onEditSettingsLinkClicked prop. But when I use DevToolsPresetSelection({}) (i.e.
pass an empty OwnProps object), then it does detect it.

Depends on D120149

Comment 9

[Markus Stange [:mstange]]

Attachment: Bug 1721072 - Replace many bind calls with fat arrow functions stored in class properties. r=canaltinova

Depends on D120150

Comment 10

[Markus Stange [:mstange]]

Attachment: Bug 1721072 - Rename didRecordingUnexpectedlyStopped to recordingUnexpectedlyStopped. r=canaltinova

The old name had both 'did' and 'stopped' in it.

Depends on D120151

Comment 11

[Markus Stange [:mstange]]

Attachment: Bug 1721072 - Properly await gInit's completion. r=canaltinova

Now gDevTools.showToolboxForTab will only finish once gInit is done.
Without this, the test helper function withDevToolsPanel would sometimes
call the callback before panelWin.gStore was assigned, leading to test
failures. I noticed this when I added another await to gInit, which
affected ordering.

Depends on D120152

Comment 12

[Markus Stange [:mstange]]

Attachment: Bug 1721072 - Move isSupportedPlatform to the store initialization action. r=canaltinova

This means that we don't need to mount the ProfilerEventHandling component to get this piece of information.

Depends on D120153

Comment 13

[Markus Stange [:mstange]]

Attachment: Bug 1721072 - Stop instantiating the ProfilerEventHandling component on about:profiling. r=canaltinova

This component now only does two things:

1. It maintaings the redux state's recordingState property, which is only used
   by the RecordingButton component, which is only used in the devtools panel.
2. It stops the profiler if it's still running when the component is unloaded.

about:profiling doesn't need those things to happen.

Depends on D120154

Comment 14

[Markus Stange [:mstange]]

Attachment: Bug 1721072 - Remove two unused props from ProfilerEventHandling. r=canaltinova

Depends on D120155

Comment 15

[Markus Stange [:mstange]]

Attachment: Bug 1721072 - Fix type for isSupportedPlatform. r=canaltinova

This wasn't caught because combineReducers is currently not checked by the type system.

Depends on D120156

Comment 16

[Markus Stange [:mstange]]

Attachment: Bug 1721072 - Stop using combineReducers, for improved type coverage. r=canaltinova

Depends on D120157

Comment 17

[Markus Stange [:mstange]]

Attachment: Bug 1721072 - Run prettier on perf.d.ts. r=canaltinova

Depends on D120158

Comment 18

[Markus Stange [:mstange]]

Attachment: Bug 1721072 - Move ProfilerEventHandling initialization logic into the reducer. r=canaltinova

Depends on D120159

Comment 19

[Markus Stange [:mstange]]

Attachment: Bug 1721072 - Move recordingState updates into the reducer. r=canaltinova

What ProfilerEventHandling was doing was basically a reducer, just
not in the place where reducers are normally put.

Depends on D120160

Comment 20

[Markus Stange [:mstange]]

Attachment: Bug 1721072 - Move perfFront out of the redux state, into component props. r=canaltinova

Now it's only used in components that are used in the devtools panel.
It is no longer used for about:profiling.

Depends on D120161

Comment 21

[Markus Stange [:mstange]]

Attachment: Bug 1721072 - Stop supplying a perfFront argument to about:profiling's gInit function. r=canaltinova

Depends on D120162

Comment 22

[Markus Stange [:mstange]]

Attachment: Bug 1721072 - Clarify ActorReadyGeckoProfilerInterface situation. r=canaltinova

Depends on D120163

Comment 23

[Markus Stange [:mstange]]

Attachment: Bug 1721072 - Improve type coverage for nsIProfiler. r=canaltinova

Depends on D120164

Comment 24

[Markus Stange [:mstange]]

Attachment: Bug 1721072 - Stop exporting getSymbolsFromThisBrowser and remove its pageContext argument. r=canaltinova

This function is now only used by the keyboard shortcuts and by the popup.

Depends on D120165

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f1e6a917d46e
Rename recordingPrefs to recordingSettings. r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/54a3294ac7df
Rename RecordingStateFromPreferences to RecordingSettings. r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/4b99909058ad
Remove recording settings translation functions. r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/6fdd263f4669
Add getRecordingSettingsFromPrefs and tweak prefPostfix handling. r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/a8445267393d
Remove unnecessary callback to get the objdir list. r=canaltinova,gregtatum
https://hg.mozilla.org/integration/autoland/rev/e5f14ea648ac
Move receiveProfile and symbol table functions out of the redux state, and use a react prop for it. r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/9d4aadf04e2f
Rename receiveProfile to openProfilerAndDisplayProfile. r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/eec6c510e5c3
Move openAboutProfiling out of the redux state. r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/f58ccd08bec7
Replace many bind calls with fat arrow functions stored in class properties. r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/c39344d8ee2e
Rename didRecordingUnexpectedlyStopped to recordingUnexpectedlyStopped. r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/5d2392c3c723
Properly await gInit's completion. r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/25f7e205ee23
Move isSupportedPlatform to the store initialization action. r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/bbc11216538d
Stop instantiating the ProfilerEventHandling component on about:profiling. r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/8db115bfd265
Remove two unused props from ProfilerEventHandling. r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/8451bf8b203a
Fix type for isSupportedPlatform. r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/75a7f66953f0
Stop using combineReducers, for improved type coverage. r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/6fb6a566e37b
Run prettier on perf.d.ts. r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/b72f39d675de
Move ProfilerEventHandling initialization logic into the reducer. r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/3c50ce926db3
Move recordingState updates into the reducer. r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/b3918a3a7ec9
Move perfFront out of the redux state, into component props. r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/0b2eca930c4f
Stop supplying a perfFront argument to about:profiling's gInit function. r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/3f6969fd4420
Clarify ActorReadyGeckoProfilerInterface situation. r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/9a782a67b60b
Improve type coverage for nsIProfiler. r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/0cbc2b32a366
Stop exporting getSymbolsFromThisBrowser and remove its pageContext argument. r=canaltinova

Comment 26

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/f1e6a917d46e
https://hg.mozilla.org/mozilla-central/rev/54a3294ac7df
https://hg.mozilla.org/mozilla-central/rev/4b99909058ad
https://hg.mozilla.org/mozilla-central/rev/6fdd263f4669
https://hg.mozilla.org/mozilla-central/rev/a8445267393d
https://hg.mozilla.org/mozilla-central/rev/e5f14ea648ac
https://hg.mozilla.org/mozilla-central/rev/9d4aadf04e2f
https://hg.mozilla.org/mozilla-central/rev/eec6c510e5c3
https://hg.mozilla.org/mozilla-central/rev/f58ccd08bec7
https://hg.mozilla.org/mozilla-central/rev/c39344d8ee2e
https://hg.mozilla.org/mozilla-central/rev/5d2392c3c723
https://hg.mozilla.org/mozilla-central/rev/25f7e205ee23
https://hg.mozilla.org/mozilla-central/rev/bbc11216538d
https://hg.mozilla.org/mozilla-central/rev/8db115bfd265
https://hg.mozilla.org/mozilla-central/rev/8451bf8b203a
https://hg.mozilla.org/mozilla-central/rev/75a7f66953f0
https://hg.mozilla.org/mozilla-central/rev/6fb6a566e37b
https://hg.mozilla.org/mozilla-central/rev/b72f39d675de
https://hg.mozilla.org/mozilla-central/rev/3c50ce926db3
https://hg.mozilla.org/mozilla-central/rev/b3918a3a7ec9
https://hg.mozilla.org/mozilla-central/rev/0b2eca930c4f
https://hg.mozilla.org/mozilla-central/rev/3f6969fd4420
https://hg.mozilla.org/mozilla-central/rev/9a782a67b60b
https://hg.mozilla.org/mozilla-central/rev/0cbc2b32a366