Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 77f371db authored by Alejandro Nijamkin's avatar Alejandro Nijamkin
Browse files

[flexiglass] Removes SCENE_CONTAINER ENABLED. 

THIS CL INCREASES APK AND ODEX SIZE, WE KNOW, AND IT'S APPROVED BY
SYSTEM HEALTH - PLEASE DO NOT REVERT IT FOR THAT REASON ALONE.

Change-Id: I15bf21f67a1546a38fac84cd35efb7e641020ad6

---

This CL removes SCENE_CONTAINER_ENABLED = false from Flags.kt, a hard-coded boolean that we used, in addition to the flags, to prevent builds from growing too large (the build system strips out code from paths that are unreachable and SCENE_CONTAINER_ENABLED being false makes all of Flexiglass unreachable code).

The removal of the constant boolean is necessary for several reasons:

  1. With it, nobody can turn on Flexiglass in any way without making a local code change; this prevents Teamfood to happen. We're not ready for Teamfood, but we need to get ready for it eventually.
  2. With it, end-to-end tests can't actually work with Flexiglass on; we're already funding an effort to run end-to-end tests with Flexiglass on and fix them or fix the code such that they can still run and hopefully pass. This is also not something we'll need until we enter Staging, but is something that we're anticipating.
  3. With it, development is harder as all devs have to keep a local change that sets SCENE_CONTAINER_ENABLED to true to be able to work and need to be careful to never accidentally include that in a CL.

The removal of SCENE_CONTAINER_ENABLED, therefore increases the odex
size by 2.33mb, which is expected (the build system no longer strips
code that's unreachable when the flag is off because now that code is
reachable). We performed an analysis on the increase and found what we
expected to find: mostly new parts of the Compose library that weren't
being pulled in before and lots of new Compose-based code that we added
in Flexiglass that uses that. While the size increase affects development and dogfood builds (e.g. "start" through "trunkfood full" in
Gantry) it doesn't affect release builds (e.g. "nextfood" in Gantry), at
least not until we roll out to nextfood; for release builds, the current flag value is hard-coded into the build,
causing the build system to again strip out Flexiglass as it's all
unreachable code; this has been confirmed by the ACE team and verified
by jdduke@ in this comment thread: https://googleplex-android-review.git.corp.google.com/c/platform/frameworks/base/+/26638042/comments/b834c68f_4ed62ed3
The System UI team will start working on b/331840768 to help reduce the
size back down (and alleviate future size increases) before we roll it
out to nextfood.

Bug: 283121968
Test: made sure that flexiglass can still be turned on and off using the go/flexiglass-script as described in go/flexiglass-dev-guide. This is an important test to make sure that the removal of the SCENE_CONTAINED_ENABLED constant, which was a part of how the code calculates whether Flexiglass is on or off, still works.
Flag: N/A
Change-Id: Id5c97013abb5ff291bef7f76dffdadf556cf54e6
parent c3d7b636

packages/SystemUI/src/com/android/systemui/flags/FlagDependencies.kt

