Skip to content

Origins based view for Fission#2513

Merged
gregtatum merged 3 commits into
firefox-devtools:masterfrom
gregtatum:origins
May 5, 2020
Merged

Origins based view for Fission#2513
gregtatum merged 3 commits into
firefox-devtools:masterfrom
gregtatum:origins

Conversation

@gregtatum

@gregtatum gregtatum commented Apr 21, 2020

Copy link
Copy Markdown
Member

This is now ready for review. I've split up everything into 3 commits which should be best viewed individually.

The first commit is the most verbose, as it changes the showTabOnly architecture. It introduces the TimelineTrackOrganization, which can then use switches to handle the different cases.

/**
 * Determines how the timeline's tracks are organized.
 */
export type TimelineTrackOrganization =
  | {| +type: 'full' |}
  | {| +type: 'active-tab', +browsingContextID: BrowsingContextID |}
  | {| +type: 'origins' |};

One question I have, is that do we actually need to serialize the browsingContextID to the URL? Can we just look it up by the profile.meta.configuration.activeBrowsingContextID?

The second commit actually adds all of the experimentation. I don't think this commit needs a "strong review", as its experimental. I tried to add known issues as code comments. Your welcome to do a thorough review there, or a light one, as I think it falls under "low risk" code changes. I tried to comment things as much as I could, let me know if I can clarify more. My plan is to iterate a bit with the code here, and focus on follow-ups if possible. Commit #1 and #3 are intended as non-experimental commits, and should probably have due rigor applied on the review.

Deploy preview

In order to activate this view, add &view=origins to the URL.

@gregtatum gregtatum requested a review from canova April 29, 2020 19:07
@gregtatum gregtatum changed the title [wip] Origins based view for Fission Origins based view for Fission Apr 29, 2020
@codecov

codecov Bot commented Apr 29, 2020

Copy link
Copy Markdown

Codecov Report

Merging #2513 into master will decrease coverage by 0.11%.
The diff coverage is 64.80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2513      +/-   ##
==========================================
- Coverage   86.39%   86.27%   -0.12%     
==========================================
  Files         214      214              
  Lines       16687    16801     +114     
  Branches     4283     4315      +32     
==========================================
+ Hits        14416    14495      +79     
- Misses       2068     2107      +39     
+ Partials      203      199       -4     
Impacted Files Coverage Δ
src/components/timeline/FullTimeline.js 62.85% <0.00%> (-3.81%) ⬇️
src/components/timeline/OriginsTimeline.js 0.00% <0.00%> (ø)
src/selectors/app.js 59.78% <42.18%> (+5.15%) ⬆️
src/app-logic/url-handling.js 81.21% <55.55%> (-4.93%) ⬇️
src/components/timeline/index.js 66.66% <60.00%> (-33.34%) ⬇️
src/actions/receive-profile.js 83.33% <82.72%> (-0.46%) ⬇️
src/test/fixtures/profiles/tracks.js 93.80% <89.28%> (-1.49%) ⬇️
src/selectors/profile.js 89.49% <90.47%> (-0.85%) ⬇️
src/components/timeline/TrackContextMenu.js 86.52% <100.00%> (ø)
src/reducers/app.js 98.37% <100.00%> (+0.01%) ⬆️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6bc120...f71e075. Read the comment docs.

@canova canova left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the work! It looks good to me and like the track architecture, we can use something like that for active tab too, if we can also support other track types like screenshots.
Probably it could use some code de-duplication with the active tab in the future :) But nothing to block for this PR!

Also for the question:

One question I have, is that do we actually need to serialize the browsingContextID to the URL? Can we just look it up by the profile.meta.configuration.activeBrowsingContextID?

Yeah, we can do that. I think we went with adding this ID to the URL because when we were talking about possibilities, someone mentioned that we could also want to support other (non-active) tabs in the future, like selecting a tab from a list and seeing its content instead of the active tab. So adding the active tab ID there is more future proof. But we can also handle this with an upgrader if we want that option in the future. So I'm okay with removing it from the URL.

Comment thread src/actions/receive-profile.js
Comment thread src/components/timeline/FullTimeline.js Outdated
Comment thread src/actions/receive-profile.js
Comment thread src/components/timeline/OriginsTimeline.js
Comment thread src/components/timeline/OriginsTimeline.js
Comment thread src/types/profile-derived.js
@gregtatum gregtatum merged commit bc1ed76 into firefox-devtools:master May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants