From 671c350ea68a412ef45504b51d72ce6d6902ef8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Naz=C4=B1m=20Can=20Alt=C4=B1nova?= Date: Thu, 24 Feb 2022 14:01:44 +0100 Subject: [PATCH] Make the IPC tracks hidden by default during the first load --- src/profile-logic/tracks.js | 42 ++++++++++++++++++++++--- src/test/components/LocalTrack.test.js | 30 ++++++++++++++++-- src/test/components/MarkerTable.test.js | 32 +++++++++---------- src/test/components/Timeline.test.js | 26 +++++++++++++-- src/test/store/tracks.test.js | 4 +-- 5 files changed, 106 insertions(+), 28 deletions(-) diff --git a/src/profile-logic/tracks.js b/src/profile-logic/tracks.js index 3dad7795be..dc61f39596 100644 --- a/src/profile-logic/tracks.js +++ b/src/profile-logic/tracks.js @@ -711,11 +711,7 @@ function _computeHiddenTracksForVisibleThreads( const hiddenLocalTracks = new Set( localTrackOrder.filter((localTrackIndex) => { const localTrack = localTracks[localTrackIndex]; - if (localTrack.type !== 'thread') { - // Keep non-thread local tracks visible. - return false; - } - return !visibleThreadIndexes.has(localTrack.threadIndex); + return !_isLocalTrackVisible(localTrack, visibleThreadIndexes); }) ); hiddenLocalTracksByPid.set(pid, hiddenLocalTracks); @@ -1340,3 +1336,39 @@ export function getTrackReferenceFromThreadIndex( // Failed to find the thread from its thread index. return null; } + +/* + * Returns whether the local track should be visible or not. + * If the track is not a thread, some of them can be visible by default and some + * of them can be hidden to reduce the noise. This mostly depends on either the + * usefulness or the activity of that track. + * + * TODO: Check the memory track activity here to decide if it should be visible. + */ +function _isLocalTrackVisible( + localTrack: LocalTrack, + visibleThreadIndexes: Set +): boolean { + switch (localTrack.type) { + case 'thread': + // Show the local thread if it's included in the visible thread indexes. + return visibleThreadIndexes.has(localTrack.threadIndex); + case 'network': + case 'memory': + // 'event-delay' and 'process-cpu' tracks are experimental and they should + // be visible by default whenever they are included in a profile. (fallthrough) + case 'event-delay': + case 'process-cpu': + // Power tracks are there only if the power feature is enabled. So they should + // be visible by default whenever they're included in a profile. (fallthrough) + case 'power': + // Keep non-thread local tracks visible. + return true; + case 'ipc': + // IPC tracks are not always useful to the users. So we are making them hidden + // by default to reduce the noise. + return false; + default: + throw assertExhaustiveCheck(localTrack, 'Unhandled LocalTrack type.'); + } +} diff --git a/src/test/components/LocalTrack.test.js b/src/test/components/LocalTrack.test.js index d78b294efe..e3002012b3 100644 --- a/src/test/components/LocalTrack.test.js +++ b/src/test/components/LocalTrack.test.js @@ -18,6 +18,7 @@ import { render } from 'firefox-profiler/test/fixtures/testing-library'; import { changeSelectedThreads, hideLocalTrack, + showLocalTrack, } from '../../actions/profile-view'; import { TimelineLocalTrack } from '../../components/timeline/LocalTrack'; import { @@ -130,13 +131,37 @@ describe('timeline/LocalTrack', function () { }); describe('with an IPC track', function () { + it('appears hidden by default', function () { + const { container } = setupWithIPC(); + expect(container.querySelector('.timelineTrackHidden')).toBeTruthy(); + expect(container.querySelector('.timelineTrack')).toBeFalsy(); + }); + + it('can be shown', function () { + const { dispatch, pid, trackReference, container } = setupWithIPC(); + + // First check that the IPC track is hidden by default. + expect(container.querySelector('.timelineTrackHidden')).toBeTruthy(); + expect(container.querySelector('.timelineTrack')).toBeFalsy(); + + // Now make it visible and check it. + dispatch(showLocalTrack(pid, trackReference.trackIndex)); + expect(container.querySelector('.timelineTrackHidden')).toBeFalsy(); + expect(container.querySelector('.timelineTrack')).toBeTruthy(); + }); + it('correctly renders the IPC label', function () { - const { getLocalTrackLabel } = setupWithIPC(); + const { dispatch, pid, trackReference, getLocalTrackLabel } = + setupWithIPC(); + dispatch(showLocalTrack(pid, trackReference.trackIndex)); expect(getLocalTrackLabel()).toHaveTextContent('IPC — Empty'); }); it('matches the snapshot of the IPC track', () => { - const { container } = setupWithIPC(); + const { pid, dispatch, trackReference, container, flushRafCalls } = + setupWithIPC(); + dispatch(showLocalTrack(pid, trackReference.trackIndex)); + flushRafCalls(); expect(container.firstChild).toMatchSnapshot(); }); }); @@ -191,6 +216,7 @@ function setup( pid: PID, getLocalTrackLabel, getLocalTrackRow, + flushRafCalls, }; } diff --git a/src/test/components/MarkerTable.test.js b/src/test/components/MarkerTable.test.js index e1ef7be42e..1b9d549d22 100644 --- a/src/test/components/MarkerTable.test.js +++ b/src/test/components/MarkerTable.test.js @@ -334,13 +334,13 @@ describe('MarkerTable', function () { // Make sure that it's hidden. expect(getHumanReadableTracks(getState())).toEqual([ 'hide [thread GeckoMain default]', - ' - show [ipc GeckoMain]', + ' - hide [ipc GeckoMain]', 'show [thread GeckoMain tab] SELECTED', - ' - show [ipc GeckoMain] SELECTED', + ' - hide [ipc GeckoMain] SELECTED', ' - show [thread DOM Worker]', - ' - show [ipc DOM Worker]', + ' - hide [ipc DOM Worker]', ' - show [thread Style]', - ' - show [ipc Style]', + ' - hide [ipc Style]', ]); // Check the actual behavior now. @@ -352,13 +352,13 @@ describe('MarkerTable', function () { // Make sure that it's not hidden anymore. expect(getHumanReadableTracks(getState())).toEqual([ 'show [thread GeckoMain default] SELECTED', - ' - show [ipc GeckoMain] SELECTED', + ' - hide [ipc GeckoMain] SELECTED', 'show [thread GeckoMain tab]', - ' - show [ipc GeckoMain]', + ' - hide [ipc GeckoMain]', ' - show [thread DOM Worker]', - ' - show [ipc DOM Worker]', + ' - hide [ipc DOM Worker]', ' - show [thread Style]', - ' - show [ipc Style]', + ' - hide [ipc Style]', ]); }); @@ -392,13 +392,13 @@ describe('MarkerTable', function () { // Make sure that they are hidden. expect(getHumanReadableTracks(getState())).toEqual([ 'show [thread GeckoMain default] SELECTED', - ' - show [ipc GeckoMain] SELECTED', + ' - hide [ipc GeckoMain] SELECTED', 'hide [thread GeckoMain tab]', - ' - show [ipc GeckoMain]', + ' - hide [ipc GeckoMain]', ' - hide [thread DOM Worker]', - ' - show [ipc DOM Worker]', + ' - hide [ipc DOM Worker]', ' - show [thread Style]', - ' - show [ipc Style]', + ' - hide [ipc Style]', ]); // Check the actual behavior now. @@ -410,13 +410,13 @@ describe('MarkerTable', function () { // Make sure that they are not hidden anymore. expect(getHumanReadableTracks(getState())).toEqual([ 'show [thread GeckoMain default]', - ' - show [ipc GeckoMain]', + ' - hide [ipc GeckoMain]', 'show [thread GeckoMain tab]', - ' - show [ipc GeckoMain]', + ' - hide [ipc GeckoMain]', ' - show [thread DOM Worker] SELECTED', - ' - show [ipc DOM Worker] SELECTED', + ' - hide [ipc DOM Worker] SELECTED', ' - show [thread Style]', - ' - show [ipc Style]', + ' - hide [ipc Style]', ]); }); diff --git a/src/test/components/Timeline.test.js b/src/test/components/Timeline.test.js index 6021b17d88..4288be99cc 100644 --- a/src/test/components/Timeline.test.js +++ b/src/test/components/Timeline.test.js @@ -16,9 +16,11 @@ import { selectedThreadSelectors, getRightClickedTrack, getMouseTimePosition, + getLocalTracksByPid, } from 'firefox-profiler/selectors'; import { FULL_TRACK_SCREENSHOT_HEIGHT } from 'firefox-profiler/app-logic/constants'; import { ensureExists } from 'firefox-profiler/utils/flow'; +import { showLocalTrack } from 'firefox-profiler/actions/profile-view'; import { storeWithProfile } from '../fixtures/stores'; import { @@ -79,7 +81,23 @@ describe('Timeline multiple thread selection', function () { ); flushRafCalls(); - return { ...renderResult, ...store }; + const showAllIPCTracks = () => { + const localTracksByPid = getLocalTracksByPid(store.getState()); + for (const [pid, localTracks] of localTracksByPid) { + for ( + let trackIndex = 0; + trackIndex < localTracks.length; + trackIndex++ + ) { + const localTrack = localTracks[trackIndex]; + if (localTrack.type === 'ipc') { + store.dispatch(showLocalTrack(pid, trackIndex)); + } + } + } + }; + + return { ...renderResult, ...store, showAllIPCTracks }; } it('can toggle select multiple threads', function () { @@ -994,7 +1012,8 @@ describe('Timeline multiple thread selection', function () { profile.threads[7] // DOM Worker ); - const { getState } = setup(profile); + const { getState, showAllIPCTracks } = setup(profile); + showAllIPCTracks(); expect(getHumanReadableTracks(getState())).toEqual([ 'show [thread GeckoMain default]', ' - show [ipc GeckoMain]', @@ -1092,7 +1111,8 @@ describe('Timeline multiple thread selection', function () { profile.threads[7] // DOM Worker ); - const { getState } = setup(profile); + const { getState, showAllIPCTracks } = setup(profile); + showAllIPCTracks(); expect(getHumanReadableTracks(getState())).toEqual([ 'show [thread GeckoMain default]', ' - show [ipc GeckoMain]', diff --git a/src/test/store/tracks.test.js b/src/test/store/tracks.test.js index c2efba000b..b102b647b4 100644 --- a/src/test/store/tracks.test.js +++ b/src/test/store/tracks.test.js @@ -671,10 +671,10 @@ describe('ordering and hiding', function () { 'show [thread GeckoMain default]', 'show [thread GeckoMain tab] SELECTED', // Belongs to the global tab track. - ' - show [ipc GeckoMain] SELECTED', + ' - hide [ipc GeckoMain] SELECTED', ' - show [thread DOM Worker]', // Belongs to the DOM Worker local track. - ' - show [ipc DOM Worker]', + ' - hide [ipc DOM Worker]', ' - show [thread Style]', ]); });