[Tab selector 5] Add a tab selector component and implement tab switching by canova · Pull Request #5093 · firefox-devtools/profiler

canova · 2024-08-26T11:43:26Z

This PR implements the basic tab selector component and makes it possible to switch between tabs.

Currently there are 3 main things missing, I will follow-up with these PRs after this one.

Sort the tab selector list by their thread activity.
Get the real names of web extensions.
Add icons of the domains inside the tab selector.

And then, I would like to sort the threads in the timeline differently if it's filtered by tab. Currently the parent process is always at the top. I would like to put the most active content process to the top if a tab is selected.

Deploy preview

codecov · 2024-08-26T11:52:17Z

Codecov Report

Attention: Patch coverage is 86.80556% with 19 lines in your changes missing coverage. Please review.

Project coverage is 88.48%. Comparing base (fb85b0b) to head (b5c0fd4).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
src/profile-logic/tracks.js	81.66%	11 Missing ⚠️
src/components/app/ProfileFilterNavigator.js	76.00%	5 Missing and 1 partial ⚠️
src/actions/receive-profile.js	93.93%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5093      +/-   ##
==========================================
- Coverage   88.48%   88.48%   -0.01%     
==========================================
  Files         304      305       +1     
  Lines       27523    27655     +132     
  Branches     7444     7500      +56     
==========================================
+ Hits        24355    24470     +115     
- Misses       2943     2957      +14     
- Partials      225      228       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

julienw

Thanks, this is exciting work!

My main comments are:

the selector should look more like a button, for affordance reasons. It should react on hover especially. But there are some reusable classes that you can look at.
I think that the algorithms to generate the list of tracks could be made better, unless you have some more things coming. Especially you're usually filtering after generating arrays of everything, while we could avoid adding them in the first place. I guess this comment is wrong if you intend to cache the result of adding all tracks though, so please tell me what your intent is.
what's the story for Android where one content process is shared between several tab ids? Or do we expect that Fission will be enabled soon? (Or maybe it already is and I just don't know.
(as a follow-up) I find the behavior that we can't hide the list when clicking on the button again very disturbing, do you think we can make this happen? If use a state to keep the information of whether the menu is displayed, we could maybe decide whether to showMenu or hideMenu, and also add some aria and styles to the button. Which makes me think that this "button + menu" behavior could possibly be extracted to another component.

a miriad of small things !

Thanks!

