Remove timeline graph type radio buttons by canova · Pull Request #4147 · firefox-devtools/profiler

canova · 2022-07-18T18:07:00Z

Finally I went ahead and removed our famous graph type radio buttons 😄
With this PR, it should work like explained in #3150. I've also added a function to window, so our power users still can access and change if they really need to.
This PR also removes some of the unused parts in the timeline and removes the now empty line after removing these radio buttons.

Fixes #3150
Fixes #3443

Example profile with CPU and category data: before / after
Example profile with category but without CPU data: before / after
Example profile without CPU and category data: before / after
Example chome profile: before / after
Example perf profile: before / after
Example profile for testing url upgrader: after

codecov · 2022-07-18T18:12:40Z

Codecov Report

Merging #4147 (20397bb) into main (561bc31) will increase coverage by 0.02%.
The diff coverage is 76.92%.

@@            Coverage Diff             @@
##             main    #4147      +/-   ##
==========================================
+ Coverage   88.28%   88.31%   +0.02%     
==========================================
  Files         280      280              
  Lines       24414    24408       -6     
  Branches     6490     6488       -2     
==========================================
+ Hits        21554    21555       +1     
+ Misses       2656     2650       -6     
+ Partials      204      203       -1

Impacted Files	Coverage Δ
src/app-logic/constants.js	`100.00% <ø> (ø)`
src/components/timeline/FullTimeline.js	`100.00% <ø> (+34.21%)`	⬆️
src/components/timeline/TrackThread.js	`92.20% <ø> (ø)`
src/selectors/app.js	`55.12% <ø> (-0.29%)`	⬇️
src/utils/window-console.js	`40.90% <11.11%> (-4.71%)`	⬇️
src/app-logic/url-handling.js	`86.50% <92.30%> (+0.93%)`	⬆️
src/actions/receive-profile.js	`85.84% <100.00%> (-0.26%)`	⬇️
src/components/shared/thread/ActivityGraph.js	`89.79% <100.00%> (+0.43%)`	⬆️
src/profile-logic/profile-data.js	`90.38% <100.00%> (+0.06%)`	⬆️
src/reducers/url-state.js	`96.84% <100.00%> (-0.02%)`	⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 561bc31...20397bb. Read the comment docs.

julienw

Thanks, this looks good to me!

I only have one important comment about where we can compute the timelineType. Tell me what you think!

julienw · 2022-07-19T13:01:06Z

+        ❗ The timeline type "${timelineType}" is unknown.
+        💡 Valid types are: "cpu-category", "category", or "stack".
+        Please try again 😊


Good choice of emojis 🤩

Actually I copy/pasted most of it from another function here 😄

julienw · 2022-07-19T13:13:58Z

-      search: '',
+      search: `?v=${CURRENT_URL_VERSION}`,


Just wondering why this is needed?

If the version number is not there, it tries to upgrade the url first. Since we change the default value from 'category' to 'cpu-category', it prints 'category' here in this test. But this test is not checking the upgrader at all, so it should not check this path and focus only on the default parameters.

julienw · 2022-07-19T13:14:19Z

        class="photon-label photon-label-micro timelineSettingsToggleLabel"
      >
        <input
-          checked=""


do you know why this change happened?

and now the full snapshot is removed so.... let's not focus on that :D

Yeah, I removed this test, but to give you more context. We did multiple things that affected this in the first place:

Make the cpu-category the default one

Properly upgrade the old profiles.

So the 1 makes this unchecked and 2 makes it checked again. But since it's removed, we don't need to worry about it now.

julienw · 2022-07-19T13:24:35Z

        class="photon-label photon-label-micro timelineSettingsToggleLabel"
      >
        <input
+          checked=""


oh it's back 🤔

julienw · 2022-07-19T13:24:56Z

    case 'PROFILE_LOADED':
-      if (!action.profile.meta.categories) {
-        // An imported profile did not provide its own categories. Use the stack view instead.
-        return 'stack';
-      }
      return state;


idea: use determineTimelineType here, and remove the property from the actions VIEW_..._PROFILE.

This looks conceptually more correct to me because (in theory) the reduces is where the computation should happen.

It's not 100% clear to me how everything is organized at load time, but it seems to be that this would work because the sequence is:

LOAD_PROFILE

UPDATE_URL_STATE

finalizeProfileView calling VIEW_..._PROFILE

Therefore the information from the URL would override what's set in LOAD_PROFILE, which is what we're trying to do in finalizeProfileView.

what do you think?
I'm fine if you want to do it in a separate PR, however it would be good to do it soon while it's fresh in our heads.

(note: if we decide to not do this, then you can remove thiscase block here)

You are right about the order of the actions. But there is one problem with the UPDATE_URL_STATE.
When we add it to the LOAD_PROFILE it changes the url state. But then, during the UPDATE_URL_STATE, it actually replaces the whole urlState object with a new one. Because of this, whenever we update the url state during the LOAD_PROFILE, it's going to be overwritten no matter what. Because of this, we need to do it during the finalize function to make sure that we have a new url state and we can do this addition afterwards depending on the profile data. So unfortunately, we can't change it during the LOAD_PROFILE action.

I will remove this case from the reducer instead.

julienw · 2022-07-19T14:12:18Z

+      urlState.profileSpecific.timelineType === 'cpu-category'
        ? undefined
        : urlState.profileSpecific.timelineType,


I was wondering about not adding it at all if the type is the default (computed) one. But we'd need the profile data, and we don't have it here. Then I'm fine with this.

Yeah, I think it should be easiest to make the 'cpu-category' the default. Otherwise we need the profile which makes it harder.

canova

Thanks for the review @julienw. Could you take a look at it again?

canova · 2022-07-20T13:31:34Z

+        ❗ The timeline type "${timelineType}" is unknown.
+        💡 Valid types are: "cpu-category", "category", or "stack".
+        Please try again 😊


Actually I copy/pasted most of it from another function here 😄

canova · 2022-07-20T13:33:35Z

-      search: '',
+      search: `?v=${CURRENT_URL_VERSION}`,


If the version number is not there, it tries to upgrade the url first. Since we change the default value from 'category' to 'cpu-category', it prints 'category' here in this test. But this test is not checking the upgrader at all, so it should not check this path and focus only on the default parameters.

canova · 2022-07-20T13:36:48Z

        class="photon-label photon-label-micro timelineSettingsToggleLabel"
      >
        <input
-          checked=""


Yeah, I removed this test, but to give you more context. We did multiple things that affected this in the first place:

Make the cpu-category the default one

Properly upgrade the old profiles.

So the 1 makes this unchecked and 2 makes it checked again. But since it's removed, we don't need to worry about it now.

canova · 2022-07-20T13:38:24Z

+      urlState.profileSpecific.timelineType === 'cpu-category'
        ? undefined
        : urlState.profileSpecific.timelineType,


Yeah, I think it should be easiest to make the 'cpu-category' the default. Otherwise we need the profile which makes it harder.

canova · 2022-07-20T14:12:33Z

    case 'PROFILE_LOADED':
-      if (!action.profile.meta.categories) {
-        // An imported profile did not provide its own categories. Use the stack view instead.
-        return 'stack';
-      }
      return state;


You are right about the order of the actions. But there is one problem with the UPDATE_URL_STATE.
When we add it to the LOAD_PROFILE it changes the url state. But then, during the UPDATE_URL_STATE, it actually replaces the whole urlState object with a new one. Because of this, whenever we update the url state during the LOAD_PROFILE, it's going to be overwritten no matter what. Because of this, we need to do it during the finalize function to make sure that we have a new url state and we can do this addition afterwards depending on the profile data. So unfortunately, we can't change it during the LOAD_PROFILE action.

I will remove this case from the reducer instead.

julienw

makes sense, thanks for the cleanup!

This was disabled for a long while ago and was waiting for us to re-enabled it again. But we've changed our minds about adding a checkbox here. It's not needed anymore.

canova requested a review from a team as a code owner July 18, 2022 18:07

canova requested a review from julienw July 18, 2022 18:09

julienw reviewed Jul 19, 2022

View reviewed changes

canova commented Jul 20, 2022

View reviewed changes

canova force-pushed the remove-graph-type-radio branch from 20fd22f to 0fe0988 Compare July 20, 2022 14:16

canova requested a review from julienw July 20, 2022 14:26

julienw approved these changes Jul 22, 2022

View reviewed changes

canova added 8 commits July 25, 2022 16:15

Default to cpu-category for timeline type url state and add an upgrader

08bc7ea

Determine the timeline type depending on the profile data

f348aac

Do not show the CPU usage field in the tooltip if the view is not cpu

224396f

Remove the timeline type radio buttons from the timeline

9ccd25b

Remove the unused active tab checkbox from the timeline

1261c2c

This was disabled for a long while ago and was waiting for us to re-enabled it again. But we've changed our minds about adding a checkbox here. It's not needed anymore.

Remove the timeline header

af151da

Add a toggleTimelineType function to window object for console usage

5a94ed7

Remove the unnecessary action case from timelineType selector

20397bb

canova force-pushed the remove-graph-type-radio branch from 0fe0988 to 20397bb Compare July 25, 2022 14:17

canova merged commit 2431ece into firefox-devtools:main Jul 25, 2022

canova deleted the remove-graph-type-radio branch July 25, 2022 14:30

canova mentioned this pull request Jul 26, 2022

Deploy July 26, 2022 #4156

Merged

Conversation

canova commented Jul 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jul 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

julienw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

canova left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julienw left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

canova commented Jul 18, 2022 •

edited

Loading

codecov Bot commented Jul 18, 2022 •

edited

Loading