+1 −1
Original line number Diff line number Diff line
@@ -53,7 +53,7 @@ class FlagDependencies @Inject constructor(featureFlags: FeatureFlagsClassic, ha 


        // SceneContainer dependencies

        SceneContainerFlag.getFlagDependencies().forEach { (alpha, beta) -> alpha dependsOn beta }

        SceneContainerFlag.getMainStaticFlag() dependsOn MIGRATE_KEYGUARD_STATUS_BAR_VIEW

        SceneContainerFlag.getMainAconfigFlag() dependsOn MIGRATE_KEYGUARD_STATUS_BAR_VIEW



        // ComposeLockscreen dependencies

        ComposeLockscreen.token dependsOn KeyguardBottomAreaRefactor.token

packages/SystemUI/src/com/android/systemui/flags/Flags.kt

+0 −8
Original line number Diff line number Diff line
@@ -418,14 +418,6 @@ object Flags { 
    val CLIPBOARD_SHARED_TRANSITIONS =

            unreleasedFlag("clipboard_shared_transitions", teamfood = true)



    /**

     * Whether the scene container (Flexiglass) is enabled. Note that SceneContainerFlags#isEnabled

     * should be checked and toggled together with [SCENE_CONTAINER_ENABLED] so that ProGuard can

     * remove unused code from our APK at compile time.

     */

    // TODO(b/283300105): Tracking Bug

    @JvmField val SCENE_CONTAINER_ENABLED = false



    /**

     * Whether the compose bouncer is enabled. This ensures ProGuard can

     * remove unused code from our APK at compile time.

packages/SystemUI/src/com/android/systemui/scene/shared/flag/SceneContainerFlags.kt

+3 −19
Original line number Diff line number Diff line
@@ -22,7 +22,6 @@ import com.android.systemui.Flags.FLAG_SCENE_CONTAINER 
import com.android.systemui.Flags.sceneContainer

import com.android.systemui.dagger.SysUISingleton

import com.android.systemui.flags.FlagToken

import com.android.systemui.flags.Flags.SCENE_CONTAINER_ENABLED

import com.android.systemui.flags.RefactorFlagUtils

import com.android.systemui.keyguard.KeyguardBottomAreaRefactor

import com.android.systemui.keyguard.KeyguardWmStateRefactor
@@ -40,7 +39,6 @@ object SceneContainerFlag { 
    @JvmStatic

    inline val isEnabled

        get() =

            SCENE_CONTAINER_ENABLED && // mainStaticFlag

            sceneContainer() && // mainAconfigFlag

            KeyguardBottomAreaRefactor.isEnabled &&

                MigrateClocksToBlueprint.isEnabled &&
@@ -49,14 +47,6 @@ object SceneContainerFlag { 
                KeyguardWmStateRefactor.isEnabled

    // NOTE: Changes should also be made in getSecondaryFlags and @EnableSceneContainer



    /**

     * The main static flag, SCENE_CONTAINER_ENABLED. This is an explicit static flag check that

     * helps with downstream optimizations (like unused code stripping) in builds where aconfig

     * flags are still writable. Do not remove!

     */

    inline fun getMainStaticFlag() =

        FlagToken("Flags.SCENE_CONTAINER_ENABLED", SCENE_CONTAINER_ENABLED)



    /** The main aconfig flag. */

    inline fun getMainAconfigFlag() = FlagToken(FLAG_SCENE_CONTAINER, sceneContainer())
@@ -73,19 +63,13 @@ object SceneContainerFlag { 


    /** The full set of requirements for SceneContainer */

    inline fun getAllRequirements(): Sequence<FlagToken> {

        return sequenceOf(getMainStaticFlag(), getMainAconfigFlag()) + getSecondaryFlags()

        return sequenceOf(getMainAconfigFlag()) + getSecondaryFlags()

    }



    /** Return all dependencies of this flag in pairs where [Pair.first] depends on [Pair.second] */

    inline fun getFlagDependencies(): Sequence<Pair<FlagToken, FlagToken>> {

        val mainStaticFlag = getMainStaticFlag()

        val mainAconfigFlag = getMainAconfigFlag()

        return sequence {

            // The static and aconfig flags should be equal; make them co-dependent

            yield(mainAconfigFlag to mainStaticFlag)

            yield(mainStaticFlag to mainAconfigFlag)

            // all other flags depend on the static flag for brevity

        } + getSecondaryFlags().map { mainStaticFlag to it }

        return getSecondaryFlags().map { mainAconfigFlag to it }

    }



    /**

packages/SystemUI/tests/utils/src/com/android/systemui/flags/SceneContainerRule.kt

+2 −34
Original line number Diff line number Diff line
@@ -16,16 +16,14 @@ 


package com.android.systemui.flags



import android.util.Log

import com.android.systemui.scene.shared.flag.SceneContainerFlag

import org.junit.Assert

import org.junit.Assume

import org.junit.rules.TestRule

import org.junit.runner.Description

import org.junit.runners.model.Statement



/**

 * Should always be used with [SetFlagsRule] and should be ordered after it.

 * Should always be used with `SetFlagsRule` and should be ordered after it.

 *

 * Used to ensure tests annotated with [EnableSceneContainer] can actually get `true` from

 * [SceneContainerFlag.isEnabled].
@@ -35,15 +33,10 @@ class SceneContainerRule : TestRule { 
        return object : Statement() {

            @Throws(Throwable::class)

            override fun evaluate() {

                val initialEnabledValue = Flags.SCENE_CONTAINER_ENABLED

                val hasAnnotation =

                    description?.testClass?.getAnnotation(EnableSceneContainer::class.java) !=

                        null || description?.getAnnotation(EnableSceneContainer::class.java) != null

                if (hasAnnotation) {

                    Assume.assumeTrue(

                        "Couldn't set Flags.SCENE_CONTAINER_ENABLED for @EnableSceneContainer test",

                        trySetSceneContainerEnabled(true)

                    )

                    Assert.assertTrue(

                        "SceneContainerFlag.isEnabled is false:" +

                            "\n * Did you forget to add a new aconfig flag dependency in" +
@@ -52,32 +45,7 @@ class SceneContainerRule : TestRule { 
                        SceneContainerFlag.isEnabled

                    )

                }

                try {

                base?.evaluate()

                } finally {

                    if (hasAnnotation) {

                        trySetSceneContainerEnabled(initialEnabledValue)

                    }

                }

            }

        }

    }



    companion object {

        fun trySetSceneContainerEnabled(enabled: Boolean): Boolean {

            if (Flags.SCENE_CONTAINER_ENABLED == enabled) {

                return true

            }

            return try {

                // TODO(b/283300105): remove this reflection setting once the hard-coded

                //  Flags.SCENE_CONTAINER_ENABLED is no longer needed.

                val field = Flags::class.java.getField("SCENE_CONTAINER_ENABLED")

                field.isAccessible = true

                field.set(null, enabled) // note: this does not work with multivalent tests

                true

            } catch (t: Throwable) {

                Log.e("SceneContainerRule", "Unable to set SCENE_CONTAINER_ENABLED=$enabled", t)

                false

            }

        }

    }