Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 28 additions & 1 deletion src/actions/receive-profile.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,14 @@ import {
getActiveTabID,
getMarkerSchemaByName,
} from 'firefox-profiler/selectors';
import { getSelectedTab } from 'firefox-profiler/selectors/url-state';
import {
withHistoryReplaceStateAsync,
withHistoryReplaceStateSync,
stateFromLocation,
ensureIsValidDataSource,
} from 'firefox-profiler/app-logic/url-handling';
import { tabsShowingSampleData } from 'firefox-profiler/app-logic/tabs-handling';
import {
initializeLocalTrackOrderByPid,
computeLocalTracksByPid,
Expand All @@ -59,7 +61,10 @@ import { computeActiveTabTracks } from 'firefox-profiler/profile-logic/active-ta
import { setDataSource } from './profile-view';
import { fatalError } from './errors';
import { GOOGLE_STORAGE_BUCKET } from 'firefox-profiler/app-logic/constants';
import { determineTimelineType } from 'firefox-profiler/profile-logic/profile-data';
import {
determineTimelineType,
hasUsefulSamples,
} from 'firefox-profiler/profile-logic/profile-data';

import type {
RequestedLib,
Expand Down Expand Up @@ -345,10 +350,32 @@ export function finalizeFullProfileView(
timelineType = determineTimelineType(profile);
}

// If the currently selected tab is only visible when the selected track
// has samples, verify that the selected track has samples, and if not
// select the marker chart.
let selectedTab = getSelectedTab(getState());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Can we add a short code comment here maybe telling them that we are doing this for profiles without samples data.

if (tabsShowingSampleData.includes(selectedTab)) {
let hasSamples = false;
for (const threadIndex of selectedThreadIndexes) {
const thread = profile.threads[threadIndex];
const { samples, jsAllocations, nativeAllocations } = thread;
hasSamples = [samples, jsAllocations, nativeAllocations].some((table) =>
hasUsefulSamples(table, thread)
);
if (hasSamples) {
break;
}
}
if (!hasSamples) {
selectedTab = 'marker-chart';
}
}

withHistoryReplaceStateSync(() => {
dispatch({
type: 'VIEW_FULL_PROFILE',
selectedThreadIndexes,
selectedTab,
globalTracks,
globalTrackOrder,
localTracksByPid,
Expand Down
5 changes: 4 additions & 1 deletion src/components/timeline/TrackThread.js
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ type StateProps = {|
+isExperimentalCPUGraphsEnabled: boolean,
+maxThreadCPUDeltaPerMs: number,
+implementationFilter: ImplementationFilter,
+callTreeVisible: boolean,
|};

type DispatchProps = {|
Expand Down Expand Up @@ -137,6 +138,7 @@ class TimelineTrackThreadImpl extends PureComponent<Props> {
focusCallTree,
invertCallstack,
selectedThreadIndexes,
callTreeVisible,
} = this.props;

// Sample clicking only works for one thread. See issue #2709
Expand All @@ -149,7 +151,7 @@ class TimelineTrackThreadImpl extends PureComponent<Props> {
selectLeafCallNode(threadsKey, sampleIndex);
}

if (sampleIndex !== null) {
if (sampleIndex !== null && callTreeVisible) {
// If the user clicked outside of the activity graph (sampleIndex === null),
// then we don't need to focus the call tree. This action also selects
// the call tree panel, which we don't want either in this case.
Expand Down Expand Up @@ -396,6 +398,7 @@ export const TimelineTrackThread = explicitConnect<
isExperimentalCPUGraphsEnabled: getIsExperimentalCPUGraphsEnabled(state),
maxThreadCPUDeltaPerMs: getMaxThreadCPUDeltaPerMs(state),
implementationFilter: getImplementationFilter(state),
callTreeVisible: selectors.getUsefulTabs(state).includes('calltree'),
};
},
mapDispatchToProps: {
Expand Down
1 change: 1 addition & 0 deletions src/reducers/url-state.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ const selectedTab: Reducer<TabSlug> = (state = 'calltree', action) => {
switch (action.type) {
case 'CHANGE_SELECTED_TAB':
case 'SELECT_TRACK':
case 'VIEW_FULL_PROFILE':
return action.selectedTab;
case 'FOCUS_CALL_TREE':
return 'calltree';
Expand Down
12 changes: 12 additions & 0 deletions src/test/components/ThreadActivityGraph.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,7 @@ describe('ThreadActivityGraph', function () {
it('when clicking a stack, this selects the call tree panel', function () {
const { dispatch, getState, clickActivityGraph } = setup();

expect(getSelectedTab(getState())).toBe('calltree');
dispatch(changeSelectedTab('marker-chart'));

// The full call node at this sample is:
Expand All @@ -278,6 +279,7 @@ describe('ThreadActivityGraph', function () {
it(`when clicking outside of the graph, this doesn't select the call tree panel`, function () {
const { dispatch, getState, clickActivityGraph } = setup();

expect(getSelectedTab(getState())).toBe('calltree');
dispatch(changeSelectedTab('marker-chart'));

// There's no sample at this location.
Expand All @@ -286,6 +288,16 @@ describe('ThreadActivityGraph', function () {
expect(getLastVisibleThreadTabSlug(getState())).toBe('marker-chart');
});

it("when clicking a sample in a track with only '(root)' samples, this doesn't select the hidden call tree panel", function () {
const { profile } = getProfileFromTextSamples('(root)');
const { getState, clickActivityGraph } = setup(profile);

expect(getSelectedTab(getState())).toBe('marker-chart');

clickActivityGraph(1, 0.2);
expect(getSelectedTab(getState())).toBe('marker-chart');
});

it('will redraw even when there are no samples in range', function () {
const { dispatch } = setup();
flushDrawLog();
Expand Down
6 changes: 4 additions & 2 deletions src/test/store/profile-view.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -416,14 +416,16 @@ describe('actions/ProfileView', function () {
pid: '0',
};

it('starts out with the thread track and call tree selected', function () {
it('starts out with the thread track and marker chart selected', function () {
const profile = getNetworkTrackProfile();
const { getState } = storeWithProfile(profile);
expect(UrlStateSelectors.getSelectedThreadIndexes(getState())).toEqual(
new Set([0])
);
// The profile contains only markers, so the default tab is the
// marker-chart rather than the calltree.
expect(UrlStateSelectors.getSelectedTab(getState())).toEqual(
'calltree'
'marker-chart'
);
});

Expand Down
1 change: 1 addition & 0 deletions src/types/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,7 @@ type ReceiveProfileAction =
+hiddenLocalTracksByPid: Map<Pid, Set<TrackIndex>>,
+localTrackOrderByPid: Map<Pid, TrackIndex[]>,
+timelineType: TimelineType | null,
+selectedTab: TabSlug,
|}
| {|
+type: 'VIEW_ORIGINS_PROFILE',
Expand Down