Skip to content

Comments

Network track for the timeline#1189

Merged
gregtatum merged 6 commits intofirefox-devtools:masterfrom
gregtatum:network-track
Aug 29, 2018
Merged

Network track for the timeline#1189
gregtatum merged 6 commits intofirefox-devtools:masterfrom
gregtatum:network-track

Conversation

@gregtatum
Copy link
Member

@gregtatum gregtatum commented Aug 9, 2018

Deploy preview

I'm pushing this up at the end of the day, feel free to look at the deploy preview, but I plan on reading through it all again with some fresh eyes tomorrow before asking for review.

Edit: This is now ready for review, @ola, would you feel comfortable reviewing this? I have split the work into multiple logical commits, so it will be easiest to look at each in turn, rather than the whole at once. I still haven't reproduced the first-paint effect Markus describes below.

Also resolves #1196

@mstange
Copy link
Contributor

mstange commented Aug 9, 2018

There seems to be an odd first-paint effect where the whole canvas is filled with requests:
screen shot 2018-08-09 at 6 47 24 pm
screen shot 2018-08-09 at 6 47 33 pm

@mstange
Copy link
Contributor

mstange commented Aug 9, 2018

Wheel scrolling doesn't seem to work while the mouse is over the network track.

@gregtatum
Copy link
Member Author

There seems to be an odd first-paint effect where the whole canvas is filled with requests

@mstange: I can't reproduce this. I tried recording my screen, and then loading both from the add-on, viewing existing profiles, and committing new ranges.

@codecov
Copy link

codecov bot commented Aug 10, 2018

Codecov Report

Merging #1189 into master will increase coverage by 0.15%.
The diff coverage is 85.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1189      +/-   ##
==========================================
+ Coverage    75.9%   76.06%   +0.15%     
==========================================
  Files         144      145       +1     
  Lines        9364     9501     +137     
  Branches     2319     2350      +31     
==========================================
+ Hits         7108     7227     +119     
- Misses       2013     2031      +18     
  Partials      243      243
Impacted Files Coverage Δ
src/types/actions.js 0% <ø> (ø) ⬆️
src/test/fixtures/mocks/canvas-context.js 94.73% <ø> (ø) ⬆️
src/reducers/app.js 96.42% <100%> (+0.59%) ⬆️
src/components/timeline/GlobalTrack.js 80% <100%> (+0.28%) ⬆️
src/reducers/url-state.js 96.74% <100%> (+0.02%) ⬆️
src/profile-logic/tracks.js 94.08% <100%> (+1.01%) ⬆️
src/test/fixtures/profiles/make-profile.js 96.64% <100%> (+0.11%) ⬆️
src/actions/profile-view.js 89.87% <71.05%> (-3.43%) ⬇️
src/components/timeline/TrackNetwork.js 83.01% <83.01%> (ø)
src/reducers/profile-view.js 94.7% <88.23%> (-0.26%) ⬇️
... and 4 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 0b84472...fc86aa1. Read the comment docs.

expect(getRightClickedTrack(getState())).not.toEqual(trackReference);
expect(getSelectedThreadIndex(getState())).not.toBe(threadIndex);
expect(getLocalTrackRow().hasClass('selected')).toBe(false);
describe('with a thread track', function() {
Copy link
Member Author

Choose a reason for hiding this comment

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

I ended up taking Nicolas Chevobbe's review advice here and re-organized these test files, as I was feeling lost while working on the tests. The setup functions are located at the bottom of the page.

I would recommend turning on "hide whitespace changes" in the Diff Settings on this review, as there is an indentation change.

expect(view.find('.timelineTrack').exists()).toBe(false);
});
});

Copy link
Member Author

Choose a reason for hiding this comment

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

The following are the new tests:

@gregtatum
Copy link
Member Author

Wheel scrolling doesn't seem to work while the mouse is over the network track.

This is fixed in the latest code.

Copy link
Contributor

@zoepage zoepage left a comment

Choose a reason for hiding this comment

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

This PR looks really good. :)

Besides the one question above, I realized that the markers in the network panel get updated when selecting a range, but the list is not updated (see screenshot, checked with following profile)

screen shot 2018-08-17 at 12 23 28

When you select a range for the marker-table, the markers as well as the list get updated.

========

When selecting just the network thread, the shadow makes it a bit hard so see the network markers in the thread. It also feels dense.

screen shot 2018-08-17 at 12 18 40

========

How would you feel about adding one more row or some additional padding at the bottom of the network thread (so increase the min-height), as the thread seems to get "swallowed" when it does not contain markers?

screen shot 2018-08-17 at 12 18 19

type: 'Network',
id,
pri: 0,
status: 'DONE',
Copy link
Contributor

Choose a reason for hiding this comment

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

The status DONE got me confused as I thought we just have START and STOP, so I thought I rather check in if there is something I am missing? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

It was just mock data, I didn't remember what the strings were off the top of my head, since they aren't in our types. I'll change it to STOP to be consistent and not confusing.

@gregtatum
Copy link
Member Author

When selecting just the network thread, the shadow makes it a bit hard so see the network markers in the thread. It also feels dense.

The shadow is created by the OverflowEdgeIndicator component. It should only show up when the timeline can scroll down, which this is not doing in your screenshots. I narrowed it down to an STR. I think we should fix this in a follow-up, I filed that as #1205.

Besides the one question above, I realized that the markers in the network panel get updated when selecting a range, but the list is not updated (see screenshot, checked with following profile)

This is #865.

How would you feel about adding one more row or some additional padding at the bottom of the network thread (so increase the min-height), as the thread seems to get "swallowed" when it does not contain markers?

Yeah, I can throw another row in there for that case. I feel like the network track being empty is a bit weird, although I think having it disappear might be confusing. Perhaps having it hide itself is a better option here?

@gregtatum
Copy link
Member Author

@zoepage I added 11e9b7e to address your review comments, and filed the follow-up #1205 to deal with the shadow issue. This is ready for your re-review now.

@gregtatum
Copy link
Member Author

I have a PR #1209 for addressing the shadow issue #1205.

@zoepage
Copy link
Contributor

zoepage commented Aug 28, 2018

Yeah, I can throw another row in there for that case. I feel like the network track being empty is a bit weird, although I think having it disappear might be confusing. Perhaps having it hide itself is a better option here?

+1 in hiding itself when it is empty. Do you want to file a follow up for this?

In case we have just very few markers and the row is still min-height, we can re-check again when it landed and file a follow up bug, if this still feels like it gets "swallowed".

Thank you for the changes :)

@gregtatum gregtatum merged commit 6e5306b into firefox-devtools:master Aug 29, 2018
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.

Can't scroll the header while the mouse is over a process track with no main thread

3 participants