[CP] Cached DisplayList opacity inheritance fix (#39690)

[zanderso]

This is the CP PR for flutter/flutter#120985

[zanderso]

@flar Could you suggest a fix for the build failure in the presubmits? Does this CP depend on all or part of a previous change that we'd also need to CP?

[chinmaygarde]

The failure does look real. Not sure why it wasn't caught earlier.

[itsjustkevin]

@zanderso any update on the failures here?

[flar]

This getter should have always been const. It was originally written without "const" because no code that called it needed it to work on a const value.

A couple of months ago, during the DisplayList RTree work, this oversight was noted and corrected and the getter was made const. And this fix was written against that change where this getter was already const.

The RTree code went in long enough ago (2 months) that I would have thought it would be included in the latest release candidates (?), where did the branch for this CP originate from?

We can either mark that getter "const" as part of this CP, or if this CP is to an older release we can decide to only CP this fix to more recent releases and candidates and let the bug stand in the older releases?

Simply adding the const modifier to the getter in question is pretty trivial though, even though it might potentially block merging a more recent fix into this release source base. I'm not sure we plan to back-port the DisplayList RTree work that originated the change, though, so that should be safe...

[chinmaygarde]

I suppose that makes this unsafe for a cherry-pick.

[flar]

I suppose that makes this unsafe for a cherry-pick.

Perhaps for the release targeted here that didn't include the fix that made it const.

But, it's also only a one line change to make the getter const and then this CP would probably succeed...

[zanderso]

@flar @chinmaygarde if you can link me to the spot where the const modifier needs to be added, I will update this PR, and we can see what the presubs think of that.

[flar]

@flar @chinmaygarde if you can link me to the spot where the const modifier needs to be added, I will update this PR, and we can see what the presubs think of that.

From the build logs:

../../flutter/display_list/display_list.h:272:8: note: 'can_apply_group_opacity' declared here
  bool can_apply_group_opacity() { return can_apply_group_opacity_; }

[zanderso]

After adding the const qualifier, I'm not hitting this:

../../flutter/flow/layers/display_list_layer_unittests.cc -o obj/flutter/flow/layers/flow_unittests.display_list_layer_unittests.o
../../flutter/flow/layers/display_list_layer_unittests.cc:532:24: error: no matching constructor for initialization of 'DisplayListBuilder'
    DisplayListBuilder builder(false);
                       ^       ~~~~~
../../flutter/display_list/display_list_builder.h:27:7: note: candidate constructor (the implicit copy constructor) not viable: no known conversion from 'bool' to 'const flutter::DisplayListBuilder' for 1st argument
class DisplayListBuilder final : public virtual Dispatcher,
      ^
../../flutter/display_list/display_list_builder.h:34:12: note: candidate constructor not viable: no known conversion from 'bool' to 'const SkRect' for 1st argument
  explicit DisplayListBuilder(const SkRect& cull_rect = kMaxCullRect);
           ^
1 error generated.

Is there another PR that should be included in this CP?

[flar]

Ugh, that change in the constructors was brought in by #34365 3 months ago which was a major restructuring of the Builder to compute bounds during construction rather than as a lazy pass. That whole PR would have to be combined.

It may be possible to simply remove the argument from that constructor. I think DisplayListBuilder foo; used to be an allowed way to construct one before we added rtree arguments to the constructor.

Is it worth back-porting this fix to 3.7 candidate 1? Is there a more recent candidate to target for CP that includes these other fixes?

Update - indeed there are many other declarations of DisplayListBuilder in that unit test file that use the default construction. I believe the default construction is identical to using the (false) constructor in all cases because the default for that parameter is false in all constructors (including one that defaults all values that should get used for a bare declaration).

[flar]

Fingers crossed emoji...

[flar]

LGTM

[zanderso]

@godofredoc is it expected that the Mac mac_ios_engine_profile and Mac mac_ios_engine_release presubmit checks would be queueing for over an hour?

[zanderso]

The 2 queued builds were removed and will never run. @itsjustkevin they can be ignored for the purposes of landing this CP. /cc @godofredoc

[itsjustkevin]

Failing tests are no longer part of presubmits, merging PR.