Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@chinmaygarde
Copy link
Member

No functional change. Makes the display list subsystem easier to navigate as the
major classes are in their own TUs. Also avoids importing unnecessary headers
when the previous kitchen sink header was imported. I've tried to remove all
display list related imports and start from scratch but I may have missed some
files. Minor structs and classes (like the ones in utils, ops, etc..) still
don't get their own TUs though.

There were two related changes being made to this subsystem that have already
landed. So I don't think I am stepping on anyones toes with the reorganization.
Happy to incorporate any work-in-progress changes being made to the this
subsystem before submitting.

@chinmaygarde
Copy link
Member Author

@flar I think I have incorporated the WIP changes you warned me about. Let me know if I missed any. The merges are easy since the I just moved some hunks here.

No functional change. Makes the display list subsystem easier to navigate as the
major classes are in their own TUs. Also avoids importing unnecessary headers
when the previous kitchen sink header was imported. I've tried to remove all
display list related imports and start from scratch but I may have missed some
files. Minor structs and classes (like the ones in utils, ops, etc..) still
don't get their own TUs though.

There were [two](flutter#29562) [related](flutter#30484) changes being made to this subsystem that have since
landed. So I don't think I am stepping on anyones toes with the reorganization.
Happy to incorporate any work-in-progress changes being made to the this
subsystem before submitting.
@chinmaygarde chinmaygarde merged commit 41869f6 into flutter:main Dec 28, 2021
@chinmaygarde chinmaygarde deleted the reorg_dl branch December 28, 2021 23:19

namespace flutter {

// Just exists to ensure that the header can be cleanly imported.
Copy link
Member

Choose a reason for hiding this comment

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

b/212527291 <- looks like the internal compiler caught a problem w.r.t. this. Does display_list_ops.h need to include display_list_dispatcher.h?

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 internal compiler isn't supposed to compile that TU as display_list_ops.cc isn't in the GN definition. But, it should be as the point of that file is to make sure it imports cleanly (as the comment indicates). I guess the import process just copies the files and compiles them all. I'm going to mark that as wont-fix but also submit a patch to the GN rules that compiles this TU (and fix associated issues). That may fix that specific issue with the internal compiler. If there are other issues with the import process itself, I'll redirect them to Flutter@Google.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants