Skip to content
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

Pretty print the flow tree to json (Fix #12675) #13092

Closed
wants to merge 1 commit into from

Conversation

@yoyo930021
Copy link
Contributor

yoyo930021 commented Aug 28, 2016


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #12675 (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because is debug use no test

This change is Reviewable

@highfive
Copy link

highfive commented Aug 28, 2016

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @emilio (or someone else) soon.

@highfive
Copy link

highfive commented Aug 28, 2016

warning Warning warning

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

yoyo930021 commented Aug 28, 2016

doesn't make for print_extra_flow_children function ->
self.print_extra_flow_children(print_tree);

        print_tree.new_level(format!("{:?}", self));
        self.print_extra_flow_children(print_tree);
        for kid in child_iter(self) {
            kid.print_with_tree(print_tree);
        }
        print_tree.end_level();

I can't handle this part,
so I want to open issues for it.

@izgzhen izgzhen changed the title Pretty print the flow tree #12675 to json Pretty print the flow tree to son (Fix #12675) Aug 28, 2016
@izgzhen izgzhen changed the title Pretty print the flow tree to son (Fix #12675) Pretty print the flow tree to json (Fix #12675) Aug 28, 2016
@izgzhen
Copy link
Contributor

izgzhen commented Aug 28, 2016

Hmm ... the issue you are trying to solve doesn't look easy to me actually. And I suppose that we can try to add another function emitting JSON object while retaining the old print function. But I am not very sure about the plan. @shinglyu idea?

@shinglyu
Copy link
Member

shinglyu commented Aug 30, 2016

This looks OK, but in order not to surprise people who uses this feature, we should wait until we have another tool that can print in the old format. (Maybe a python script that takes the JSON and print as a tree) Then we can merge all the patches at once.

Also I forgot to mention, we should check if bit of information in the old format is in the new JSON format, if something is missing, we should definitely add that to the JSON output.

@shinglyu
Copy link
Member

shinglyu commented Aug 30, 2016

Or we could create a new option in components/util/opts.rs called dump-flow-tree-json, and don't touch the original dump-flow-tree, this will keep the backward compatibility so we can experiment more freely.

@yoyo930021
Copy link
Contributor Author

yoyo930021 commented Aug 30, 2016

@shinglyu
I try!

@bors-servo
Copy link
Contributor

bors-servo commented Sep 27, 2016

The latest upstream changes (presumably #13346) made this pull request unmergeable. Please resolve the merge conflicts.

@nox
Copy link
Member

nox commented Nov 7, 2016

@yoyo930021 Any news on this?

@yoyo930021
Copy link
Contributor Author

yoyo930021 commented Nov 7, 2016

Sorry , no time to do it.
And I meet a few problem.

@shinglyu
Copy link
Member

shinglyu commented Nov 9, 2016

This feature can now use the JOSN serialization from #13740. If someone is interested I can point them to the related code.

@wafflespeanut
Copy link
Member

wafflespeanut commented Dec 28, 2016

Closing based on inactivity :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

9 participants
You can’t perform that action at this time.