Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show add-on name on flame graph tooltip #2921

Merged

Conversation

NisaSource
Copy link
Contributor

@NisaSource NisaSource commented Oct 12, 2020

This is issue #2790. I'm waiting for somebody to review and advise me as I see performance issue.
I have added library (add-on name) to tooltip but it isn't good from performance view because when I hover on tooltip with my mouse it is calling redux action multiple times instead of one time.

Copy link
Member

@canova canova left a comment

Choose a reason for hiding this comment

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

Thanks for the work!

So in this PR, we are using selectedNodeSelectors but that will not help us for this case because we are not "selecting" a node. We are actually "hovering" a node. So we should find a different solution for this. For example this is the function call we do to get the display information from the callNodeIndex. So, we should ideally change this function and return an additional lib in this so we can use it here.

displayData = callTree.getDisplayData(callNodeIndex);

This is the function we call there:

getDisplayData(callNodeIndex: IndexIntoCallNodeTable): CallNodeDisplayData {

You can also take a look at the getLib selector to have an idea on how we get this string.

I hope this and the comments below will give you some idea on how to fix this. Let me know if you have questions!

src/components/flame-graph/Canvas.js Outdated Show resolved Hide resolved
src/components/flame-graph/FlameGraph.js Outdated Show resolved Hide resolved
src/components/tooltip/CallNode.js Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 14, 2020

Codecov Report

Merging #2921 into main will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2921      +/-   ##
==========================================
+ Coverage   87.55%   87.58%   +0.03%     
==========================================
  Files         235      235              
  Lines       18802    18803       +1     
  Branches     4784     4784              
==========================================
+ Hits        16462    16469       +7     
+ Misses       2163     2158       -5     
+ Partials      177      176       -1     
Impacted Files Coverage Δ
src/components/tooltip/CallNode.js 87.05% <100.00%> (+7.29%) ⬆️

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 faa830c...acee7b6. Read the comment docs.

@NisaSource NisaSource marked this pull request as ready for review October 14, 2020 11:49
@julienw julienw self-requested a review October 16, 2020 14:37
Copy link
Contributor

@julienw julienw left a comment

Choose a reason for hiding this comment

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

Thanks for doing this, this is close to what was suggested the issue. But I think the suggestion wasn't the best, so I've left another suggestion which I think will give a better result.
Please try it and tell me how this looks!

src/components/tooltip/CallNode.js Outdated Show resolved Hide resolved
@julienw
Copy link
Contributor

julienw commented Oct 16, 2020

Please request review again by clicking on the small arrows in the Reviewers section at the top right of this page. Thanks!

Copy link
Contributor

@julienw julienw left a comment

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!
I added a commit that adds a test to test this. This doesn't test the case of an addon but the case of using a library, but this should exercize the same code. I did it myself because this was a bit technical and I didn't want to bother you with that.

Hey @canova, would you please have a look at this added commit? Thanks!

@julienw julienw requested a review from canova October 19, 2020 13:42
@julienw
Copy link
Contributor

julienw commented Oct 19, 2020

image
😊

Copy link
Member

@canova canova left a comment

Choose a reason for hiding this comment

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

Thanks for adding a test Julien, looks good to me!
Also thanks for working on this PR @NisaSource!

@julienw julienw force-pushed the show-library-on-flame-graph-tooltip branch from a0456e0 to acee7b6 Compare October 19, 2020 14:28
@julienw
Copy link
Contributor

julienw commented Oct 19, 2020

FYI I squashed all your commits into one, so that it was easier to rebase on top of the latest code, but still keep your authorship information for your work.

Now waiting for all tests before landing!

@julienw julienw merged commit 42bbdd0 into firefox-devtools:main Oct 19, 2020
@NisaSource
Copy link
Contributor Author

Thank you @julienw and @canova :)

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.

None yet

3 participants