julienw · 2024-08-29T13:47:54Z

    for (let i = 0; i < thread.frameTable.length; i++) {
      const innerWindowID = thread.frameTable.innerWindowID[i];
-      if (innerWindowID === null) {
+      if (innerWindowID === null || innerWindowID === 0) {


(idea for future optimization) if we used 0 always instead of null, maybe we could use a TypedArray for innerWindowID

julienw · 2024-08-29T13:56:00Z

+.react-contextmenu-item.tabSelectorMenuItem.checked:not(
+    .react-contextmenu-item--disabled
+  )::before {
+  /* Move the checkmark to the left instead of right, as it's logically better. */


I wonder if we should evolve the context menu lib instead of this hack :-)

I'm not so sure about it. It's indeed better to update the context menu, but since it's also published in npm, we need to create a major release just in case someone else is using. Updating the context menu, bumping the version, releasing it, then updating the version on the profiler is a lot more work :) I would prefer to do that as a follow-up.

Note that it would be a minor version probably, not a major, because it wouldn't break anything. I'm fine with a follow-up.

julienw · 2024-08-29T17:53:44Z

+  // At the end, we need to filter global tracks by current tab.
+  return filterGlobalTracksByTab(
+    globalTracks,
+    profile,
+    tabID,
+    tabToThreadIndexesMap
+  );


I find that the way the function idsdone is a bit strange

Add all global tracks to an array

Sort them

Like an afterthought, remove all the ones that shouldn't be there

I think it would be possible to not add them in the first place, in step 1.
The small algorithm change might be where we have The main thread index was found, add it.. And also deal with the case of tabToThreadIndexesMap or the retrieved threadIndexes being unavailable but I'm sure you can deal with that.

I believe the logic could be simpler. At least I'd like to see how this could look.

This was intentional due to how we construct the global tracks array. If you look at above, you'll see that there are 2 places where we construct the global process track: 1, 2

The first place is fine, it's a place where we have the mainThreadIndex. But if you look at the second place, we don't really know the mainThreadIndex of that process yet. So the branch where you pointed comes to play. We continue iterating and if we find the global track, we are adding the main thread index there. But the issue is, we already have that global track inside the globalTracks array. So what we need to do is to splice the array and remove that element after we already added it.

So this fact that we have to remove for some cases anyways, seemed more strange to me and wanted to make it a different operation. But you are right about sorting. I think we can do the sorting at the end instead of doing this at the end. So I changed it with this, but let me know what you think.

julienw · 2024-08-29T18:00:02Z

+  // Next, filter the tracks by the tab selector threads.
+  scores = scores.filter(({ threadIndex }) => allTrackThreads.has(threadIndex));


Instead of filtering as an afterthought, would it be possible to use the global and local tracks information directly and generate the scores only for those? Unless we'd like to cache the scores for a faster switch (if the scoring computation is slow, I don't know).

Yes, it's also intentional for caching. I will follow-up with caching in the following PRs. Because we need to iterate the threads every time ew need to calculate the scores so they are not great right now.

I'm not super convinced but OK.

julienw · 2024-08-29T18:09:47Z

+    expect(getTabFilter(getState())).toBe(null);
+
+    // Change the tab filter by clicking on the menu item.
+    const mozillaTab = getByText('mozilla.org');


once you'll move to a button, you can use screen.getByRole('button', { name: 'mozilla.org' }), this also checks for the accessibility of the element (does it has the right role and the right name?). Same below.
If that doesn't work it means there's an accessibility problem (but sometimes it's not super easy to debug, so feel free to use getByText if you have issues and can't figure them out)

These are the items inside the context menu, which were added with MenuItem component. This component uses div instead of buttons still (but has some aria attributes). We need to change the libarary to implement this. Let's make it a followup.

canova

Thanks for the reviews. I updated the code and added some comments. Let me know what you think!
I changed the span into a button but it behaves slightly differently due to some limitations of how we use it. See my message below.

Edit: I left the older commits as is to make them easier to review. Last 6 commits are new.

canova · 2024-08-29T22:06:00Z

+.react-contextmenu-item.tabSelectorMenuItem.checked:not(
+    .react-contextmenu-item--disabled
+  )::before {
+  /* Move the checkmark to the left instead of right, as it's logically better. */


I'm not so sure about it. It's indeed better to update the context menu, but since it's also published in npm, we need to create a major release just in case someone else is using. Updating the context menu, bumping the version, releasing it, then updating the version on the profiler is a lot more work :) I would prefer to do that as a follow-up.

canova · 2024-08-29T23:19:50Z

+
+    // TODO: Remove this once we ship the tab selector and remove the active tab view.
+    const filterPageDataForActiveTab = getProfileFilterPageData(state);
+    const pageDataByTabID = getProfileFilterPageDataByTabID(state);


See the second item in the PR message. And see the deploy preview PR with the correct names. My next PR is fixing that.

canova · 2024-08-29T23:30:43Z

  const localTracksByPid = new Map();

+  // Create a new set of avaiable pids, so we can filter out the local tracks
+  // if their globalTracks are also filtered out by the tab selector.


I initially didn't add this change but it was failing to render without throwing any errors during the timeline component rendering. I think we iterate over these localTracksByPid and try to find their global tracks out of it. If they don't exist, it throws an error. (This was the error we hunted down together during the perftools work week :))

canova · 2024-08-30T09:24:08Z

+  tabID: TabID | null,
+  tabToThreadIndexesMap: Map<ThreadIndex, Set<TabID>>
+): GlobalTrack[] {
+  if (tabID === null) {


As far as I know it's not possible to get a zero tabID from the backend, it should be always non-zero. But you're right, the get operation will always return nothing.

canova · 2024-08-30T11:34:23Z

+          tabFilter !== null ? pageDataByTabID.get(tabFilter) : null;
+
+        firstItem = (
+          <span


I initially made it a span because there could be a button that wraps it and didn't want to create nested buttons as they are not valid. For example when you commit a range, it will create a button around that first item so you can navigate back. I changed the code to create a button, only when there is no committed range.

canova · 2024-08-30T11:39:23Z

+    expect(getTabFilter(getState())).toBe(null);
+
+    // Change the tab filter by clicking on the menu item.
+    const mozillaTab = getByText('mozilla.org');


These are the items inside the context menu, which were added with MenuItem component. This component uses div instead of buttons still (but has some aria attributes). We need to change the libarary to implement this. Let's make it a followup.

canova · 2024-08-30T11:59:27Z

+  // At the end, we need to filter global tracks by current tab.
+  return filterGlobalTracksByTab(
+    globalTracks,
+    profile,
+    tabID,
+    tabToThreadIndexesMap
+  );


This was intentional due to how we construct the global tracks array. If you look at above, you'll see that there are 2 places where we construct the global process track: 1, 2

The first place is fine, it's a place where we have the mainThreadIndex. But if you look at the second place, we don't really know the mainThreadIndex of that process yet. So the branch where you pointed comes to play. We continue iterating and if we find the global track, we are adding the main thread index there. But the issue is, we already have that global track inside the globalTracks array. So what we need to do is to splice the array and remove that element after we already added it.

So this fact that we have to remove for some cases anyways, seemed more strange to me and wanted to make it a different operation. But you are right about sorting. I think we can do the sorting at the end instead of doing this at the end. So I changed it with this, but let me know what you think.

canova · 2024-08-30T12:04:44Z

+  // Next, filter the tracks by the tab selector threads.
+  scores = scores.filter(({ threadIndex }) => allTrackThreads.has(threadIndex));


Yes, it's also intentional for caching. I will follow-up with caching in the following PRs. Because we need to iterate the threads every time ew need to calculate the scores so they are not great right now.

canova · 2024-08-30T13:32:38Z

Also answering some of the questions you had in the main review comment:

the selector should look more like a button, for affordance reasons. It should react on hover especially. But there are some reusable classes that you can look at.

That's fixed now.

what's the story for Android where one content process is shared between several tab ids? Or do we expect that Fission will be enabled soon? (Or maybe it already is and I just don't know.

Currently, we are going from tabs (from pages array) -> tracks. Not from tracks -> tabs. Because of this direction, it doesn't matter if a thread has multiple origins in it or not. It will always show all the tabs that we have in the profile and filter the tracks accordingly. Let's say that we have Web Content #1 track, and it has both mozilla.org and cnn.com origins. Since the pages array has both of them, we will properly show 2 tabs in the selector list. And selecting both of these items will result in showing Web Content #1 track.

(as a follow-up) I find the behavior that we can't hide the list when clicking on the button again very disturbing, do you think we can make this happen? If use a state to keep the information of whether the menu is displayed, we could maybe decide whether to showMenu or hideMenu, and also add some aria and styles to the button. Which makes me think that this "button + menu" behavior could possibly be extracted to another component.

Yeah we can do that as a follow-up. I didn't do it yet since it was similar on other places, but they are annoying there as well.

julienw · 2024-09-03T14:03:13Z

what's the story for Android where one content process is shared between several tab ids? Or do we expect that Fission will be enabled soon? (Or maybe it already is and I just don't know.

Currently, we are going from tabs (from pages array) -> tracks. Not from tracks -> tabs. Because of this direction, it doesn't matter if a thread has multiple origins in it or not. It will always show all the tabs that we have in the profile and filter the tracks accordingly. Let's say that we have Web Content #1 track, and it has both mozilla.org and cnn.com origins. Since the pages array has both of them, we will properly show 2 tabs in the selector list. And selecting both of these items will result in showing Web Content #1 track.

If the user selects cnn.com, will they see the data for both cnn.com and mozilla.org origins?

julienw

thanks!
let's move forward with that
I only have a few comments but you can merge after that!

julienw · 2024-09-04T13:23:52Z

    return new Set(profile.meta.initialVisibleThreads);
  }

+  const allTrackThreads = new Set();


optional super nit: if we're keeping it now, I'd move the computation of allTrackThreads to a separate function, so that this function is more readable (and easily memoizable too).

julienw · 2024-09-04T13:24:26Z

+  // Next, filter the tracks by the tab selector threads.
+  scores = scores.filter(({ threadIndex }) => allTrackThreads.has(threadIndex));


I'm not super convinced but OK.

julienw · 2024-09-04T13:28:55Z

  const localTracksByPid = new Map();

+  // Create a new set of avaiable pids, so we can filter out the local tracks
+  // if their globalTracks are also filtered out by the tab selector.


I see... I guess this makes this Map more consistent. I would have liked that we don't have to regenerate every object but rather hide or "skip over" some elements instead. But I think this works for now.

…creation Because Firefox backend always assumes the zero value is null in their internal represantion.

Currently we are adding this only, it will be used in the following commits.

canova

Thanks for the review. I addressed your comments and disabled the tab selector for now until we land the other PRs. I'll land this soon.

canova · 2024-09-18T10:33:04Z

  const localTracksByPid = new Map();

+  // Create a new set of avaiable pids, so we can filter out the local tracks
+  // if their globalTracks are also filtered out by the tab selector.


Yeah, we can look at implementing it that way later.

[Julien Wajsberg] Two optimizations for the marker chart (#5121) [Nazım Can Altınova] [Tab selector 5] Add a tab selector component and implement tab switching (#5093) [Julien Wajsberg] Support profiling from the toolbox in Thunderbird Release (#5135) [Richard Fine] Add a dedicated symbolication tool (#5123) [Julien Wajsberg] Export a tool to extract gecko logs from a profile (#4973) Shout-out to our localizers: de: Michael Köhler el: Jim Spentzos en-CA: chutten en-GB: Ian Neal es-CL: ravmn fr: Théo Chevalier fy-NL: Fjoerfoks ia: Melo46 it: Francesco Lodolo [:flod] nl: Mark Heijl pt-BR: Marcelo Ghelman ru: Valery Ledovskoy sv-SE: Andreas Pettersson uk: Lobodzets zh-CN: Olvcpr423 zh-TW: Pin-guang Chen

canova requested a review from a team as a code owner August 26, 2024 11:43

canova requested review from julienw and removed request for a team August 26, 2024 11:43

canova force-pushed the tab-selector-5 branch from f9a688d to 6c8cf17 Compare August 26, 2024 11:47

flodolo approved these changes Aug 26, 2024

View reviewed changes

Comment thread locales/en-US/app.ftl Outdated

canova force-pushed the tab-selector-5 branch from 6c8cf17 to fd7dede Compare August 27, 2024 09:52

julienw reviewed Aug 29, 2024

View reviewed changes

canova commented Aug 30, 2024

View reviewed changes

canova force-pushed the tab-selector-5 branch from 11862c0 to b5c0fd4 Compare August 30, 2024 13:26

canova requested a review from julienw August 30, 2024 13:35

julienw approved these changes Sep 4, 2024

View reviewed changes

canova added 14 commits September 18, 2024 12:43

Also check if innerWindowIDs are zero during tab to thread index map …

caaa4e0

…creation Because Firefox backend always assumes the zero value is null in their internal represantion.

Add the TabSelectorMenu component

3498a6d

Currently we are adding this only, it will be used in the following commits.

Add some proper urls to the fixture pages array

28cc4fc

Add simple tests for TabSelectorMenu

22ebc4d

Add TabSelectorMenu to FilterNavigatorBar

7070366

Change the cursor of tab selector

9b382a2

Implement tab switching on tab selector

2d49cc1

Add some tests for tab switching behavior

509d3f8

Make the tab selector scrollable if the list is too long

4d37bcd

Address the review comments for ProfileFilterNavigator

6c9d3ba

Address the minor review comments

c2f1e3e

Do no use container in tests anymore

6ff6a50

Convert the tab selector span into a button

7997eae

Filter first the global tracks before sorting them

749683e

canova force-pushed the tab-selector-5 branch from b5c0fd4 to 2f5c2de Compare September 18, 2024 10:46

Address the last review comments

5b1b279

Hide the tab selector for now

2b2b434

canova force-pushed the tab-selector-5 branch from 2f5c2de to 2b2b434 Compare September 18, 2024 10:51

canova commented Sep 18, 2024

View reviewed changes

canova merged commit 7562508 into firefox-devtools:main Sep 18, 2024

julienw mentioned this pull request Sep 23, 2024

Deploy Sep 23, 2024 #5137

Merged

		// Next, filter the tracks by the tab selector threads.
		scores = scores.filter(({ threadIndex }) => allTrackThreads.has(threadIndex));

Conversation

canova commented Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

julienw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

canova left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

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 commented Aug 30, 2024

Uh oh!

julienw commented Sep 3, 2024

Uh oh!

julienw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

canova left a comment

canova commented Aug 26, 2024 •

edited

Loading

codecov Bot commented Aug 26, 2024 •

edited

Loading

canova left a comment •

edited

Loading