Skip to content

/ servo

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Print WebRender display lists #19826

Closed
wants to merge 1 commit into from
Closed

[WIP] Print WebRender display lists #19826

wants to merge 1 commit into from
+27 −33

Conversation

@pyfisch
Copy link
Contributor

pyfisch commented Jan 21, 2018 •
edited by SimonSapin

The debug options

  • dump-display-list
  • dump-display-list-json

now print WebRender display lists instead of Servo DLs.

This change is Reviewable
@highfive
Copy link

highfive commented Jan 21, 2018

Heads up! This PR modifies the following files:

  • @emilio: components/layout/display_list/builder.rs
@highfive
Copy link

highfive commented Jan 21, 2018

warning Warning warning

  • These commits modify layout code, but no tests are modified. Please consider adding a test!
@pyfisch
Copy link
Contributor Author

pyfisch commented Jan 21, 2018

See #19676

Blocked by: servo/webrender#2328

cc @mrobinson @emilio

The main differences between the two formats are:

  • While GFX display lists contain TextDisplayItem which contain text runs the webrender display lists only contain glyphs. This makes text unreadable and consumes more space but also makes the information accessible where exactly glyphs are positioned.
  • GFX display lists are printed as a tree and while the WebRender lists also form a tree they are printed as a flat list.
  • WebRender display lists are generally more verbose but I guess this can be avoided in part by not serializing default values.
@mrobinson
mrobinson requested changes Jan 23, 2018
View changes
Copy link
Member

mrobinson left a comment

I'm happy to see us continuing to move along this path.
components/layout/display_list/builder.rs Outdated
color: background_color.to_layout(),
})));

// Skip transparent backgrounds.

This comment has been minimized.

Sign in to view
@mrobinson

mrobinson Jan 23, 2018

Member

We need to include transparent backgrounds to properly support hit testing.

This comment has been minimized.

Sign in to view
@pyfisch

pyfisch Jan 24, 2018

Author Contributor

Did not know that.
components/layout_thread/lib.rs Outdated
debug!("Layout done!");

// TODO: Avoid the temporary conversion and build webrender sc/dl directly!
let wr_dl = rw_data.display_list.as_ref().unwrap().convert_to_webrender(self.id).finalize();

This comment has been minimized.

Sign in to view
@mrobinson

mrobinson Jan 23, 2018

Member

I think it's be better to avoid obscure abbreviations here. Bytes are practically free! You can just write webrender_display_list.

This comment has been minimized.

Sign in to view
@pyfisch

pyfisch Jan 24, 2018

Author Contributor

Bytes happen to be practically free but my screen can still only display 100 character in a line. 😛

This comment has been minimized.

Sign in to view
@mrobinson

mrobinson Jan 24, 2018

Member

It's true that long names can lead to having to insert more new lines, but descriptive names make it a lot easier for people who are digesting the code for the first time.
components/layout_thread/lib.rs Outdated
if opts::get().dump_display_list {
display_list.print();
println!("// Display List from {:?}\n{}",

This comment has been minimized.

Sign in to view
@mrobinson

mrobinson Jan 23, 2018

Member

Looking at this now, I'm wondering if it's better to make WebRender responsible for printing the display list? Or is WebRender already doing a lot of the work here?

With this change, can't we remove the printing methods from the DisplayList struct as well?

This comment has been minimized.

Sign in to view
@pyfisch

pyfisch Jan 24, 2018

Author Contributor

Looking at this now, I'm wondering if it's better to make WebRender responsible for printing the display list? Or is WebRender already doing a lot of the work here?

I am not sure if I understand these questions. WebRender has the types and visitors for serialization. The RON crate formats it to actual text and servo just prints it. The comment heading Display List from {:?}is just because I found it somewhat difficult to see where one display list ends and another starts.

With this change, can't we remove the printing methods from the DisplayList struct as well?
Yes we can. This was the WIP part of the PR.
@pyfisch pyfisch force-pushed the pyfisch:dl-print branch from 6e202cd to 6309d69 Jan 24, 2018
@pyfisch
Copy link
Contributor Author

pyfisch commented Jan 24, 2018

Updated. Not sure if I missed some printing stuff.

I am not entirely sure how the display lists in servo will look after the migration of WebRender is complete but probably some structure is needed to represent the hierarchy of StackingContexts and to order the display items. This could be printed instead of the WebRender display lists. (And probably formatted as tree like today)
@pyfisch
Print WebRender display lists
Loading status checks…
29e024c 
The debug options

* dump-display-list
* dump-display-list-json

now print WebRender display lists instead of Servo DLs.
@pyfisch pyfisch force-pushed the pyfisch:dl-print branch from 6309d69 to 29e024c Jan 25, 2018
@pyfisch
Copy link
Contributor Author

pyfisch commented Jan 25, 2018

Finally the name is correct.

Some example output: https://gist.github.com/pyfisch/bc859bc53688aa0bc0db6726f9f61205
@mrobinson
Copy link
Member

mrobinson commented Jan 29, 2018

The ron output doesn't look super useful for debugging display list issues... What is the best way to make the WebRender display list as readable as the previous output?
@pyfisch
Copy link
Contributor Author

pyfisch commented Jan 29, 2018

I am not sure. Probably the WebRender display lists should also be printed before flattening occurs. (And text spans should be printed and not resolved to glyphs.)
@pyfisch pyfisch closed this Jan 29, 2018
@mrobinson
Copy link
Member

mrobinson commented Jan 29, 2018

@pyfisch Sorry! I didn't mean to imply that you needed to close this bug. I was just curious if there was a plan to go from the serialization-type output to one oriented toward debugging issues.
@pyfisch
Copy link
Contributor Author

pyfisch commented Jan 29, 2018

I now think that unless WebRender gets some really good serialization for debugging itself it is best to stick with servos own serialization logic and make it work well with the types from WebRender as items.
@mrobinson
Copy link
Member

mrobinson commented Jan 29, 2018

@pyfisch Okay. I'm sorry if I wasted your time in this review. It wasn't my intention at all.
@pyfisch pyfisch deleted the pyfisch:dl-print branch May 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrobinson mrobinson

Assignees

@emilio emilio

Labels
S-awaiting-review
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
@pyfisch @highfive @mrobinson @emilio
You can’t perform that action at this time.