Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6261 +/- ##
==========================================
+ Coverage 84.11% 84.15% +0.03%
==========================================
Files 356 357 +1
Lines 38444 38588 +144
Branches 10887 10814 -73
==========================================
+ Hits 32339 32473 +134
- Misses 5676 5686 +10
Partials 429 429 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
281d211 to
900ffff
Compare
|
Thanks for your feedback, @mstange! I've addressed the issues and made some other improvements, please re-review when you have time. |
mstange
left a comment
mstange
left a comment
There was a problem hiding this comment.
Thanks! I dug into the patch in a bit more detail and found more things; it's a big patch and I'm not sure if I've found everything yet, so apologies in advance if this needs another round or two.
One thing that came to mind is whether it makes sense to take care of #4162 at the same time. Rather than sanitizing out all screenshot fields during sanitization, we could add a privacy category for screenshots and annotate the screenshot URL fields with that. Not sure if that would be an improvement but it seems somewhat consistent; it would let us move towards a world where sanitization only needs to look at the privacy categories.
| "windowWidth": 1280, | ||
| "windowHeight": 1000 |
There was a problem hiding this comment.
Hmm, what am I looking at here? Wouldn't we expect a windowSize property here?
There was a problem hiding this comment.
But this is an upgrader input fixture for processed profile version 24. So the two fields are expected. We also have the upgraded output snapshot that verifies their conversion to windowSize. Am I not getting right smth?
The direction makes sense to me too, but I'd prefer to keep #4162 separate to avoid expanding this patch further. |
Combine schemas from all input profiles instead of keeping only the first profile's schemas. Schema-driven screenshots need their field definitions to translate image URL indexes and create screenshot tracks, including when only the second profile contains screenshots.
Add schema formats for image string indexes and window dimensions so marker rendering can describe screenshots without knowing their payload field names. Resolve the image aspect ratio through a sibling size field and keep image data out of text formatting and marker searches.
Store window dimensions as one windowSize value so a schema field can describe them together. Normalize window IDs to strings at the profile boundary so consumers can use one representation regardless of whether Gecko supplies a numeric ID or a hexadecimal string. Apply the same payload shape in Gecko processing, Chrome import, and the processed profile upgrader.
Persist screenshot field definitions with profiles so tooltip rendering, string-index translation, and query output can use the schema instead of separate CompositorScreenshot cases. Identify image URLs by their field format to keep query results compact.
Encode screenshot lifetimes as start and end marker pairs and include the window ID in their names. Generic interval derivation can then match screenshots without its own per-window bookkeeping. Window destruction closes the last screenshot. An unclosed window leaves an incomplete interval extending to the end of the thread.
Select screenshot markers through the timeline-screenshots display location and group tracks by marker name. Read image fields from their schema so tracks do not depend on CompositorScreenshot payload names. Use the same classification during sanitization and comparison, and keep first-screenshot ordering so shared track indexes remain stable.
|
Thanks a lot for your thorough feedback, @mstange! I've addressed the issues now. Also, to make it easier to review I've split the changes further into more commits - hope that helps! |
2 participants
Main | Deploy preview
Screenshot markers had no schema and were handled by custom code in tooltip rendering, string-table lookup, marker derivation, and screenshot track creation.
They now use a marker schema. The
windowWidthandwindowHeightfields collapse intowindowSize: { width, height }, described by the newscreenshot-sizeformat. The newscreenshot-data-urlformat renders a string-table image URL using the aspect ratio of the siblingwindowSizefield. The schema also uses the newtimeline-screenshotsdisplay location to drive screenshot track creation.Screenshot markers are stored as start and end pairs named
CompositorScreenshot <windowID>, allowing generic name-based pairing.CompositorScreenshotWindowDestroyedis converted into the end marker of the last screenshot for its window.Notes:
incomplete. Its time range is unchanged, but its duration displays as unknown in tooltips and the Marker Table, affecting duration sorting.CompositorScreenshotWindowDestroyedrow.computeGlobalTracksstill adds windows in first-screenshot order.Closes #5303
Profile