Make the display list dumping code more generic. #2893

staktrace · 2018-07-13T18:17:40Z

This allows dumping a subset of items in the display list, at specified
indentation.

This change is

staktrace · 2018-07-13T18:17:50Z

This is desirable for https://bugzilla.mozilla.org/show_bug.cgi?id=1475637

kvark · 2018-07-13T19:25:11Z

webrender_api/src/display_list.rs

        {
            let mut iter = BuiltDisplayListIter::new(&temp);
            while let Some(item) = iter.next_raw() {
-                println!("{:?}", item.display_item());
+                if index >= start.unwrap_or(0) && index < end.unwrap_or(u32::MAX) {


nit: end.map_or(true, |e| index < e) would avoid the need to include u32

kvark · 2018-07-13T19:25:57Z

webrender_api/src/display_list.rs

+        indent: usize,
+        start: Option<u32>,
+        end: Option<u32>,
+    ) -> u32 {


any reason the start/end/result are u32 and not usize? Typically, seeing a sized type implies some interop/size constraints, which I don't see here.

kvark · 2018-07-13T19:26:20Z

webrender_api/src/display_list.rs

@@ -915,18 +915,36 @@ impl DisplayListBuilder {
        self.save_state.take().expect("No save to clear in DisplayListBuilder");
    }

-    pub fn print_display_list(&mut self) {
+    /// Print the display items in the list to stderr. If the start parameter


Should this really be in the stderr?

stdout is buffered often, so things written to stdout don't always show up if you crash right after, for example. This is one of the reasons why pretty much all of gecko logging goes to stderr, and if this goes to stdout it won't be interleaved properly with the rest of the output. Although maybe this should return a string rather than dumping it directly and the caller can decide where to put it.

(The rest of your comments make sense, will update the patch accordingly)

FYI, my experience so far indicates that rust's println! flushes, but print! does not.

Even if that's the case, these prints are going to be interleaved between prints from C++ code, so they should probably go to the same place.

kvark · 2018-07-13T19:26:34Z

webrender_api/src/display_list.rs

+                if index >= start.unwrap_or(0) && index < end.unwrap_or(u32::MAX) {
+                    eprintln!("{}{:?}", "  ".repeat(indent), item.display_item());
+                }
+                index = index + 1;


nit: index += 1;

This allows dumping a subset of items in the display list, at specified indentation.

staktrace · 2018-07-13T20:31:56Z

Updated patch to address comments, but left it going to stderr. If you prefer I can split this into two methods: one that does what the old one did (print the whole thing to stdout), and one that takes the new arguments but returns the string instead of printing it. The caller can then print it to stderr or stdout as desired.

edit: typo

gw3583 · 2018-07-16T21:21:37Z

@kvark is this ready to merge?

kvark · 2018-07-17T16:17:59Z

@bors-servo r+

bors-servo · 2018-07-17T16:18:00Z

📌 Commit 15a2efd has been approved by kvark

bors-servo · 2018-07-17T16:18:04Z

⌛ Testing commit 15a2efd with merge 88dab3f...

Make the display list dumping code more generic. This allows dumping a subset of items in the display list, at specified indentation.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2893)

bors-servo · 2018-07-17T17:37:57Z

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing 88dab3f to master...

… string. r?kvark For proper android support we need to dump this to logcat instead of stderr, so it seems better to just have a function to return the dumped string and we can send it wherever we want in the embedding application. This also changes the print_display_list back to the original semantics (prior to servo/webrender#2893) where it just dumps the whole display list to stdout. Differential Revision: https://phabricator.services.mozilla.com/D11195

@kvark

For proper android support we need to dump this to logcat instead of stderr, so it seems better to just have a function to return the dumped string and we can send it wherever we want in the embedding application. This also changes the print_display_list back to the original semantics (prior to servo#2893) where it just dumps the whole display list to stdout. Reviewed by @kvark at https://phabricator.services.mozilla.com/D11195 Related bug in bugzilla: https://bugzil.la/1505375

@kvark

For proper android support we need to dump this to logcat instead of stderr, so it seems better to just have a function to return the dumped string and we can send it wherever we want in the embedding application. This also changes the print_display_list back to the original semantics (prior to servo#2893) where it just dumps the whole display list to stdout. Reviewed by @kvark at https://phabricator.services.mozilla.com/D11195 Related bug in bugzilla: https://bugzil.la/1505375

@kvark

For proper android support we need to dump this to logcat instead of stderr, so it seems better to just have a function to return the dumped string and we can send it wherever we want in the embedding application. This also changes the print_display_list back to the original semantics (prior to servo#2893) where it just dumps the whole display list to stdout. Reviewed by @kvark at https://phabricator.services.mozilla.com/D11195 Related bug in bugzilla: https://bugzil.la/1505375

@kvark

Extract a method to return the dumped display list as a string. For proper android support we need to dump this to logcat instead of stderr, so it seems better to just have a function to return the dumped string and we can send it wherever we want in the embedding application. This also changes the print_display_list back to the original semantics (prior to #2893) where it just dumps the whole display list to stdout. Reviewed by @kvark at https://phabricator.services.mozilla.com/D11195 Related bug in bugzilla: https://bugzil.la/1505375  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/3286)

@kvark

Extract a method to return the dumped display list as a string. For proper android support we need to dump this to logcat instead of stderr, so it seems better to just have a function to return the dumped string and we can send it wherever we want in the embedding application. This also changes the print_display_list back to the original semantics (prior to #2893) where it just dumps the whole display list to stdout. Reviewed by @kvark at https://phabricator.services.mozilla.com/D11195 Related bug in bugzilla: https://bugzil.la/1505375  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/3286)

staktrace force-pushed the printing branch from 990bee0 to b31c158 Compare July 13, 2018 18:24

kvark reviewed Jul 13, 2018

View reviewed changes

Make the display list dumping code more generic.

15a2efd

This allows dumping a subset of items in the display list, at specified indentation.

staktrace force-pushed the printing branch from b31c158 to 15a2efd Compare July 13, 2018 20:30

bors-servo merged commit 15a2efd into servo:master Jul 17, 2018

staktrace mentioned this pull request Nov 8, 2018

Extract a method to return the dumped display list as a string. #3286

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the display list dumping code more generic. #2893

Make the display list dumping code more generic. #2893

staktrace commented Jul 13, 2018 •

edited by larsbergstrom

staktrace commented Jul 13, 2018

kvark Jul 13, 2018

kvark Jul 13, 2018

kvark Jul 13, 2018

staktrace Jul 13, 2018

kvark Jul 13, 2018

staktrace Jul 13, 2018

kvark Jul 13, 2018

staktrace commented Jul 13, 2018 •

edited

gw3583 commented Jul 16, 2018

kvark commented Jul 17, 2018

bors-servo commented Jul 17, 2018

bors-servo commented Jul 17, 2018

bors-servo commented Jul 17, 2018

Make the display list dumping code more generic. #2893

Make the display list dumping code more generic. #2893

Conversation

staktrace commented Jul 13, 2018 • edited by larsbergstrom

staktrace commented Jul 13, 2018

kvark Jul 13, 2018

Choose a reason for hiding this comment

kvark Jul 13, 2018

Choose a reason for hiding this comment

kvark Jul 13, 2018

Choose a reason for hiding this comment

staktrace Jul 13, 2018

Choose a reason for hiding this comment

kvark Jul 13, 2018

Choose a reason for hiding this comment

staktrace Jul 13, 2018

Choose a reason for hiding this comment

kvark Jul 13, 2018

Choose a reason for hiding this comment

staktrace commented Jul 13, 2018 • edited

gw3583 commented Jul 16, 2018

kvark commented Jul 17, 2018

bors-servo commented Jul 17, 2018

bors-servo commented Jul 17, 2018

bors-servo commented Jul 17, 2018

staktrace commented Jul 13, 2018 •

edited by larsbergstrom

staktrace commented Jul 13, 2018 •

edited