Change page information structure to match the gecko side #2266
Conversation
canova
force-pushed
the
new-page-info-structure
branch
from
c7bd23b
to
427cac1
Compare
Codecov Report
@@ Coverage Diff @@
## master #2266 +/- ##
==========================================
+ Coverage 86.06% 86.11% +0.04%
==========================================
Files 203 203
Lines 14755 14819 +64
Branches 3704 3711 +7
==========================================
+ Hits 12699 12761 +62
- Misses 1884 1886 +2
Partials 172 172
Continue to review full report at Codecov.
|
canova
force-pushed
the
new-page-info-structure
branch
from
427cac1
to
fcb750a
Compare
There was a problem hiding this comment.
Looks pretty good to me! Can you add a test that exercises the upgrade before landing? I think that's the only thing I see missing. I think you can modify the stored profile fixtures, and a profile snapshot update should be sufficient. It would be good to add the commit first with the new data, then a second commit with the new changes and snapshot change.
| @@ -413,8 +413,7 @@ export type DOMEventMarkerPayload = {| | |||
| interval: 'start' | 'end', | |||
| eventType: string, | |||
| phase: 0 | 1 | 2 | 3, | |||
| docShellId?: string, | |||
| docshellHistoryId?: number, | |||
| innerWindowID?: number, | |||
There was a problem hiding this comment.
Do we have a good place to document this? I'm guessing it'll be repeated across multiple payloads.
There was a problem hiding this comment.
It's probably good enough to just grep and find the Page type.
There was a problem hiding this comment.
Document in gecko side? We have that information in base payload class in gecko: https://searchfox.org/mozilla-central/rev/5cb522c7baba24e55874809e0e206b001494c1e9/tools/profiler/public/ProfilerMarkerPayload.h#144-145 but it's optional and we only pass that info in subset of markers (because we are not able to find that info on others)
canova
force-pushed
the
new-page-info-structure
branch
2 times, most recently
from
741d665
to
c1c7b3b
Compare
canova
force-pushed
the
new-page-info-structure
branch
from
c1c7b3b
to
8fd430e
Compare
| @@ -162,6 +163,13 @@ describe('upgrading processed profiles', function() { | |||
| require('../fixtures/upgrades/processed-2.json') | |||
| ); | |||
| }); | |||
| it('should upgrade processed-3.json all the way to the current version', async function() { | |||
There was a problem hiding this comment.
I had to add a new test json here because processed-2.json was too old and upgrader 8 is removing the docshell ids and docshell history ids from network marker :/ (page information is added after upgrader 8)
|
@gregtatum It's ready for a review again. Changed the json files to test this upgrader. Second commit is the same commit that you reviewed. Added the old page information to the first commit and on the last commit updated the snapshots. |
It changes the the page structure of page information array. And adds two upgraders for gecko/processed profiles. Currently we don't do any special handling for single tab browsing yet.
Gecko sides of this patch haven't landed yet, we should first land this side and then we can land the gecko side.
Gecko bugs:
https://bugzilla.mozilla.org/show_bug.cgi?id=1583271
https://bugzilla.mozilla.org/show_bug.cgi?id=1512500
(I will put some profiles here to test with deploy preview)