[Tab selector 4] Add a getTabToThreadIndexesMap selector to get relevant threads per tab #5087
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5087 +/- ##
==========================================
- Coverage 88.49% 88.48% -0.01%
==========================================
Files 304 304
Lines 27477 27523 +46
Branches 7433 7444 +11
==========================================
+ Hits 24317 24355 +38
- Misses 2935 2943 +8
Partials 225 225 ☔ View full report in Codecov by Sentry. |
julienw
left a comment
julienw
left a comment
There was a problem hiding this comment.
Thanks for this work, I'm ready to see what's coming next :-D
Only the fact we get the top most parent in getInnerWindowIDToTabMap looks strange to me
I tried to challenge some choices in my head, but I think there was always a good reason for these choices, so looks good to me after the previous comment is fixed.
| createSelector( | ||
| getThreads, | ||
| getInnerWindowIDToTabMap, | ||
| (threads, innerWindowIDToTabMap) => { |
There was a problem hiding this comment.
This selector looks pretty big, I was thinking it could be moved (or only part of it) to profile-data.js, but I see other pages-related selectors have all the implementation inline here, so I'll let you choose.
There was a problem hiding this comment.
Good point. I think it makes sense to move it there. I'm also thinking that maybe we can create another file for things specifically for these things, like page-data or something and move all of these there. But let's keep this simple for now and think about that later. I moved this one to profile-data.js.
| return page; | ||
| }; | ||
| for (const page of pages) { | ||
| const topMostParent = getTopMostParent(page); |
There was a problem hiding this comment.
It's not clear to me why we need to get the topmost parent, it looks like page.tabID should have the right value already, right?
And in that case, similarly to above, because we have a small array usually, we could do a simple loop over all pages instead of using a map and memoizing it.
I see it's used quite a lot in the selector below so why not after all.
There was a problem hiding this comment.
Ohh, that's a great point! I totally missed it that when I was implementing. Thanks!
Later this will be used for things like gathering all the activity scores per tab to sort the tabs selector list.
…nt unnecessarily We don't need to do that because we alredy know the tabID even if it's an iframe.
| return page; | ||
| }; | ||
| for (const page of pages) { | ||
| const topMostParent = getTopMostParent(page); |
There was a problem hiding this comment.
Ohh, that's a great point! I totally missed it that when I was implementing. Thanks!
| createSelector( | ||
| getThreads, | ||
| getInnerWindowIDToTabMap, | ||
| (threads, innerWindowIDToTabMap) => { |
There was a problem hiding this comment.
Good point. I think it makes sense to move it there. I'm also thinking that maybe we can create another file for things specifically for these things, like page-data or something and move all of these there. But let's keep this simple for now and think about that later. I moved this one to profile-data.js.
[Tatsuyuki Ish] Fix type error in getPagesMap (#5063) [Nazım Can Altınova] [Tab selector 1] Add a redux state for the tab filter (#5072) [Markus Stange] Remove a test for the inverted stack chart. (#5075) [Markus Stange] Add an inverted tree test for getSamplesSelectedStates and getTreeOrderComparator (#5076) [Nazım Can Altınova] [Tab selector 2] Extract the page data in the full view (#5073) [Nazım Can Altınova] Do not crash on timeline hover/selection when a profile doesn't have any samples or markers (#5086) [Nazım Can Altınova] [Tab selector 3] Generate page information for all tabs (#5082) [Nazım Can Altınova] [Tab selector 4] Add a getTabToThreadIndexesMap selector to get relevant threads per tab (#5087) [joshuaobrien] Use the word 'archive' instead of 'zip file' in copy (#5081) [Markus Stange] Send a UserAgent header to the symbolication server again (#5103) [Julien Wajsberg] Add some console utilities to retrieve the current profile and save it to disk (#5105) [Nazım Can Altınova] Add `selectedMarker` to the console APIs (#5107) And various dependency updates.
2 participants
This is the fourth PR of #5068.
This PR creates a
getTabToThreadIndexesMapselector to be used in the next patches. It is useful to quickly get which tab uses which threads in the timeline, as well as gathering the CPU activity scores per tab to be able to sort the tab selector list. Also added some tests to check the behavior.