Repository navigation
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6404 +/- ##
==========================================
+ Coverage 84.97% 85.21% +0.24%
==========================================
Files 359 365 +6
Lines 39170 40368 +1198
Branches 11115 11492 +377
==========================================
+ Hits 33284 34399 +1115
- Misses 5449 5530 +81
- Partials 437 439 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
7141c83 to
4c6f410
Compare
ca7aa16 to
18e02ab
Compare
| # $failedCount (Number) - Number of picked source files that couldn't be downloaded | ||
| MenuButtons--publish--source-code-fetch-failed = | ||
| { $failedCount -> | ||
| [one] { $failedCount } of the picked source files could not be downloaded. It has been left out, so uploading again will share the rest. |
There was a problem hiding this comment.
"could not be downloaded`: is this correct, or should it be uploaded?
There was a problem hiding this comment.
It's correct, we first download some sources that are not available before uploading.
| # $unavailableCount (Number) - Number of listed files that can't be downloaded from here | ||
| MenuButtons--publish--source-code-left-out-unavailable = | ||
| { $unavailableCount -> | ||
| [one] { $unavailableCount } file can’t be downloaded from here, which needs the browser that recorded the profile or a local symbol server. |
There was a problem hiding this comment.
| [one] { $unavailableCount } file can’t be downloaded from here, which needs the browser that recorded the profile or a local symbol server. | |
| [one] { $unavailableCount } file can’t be downloaded from here. This needs the browser that recorded the profile or a local symbol server. |
I feel like this sentence is missing something. What's "which"?
Also wondering about requires instead of needs (also in the other string).
There was a problem hiding this comment.
Updated it to use "requires" and reworded it to make it clearer.
18e02ab to
7d7e621
Compare
|
By the way, this PR is still a draft (as a deploy preview) and I intend to make some changes depending on the feedback that I get. |
7d7e621 to
ff91c6a
Compare
ff91c6a to
b554ab6
Compare
|
Overall, looks nice UI-wise to me. One note: when I opened this profile, the button is there but it's not clickable. 
|
ba74eb3 to
01fd95c
Compare
canova
changed the title
|
This is now ready for review! @mstange @fatadel note that this still depends on #6410 (the first 3 commits belong there).
Ah thanks for the feedback @fatadel! Initially I made it so that the publicly downloadable files are not downloaded (like the ones that live on github). But I think that adds confusion and it's not really what we discussed/want. So I changed the PR to make them downloadable too. So now you should be able to click on it. |
01fd95c to
1b2e80b
Compare
getRemoveProfileInformation and getSanitizedProfile each spelled out a maker, a record with one selector per mode, and a wrapper picking from it.
--width was used as a min-width, so a long unbreakable string inside the panel could stretch it, even past the edge of the window. Use it as the width. Panels without --width still shrink to fit their content. The publish panel is the only one that sets it. Also cap every arrow panel to the window, whether it sets --width or not.
Photon checkboxes draw themselves with appearance: none, so one set to indeterminate through the DOM looked unchecked. Draw it like a checked one, with a dash instead of the check mark, in both the default and micro sizes.
1b2e80b to
43726e1
Compare
| for (let stackIndex = 0; stackIndex < stackTable.length; stackIndex++) { | ||
| const frameIndex = stackTable.frame[stackIndex]; | ||
| frame[stackIndex] = frameToSource[frameIndex]; | ||
| func[stackIndex] = funcToSource[frameTable.func[frameIndex]]; |
There was a problem hiding this comment.
In what case is this different from frame[stackIndex]?
There was a problem hiding this comment.
Mostly for inlined functions. I remember that we discussed it at some point in the past and I checked to see if any bundlers do it. I don't think currently a lot of bundlers do the inlining (maybe terser and closure compiler?), but I wanted to design this around the possibility of more bundlers supporting this in the future. Also I made the funcTable.originalLocation this way initially while adding it so inlined functions are supported in the profile format.
There was a problem hiding this comment.
Can you give me an example? For C++ with inlining, we also create funcs for the inlined functions, not just frames. So I believe we'd do the same for inlining in JS, and so the func source should still be what you want.
There was a problem hiding this comment.
Ah I see what you mean. From JS we don't do that yet. Currently the source map symbolication remaps the funcs from its definition position and each frame from its own line/column. But currently we don't split frames into inlined funcs like we do for the native code. Source maps don't have the information yet for it since they map one generated position to one original position and they don't really have a great way of defining inline depth etc. I think that would be the scopes proposal.
So for example if terser inlines b from b.js into a from a.js, a sample in the inlined part has func a with a.js as its source, and a frame that maps to b.js.
If we start creating funcs for inlined JS functions, then the frame lookup would become redundant, but currently we don't do it.
Also this means that we don't really have information about the nested inlining. But that's also a limitation that comes from source maps.
There was a problem hiding this comment.
So for example if terser inlines
bfrom b.js intoafrom a.js, a sample in the inlined part has funcawith a.js as its source, and a frame that maps to b.js.
Oh. But if we don't make different funcs for those, then they don't get different call nodes. So do you ever see the frame source in the UI?
There was a problem hiding this comment.
Hmm, I think you are right. We wouldn't see a new call node for the inlined functions since we don't create funcs for those.. We could recover their names by parsing the original source and finding the function at the frame's position, like we already do for (non-inlined) funcs, but we don't do that currently.
The only place we show this information currently would be the source view (which you can only open for that file if it's also some func's file).
I guess, we can start adding funcs for those, and/or we can also drop the frame lookup in this PR in the meantime. What do you think?
There was a problem hiding this comment.
Alright, I updated the code now!
The publish panel is going to offer the source files of a profile for sharing. This adds the profile logic for it, and the next commit hooks it up: - Which files are on a sampled stack, callers included. For source-mapped code, both the file a frame ran code from and the file of its func count, since the source view opens the latter for a call node. - Which of those to offer: all of them, but only the ones we have the contents of can be picked. - How much non-idle self time each root of the file tree has, to order the roots. - The files a set of threads runs code from, for scoping to the selected threads. - A tree of the files for the panel, with chains of only children collapsed into one row, and a row for each side of a path that is both a file and a directory. The roots are ordered by their self time, and everything below them alphabetically, directories first, so that rows don't move while files get downloaded. - A copy of the profile that carries the contents of the picked files and nothing else.
Until now the sanitizer always dropped the contents of profile.shared.sources when sharing or downloading a profile. Add an "Include source code" checkbox to the publish panel, with a file tree to pick which files to include, and presets for the current file, the selected threads and all available files. The tree lists every file on a sampled stack, with the roots that have the most self time first. Files whose contents we don't have show as unavailable and can't be picked, and the option stays disabled until at least one file is downloaded. While it's off, the checkbox says how much source code is on offer. A status line under the tree says how much the picked files add, with an info icon about the files that are left out. Turning the option on picks the narrowest preset, and it starts out on when the source view shows a pickable file. Presets that would pick the same files are offered once. The picked files are kept per panel, and forgotten when another profile is loaded. The source contents are picked in a selector before sanitizePII, so that including them doesn't turn an unsanitized upload into a sanitized one and reset the URL state. The publish panel is a bit wider, so that the tree can show longer paths.
The panel didn't mention distribution rights anywhere, and the file in the source view is included by default. When the option is on in the share panel, show a warning bar under the checkbox, where it's read before picking files. The save panel doesn't get it, since saving to disk isn't distribution.
chrome://, resource:// and moz-src:// files, and the jar: URLs into omni.ja that built-in add-ons load from, each got a root of their own in the publish panel's file tree. Put them under a single root named after the product, with the install path dropped from the omni.ja ones.
Source maps name their files relative to the bundle, with schemes like webpack://, or as paths on the build machine. Nothing in those paths says which site the code ran on, so a tree of them started with ".." or "webpack://" at the root. Find the generated bundle for each original file through the func and frame tables' original locations, and put the file under the bundle's origin. The path itself is kept as the source map wrote it. Original files that are web URLs on their own are left where they are.
The Save and Share panels have their own sharing options, and the sanitized profile is memoized per mode, so the two modes never produce the same profile object. The encoding state was a single slot keyed on that object, though. Opening one panel threw away the other one's encoding, so the profile was compressed again on every panel switch, and a panel could find a blob that was encoded with the other mode's options sitting in the slot. Give each mode its own slot, and pass the mode along with the encoding actions.
Files in the publish panel's tree could only be included once their contents were in hand. Now a file is pickable when the browser connection or a local symbol server can serve it: native files through the symbolication API, given a frame address that symbolicated to them, and JS files from the browser by their source id. The browser only keeps JS sources for the tab that captured the profile, so those are only offered for profiles loaded from the browser. Only files nothing can serve show as unavailable, and files whose fetch failed say so. Picked files that aren't downloaded yet are fetched when the upload starts, with progress in the upload panel and the usual cancel. A "Download now" button under the tree does the same on request, which is also the way to get them into a saved profile. Fetches run six at a time and land in the source code cache, so the source view benefits too. If a fetch fails, the upload stops with a message, and the file is left out of the next attempt. The address proofs for all sources are computed in one pass over the frame table and memoized, and the source view uses them too.
Now that picked files can be fetched when sharing, "Selected threads" and "All available files" can set off a lot of downloads from one click. Add an "Only already downloaded files" checkbox under the presets, per panel, on by default. With it on, the presets only pick files whose contents are in hand, and toggling it re-applies the active preset. Fetchable files can still be picked by hand in the tree. The checkbox only shows up while some pickable file would have to be fetched. Turning the option on when nothing is downloaded yet would pick nothing, so in that case it falls back to the full presets and unchecks the box.
43726e1 to
a535c10
Compare
Main | Deploy preview
Fixes #4018.
Until now source code was always stripped from shared and downloaded profiles. This adds an "Include source code" checkbox to the publish panel, with a file tree to pick which files to include.
I think some of the patches can land before. I'll extract them and land them before this patch.
Example screenshot:
