[flutter_tool] Support disabling Impeller #122960
Conversation
flutter-dashboard
bot
added
the
tool
zanderso
force-pushed
the
disable-impeller-flag
branch
2 times, most recently
from
2fa410d
to
67da160
Compare
| @@ -1128,7 +1132,7 @@ class DebuggingOptions { | |||
| if (purgePersistentCache) '--purge-persistent-cache', | |||
| if (route != null) '--route=$route', | |||
| if (platformArgs['trace-startup'] as bool? ?? false) '--trace-startup', | |||
| if (enableImpeller) '--enable-impeller', | |||
There was a problem hiding this comment.
don't we still need to pass --enable-impeller sometimes? I'm having a bit of a hard time wrapping my head around two independent flags 🙃
There was a problem hiding this comment.
This function creates the flags for iOS and iOS simulator. The --enable-impeller(=true) flag is always a no-op there after the engine flag flip. It can't override the Info.plist setting. When there is no Info.plist setting either way, the engine flag --enable-impeller=false will disable Impeller on iOS and iOS sim.
After this change, when --enable-impeller is passed on Android, --enable-impeller will still be passed to the engine.
There was a problem hiding this comment.
But we're not changing the info.plist value, just the default if not specified right? Can we update --no-enable-impeller to pass --enable-impeller=false?
There was a problem hiding this comment.
What I was having trouble figuring out was how to handle the case where the --enable-impeller flag is absent on the command line. The default value that we'd like the flag to have differs depending on the target device, so DebuggingOptions would need to know both the value of the flag, and whether the flag was present or absent on the command line. Do you have thoughts on whether that's preferable to the --disable-impeller flag?
There was a problem hiding this comment.
I don't think debugging options actually needs to know, the only difference for the tooling is what shaders are generated and we've so far worked around that by just generating both variants
There was a problem hiding this comment.
We could instead update this logic to always generate both variants for iOS and call it good?
There was a problem hiding this comment.
There was a problem hiding this comment.
Switched to an enum as discussed offline. PTAL.
zanderso
force-pushed
the
disable-impeller-flag
branch
from
67da160
to
a4ebb6d
Compare
zanderso
changed the title
zanderso
force-pushed
the
disable-impeller-flag
branch
2 times, most recently
from
97185a2
to
89cade1
Compare
zanderso
force-pushed
the
disable-impeller-flag
branch
from
89cade1
to
99f49b2
Compare
Since Impeller will be default-on on some platforms but not others, this PR allows explicitly enabling, disabling, or using the platform default when
--enable-impeller,--no-enable-impeller, or no flag is passed, respectively.