Skip to content

Add more meta flags to hide features for imported profiles#4189

Merged
mstange merged 11 commits into
firefox-devtools:mainfrom
parttimenerd:parttimenerd_more_imported_flags
Aug 18, 2022
Merged

Add more meta flags to hide features for imported profiles#4189
mstange merged 11 commits into
firefox-devtools:mainfrom
parttimenerd:parttimenerd_more_imported_flags

Conversation

@parttimenerd

@parttimenerd parttimenerd commented Aug 18, 2022

Copy link
Copy Markdown
Contributor

I propose with this PR the addition of five flags to hide information and selectors that don't make sense for many imported non-browser profiles:

  • hideNativeFrameSelection: Hide the distinguishing buttons for native vs JavaScript frames
  • hideImplementationData: Hide the "Implementation" section in the info box for frames (closes Hide "implementation" information in profiles without implementation information #3709)
  • hideSearchFoxInMenu: Hide the "Look up the function name on Searchfox" menu entry
  • hideCopyScriptURLInMenu: Hide the "Copy script URL" menu entry
  • hideStackType: Hide the stack type of frames in context menus (categories are enough for imported frames)

Sample profile file

Current production
Deploy preview

@codecov

codecov Bot commented Aug 18, 2022

Copy link
Copy Markdown

Codecov Report

Merging #4189 (1b79390) into main (aad02ed) will decrease coverage by 0.02%.
The diff coverage is 74.19%.

@@            Coverage Diff             @@
##             main    #4189      +/-   ##
==========================================
- Coverage   88.46%   88.44%   -0.03%     
==========================================
  Files         280      280              
  Lines       24527    24552      +25     
  Branches     6536     6548      +12     
==========================================
+ Hits        21697    21714      +17     
- Misses       2628     2636       +8     
  Partials      202      202              
Impacted Files Coverage Δ
src/components/flame-graph/Canvas.js 92.06% <ø> (ø)
src/components/flame-graph/FlameGraph.js 78.88% <ø> (ø)
src/components/stack-chart/Canvas.js 94.44% <ø> (ø)
src/components/stack-chart/index.js 97.77% <ø> (ø)
src/selectors/per-thread/index.js 96.15% <ø> (ø)
src/components/shared/StackSettings.js 94.73% <50.00%> (-2.49%) ⬇️
src/components/tooltip/CallNode.js 87.20% <50.00%> (-0.89%) ⬇️
src/components/shared/CallNodeContextMenu.js 89.67% <75.00%> (-0.33%) ⬇️
src/profile-logic/profile-data.js 90.40% <75.00%> (-0.15%) ⬇️
src/selectors/profile.js 96.20% <80.00%> (-0.81%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@mstange mstange left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The overall approach seems fine, but I'd like to request some different names.

For the profile.meta flags, I'd prefer names that describe the profile instead of names that tell the UI what to do. Here are some suggestions:

  • hideNativeFrameSelection + hideStackType -> usesOnlyOneStackType
  • hideImplementationData -> doesNotUseFrameImplementation
  • hideSearchFoxInMenu -> sourceCodeIsNotOnSearchfox (note the lowercase f in Searchfox)
  • hideCopyScriptURLInMenu -> nothing (see below)

For the selectors, let's use non-negated statements about the profile data.

  • getHideNativeFrameSelection + getHideStackType -> getProfileUsesMultipleStackTypes
  • getHideImplementationData -> getProfileUsesFrameImplementation
  • getHideSearchFoxInMenu -> getShouldDisplaySearchfox
  • getHideCopyScriptURLInMenu -> nothing

And for the React component prop names, please use non-negated phrasings that fit from the perspective of the component. Suggestions:

  • hideNativeFrameSelection -> allowSwitchingStackType
  • hideStackType -> displayStackType
  • hideImplementationData -> displayImplementation
  • hideSearchFoxInMenu -> displaySearchfox
  • hideCopyScriptURLInMenu -> nothing (see below)

As for "Copy Script URL", I tried it on your example profiles and it didn't copy anything. My clipboard still contained whatever I had copied before. This indicates to me that the profiler wasn't able to find any script URL - maybe it was null or an empty string? In that case, can we just hide the menu item if that happens? It may make sense to remove the hideCopyScriptURLInMenu part from this PR and take care of that menu item in a new PR.

@parttimenerd

Copy link
Copy Markdown
Contributor Author

I incorporated all your suggestions.

As for "Copy Script URL", I tried it on your example profiles and it didn't copy anything. My clipboard still contained whatever I had copied before. This indicates to me that the profiler wasn't able to find any script URL - maybe it was null or an empty string? In that case, can we just hide the menu item if that happens? It may make sense to remove the hideCopyScriptURLInMenu part from this PR and take care of that menu item in a new PR.

That's the better option for sure. I included it in this PR, as it is only a single line of added code.

@mstange mstange left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks good to me. Thanks!

Comment thread src/components/sidebar/CallTreeSidebar.js Outdated
Comment thread src/selectors/url-state.js Outdated

@mstange mstange left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good!

Comment thread src/types/profile.js
@canova canova mentioned this pull request Sep 6, 2022
mstange added a commit to mstange/samply that referenced this pull request Dec 26, 2022
This adds some of the properties which were added in firefox-devtools/profiler#4189 .
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hide "implementation" information in profiles without implementation information

3 participants