Skip to content

Remove obsolete drawShadow bounds workaround#127052

Merged
auto-submit[bot] merged 2 commits intoflutter:masterfrom
flar:remove-drawshadow-workaround
May 18, 2023
Merged

Remove obsolete drawShadow bounds workaround#127052
auto-submit[bot] merged 2 commits intoflutter:masterfrom
flar:remove-drawshadow-workaround

Conversation

@flar
Copy link
Contributor

@flar flar commented May 17, 2023 •
edited
Loading

This workaround was created 6 years ago with no links to bug databases to track. As best we can determine, the issue is no longer present in SkPicture or DisplayList and is most likely obsolete. More importantly, though, non-rendering primitives are ignored by the DisplayList construction and so this workaround will just be ignored anyway. If a problem surfaces about this issue that we haven't discovered by a thorough code search of the current code base, then workarounds should be installed within the relevant implementation modules rather than in the framework (and documented with bugs filed in either or both of Flutter's github repos, and/or the Skia issue database).

Workaround originally created in this PR: #9654

@flutter-dashboard flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label May 17, 2023
@flar flar requested a review from jason-simmons May 17, 2023 20:12
@flar
Copy link
Contributor Author

flar commented May 17, 2023

No tests are included here because:

  • we aren't sure exactly how the original problem presented itself
  • the likely suspect for problems would be to have the bounds misrepresented during raster caching, but tests that involve raster caching are fragile when executed at the framework level because the parameters of when it happens are not visible to the framework level
  • there are many tests of the bounds of the drawShadow implementation in the engine repo

@flutter-dashboard
Copy link

flutter-dashboard bot commented May 17, 2023

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #127052 at sha 8965f0e

@flutter-dashboard flutter-dashboard bot added the will affect goldens Changes to golden files label May 17, 2023
@flar flar added the autosubmit Merge PR when tree becomes green via auto submit App label May 18, 2023
@auto-submit auto-submit bot merged commit 8089a30 into flutter:master May 18, 2023
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 18, 2023
Closed
justinmc added a commit that referenced this pull request May 18, 2023
@justinmc
Revert "Remove obsolete drawShadow bounds workaround (#127052)"
63f38a2 
This reverts commit 8089a30.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 18, 2023
@engine-flutter-autoroll
8089a30 Remove obsolete drawShadow bounds workaround (flutter/flutter…
5625dd4 
…#127052)
@justinmc justinmc mentioned this pull request May 18, 2023
Merged
zanderso pushed a commit that referenced this pull request May 18, 2023
@justinmc
Revert "Remove obsolete drawShadow bounds workaround" (#127110)
6071f14 
Reverts #127052, which seems to be breaking the build
because of a goldens failure.

```
The following TestFailure was thrown running a test:
Expected: one widget whose rasterized image matches golden image "shadow.PhysicalModel.enabled.png"
  Actual: _WidgetTypeFinder:<exactly one widget with type "Container" (ignoring offstage widgets):
Container(bg: Color(0xfffff59d), margin: EdgeInsets.all(150.0))>
```

See
https://ci.chromium.org/ui/p/flutter/builders/prod/Linux%20web_tests_6/11051/overview
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 18, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 18, 2023
@engine-flutter-autoroll
8089a30 Remove obsolete drawShadow bounds workaround (flutter/flutter…
605e082 
…#127052)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 18, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 18, 2023
@engine-flutter-autoroll
8089a30 Remove obsolete drawShadow bounds workaround (flutter/flutter…
e17ad42 
…#127052)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 18, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 18, 2023
@engine-flutter-autoroll
8089a30 Remove obsolete drawShadow bounds workaround (flutter/flutter…
3146efb 
…#127052)
auto-submit bot pushed a commit to flutter/packages that referenced this pull request May 18, 2023
@engine-flutter-autoroll
Roll Flutter from d0d1feb to 5ae6438 (42 revisions) (#4038)
1e214d7 
Roll Flutter from d0d1feb to 5ae6438 (42 revisions)

flutter/flutter@d0d1feb...5ae6438

2023-05-18 [email protected] Revert "Handle null return from WillPopCallback" (flutter/flutter#127112)
2023-05-18 [email protected] Roll Flutter Engine from a22dd6438335 to 843ce0bba356 (6 revisions) (flutter/flutter#127116)
2023-05-18 [email protected] mark windows build tests non-bringup (flutter/flutter#127059)
2023-05-18 [email protected] Roll Flutter Engine from d97037077963 to a22dd6438335 (1 revision) (flutter/flutter#127071)
2023-05-18 [email protected] Roll Packages from 5c69914 to b31a128 (9 revisions) (flutter/flutter#127109)
2023-05-18 [email protected] Revert "Remove obsolete drawShadow bounds workaround" (flutter/flutter#127110)
2023-05-18 [email protected] Clip search view content during animation (flutter/flutter#126975)
2023-05-18 [email protected] Remove obsolete drawShadow bounds workaround (flutter/flutter#127052)
2023-05-17 [email protected] [Windows] Improve version migration message (flutter/flutter#127048)
2023-05-17 49699333+dependabot[bot]@users.noreply.github.com Bump actions/labeler from 9471598e3b7ff22b2fa181bd79addf94cb3e0847 to 6b107e7a7ee5e054e0bcce60de5181d21e2f00fb (flutter/flutter#127056)
2023-05-17 [email protected] Remove deprecated fixTextFieldOutlineLabel (flutter/flutter#125893)
2023-05-17 [email protected] Roll Flutter Engine from 331c5769e291 to d97037077963 (3 revisions) (flutter/flutter#127053)
2023-05-17 [email protected] shard windows build tests 3 -> 4 (flutter/flutter#127057)
2023-05-17 [email protected] Roll Flutter Engine from 0ae3719d7043 to 331c5769e291 (1 revision) (flutter/flutter#127049)
2023-05-17 [email protected] Ignore unused_element_parameter (flutter/flutter#126926)
2023-05-17 [email protected] Roll Flutter Engine from 47fd496c6f8d to 0ae3719d7043 (2 revisions) (flutter/flutter#127043)
2023-05-17 [email protected] Roll Flutter Engine from 400a26ad8343 to 47fd496c6f8d (3 revisions) (flutter/flutter#127041)
2023-05-17 [email protected] [flutter_tools] unpin and roll camera_android (flutter/flutter#126945)
2023-05-17 [email protected] Handle null return from WillPopCallback (flutter/flutter#127039)
2023-05-17 [email protected] Roll Flutter Engine from 6048360c1837 to 400a26ad8343 (1 revision) (flutter/flutter#127036)
2023-05-17 [email protected] Remove unused (and untested) parameters from private _MasterDetailFlow (flutter/flutter#126935)
2023-05-17 [email protected] Autocomplete async examples (flutter/flutter#126283)
2023-05-17 [email protected] Expose callback that allows focus traversal customization (flutter/flutter#120235)
2023-05-17 [email protected] Roll Packages from b971830 to 5c69914 (5 revisions) (flutter/flutter#127034)
2023-05-17 [email protected] Roll Flutter Engine from ea3f27383acc to 6048360c1837 (2 revisions) (flutter/flutter#127032)
2023-05-17 [email protected] Add missing `Switch.onFocusChange` test (flutter/flutter#126685)
2023-05-17 [email protected] Remove 'url_launcher' from pubspec.yaml. (flutter/flutter#126939)
2023-05-17 [email protected] Roll Flutter Engine from 4b7c37532ea4 to ea3f27383acc (1 revision) (flutter/flutter#126994)
2023-05-17 [email protected] Roll Flutter Engine from 170b45bae571 to 4b7c37532ea4 (1 revision) (flutter/flutter#126985)
2023-05-17 [email protected] Roll Flutter Engine from 28b9dc993c30 to 170b45bae571 (1 revision) (flutter/flutter#126981)
2023-05-17 [email protected] Roll Flutter Engine from 87a03e107df7 to 28b9dc993c30 (1 revision) (flutter/flutter#126974)
2023-05-17 [email protected] Roll Flutter Engine from 1c775e34e2d5 to 87a03e107df7 (3 revisions) (flutter/flutter#126968)
2023-05-16 [email protected] Fix style issues (flutter/flutter#122586)
2023-05-16 [email protected] Manual roll Flutter Engine from fe2476743b59 to 1c775e34e2d5 (10 revisions) (flutter/flutter#126961)
2023-05-16 [email protected] Make SlottedMultiChildRenderObjectWidgetMixin a concrete class (flutter/flutter#126108)
2023-05-16 [email protected] Add ScrollMetrics.extentTotal for completeness (flutter/flutter#126607)
2023-05-16 [email protected] Fix drone_dimensions. (flutter/flutter#126953)
2023-05-16 [email protected] Add checkmark style to CupertinoRadio (flutter/flutter#126480)
2023-05-16 [email protected] Move `Mac_build_test flutter_gallery__transition_perf_e2e_ios` to prod (flutter/flutter#126941)
2023-05-16 [email protected] Fix DataTableThemeData.copyWith handling of dataRowHeight (flutter/flutter#126943)
2023-05-16 [email protected] Fix copyWith method of ActionIconThemeData (flutter/flutter#126763)
2023-05-16 [email protected] Revert "Roll Flutter Engine from fe2476743b59 to 5cf141f7c03c (2 revisions)" (flutter/flutter#126954)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
...
CaseyHillers pushed a commit to CaseyHillers/flutter that referenced this pull request May 24, 2023
@flar
Remove obsolete drawShadow bounds workaround (flutter#127052)
b578b91 
This workaround was created 6 years ago with no links to bug databases to track. As best we can determine, the issue is no longer present in SkPicture or DisplayList and is most likely obsolete. More importantly, though, non-rendering primitives are ignored by the DisplayList construction and so this workaround will just be ignored anyway. If a problem surfaces about this issue that we haven't discovered by a thorough code search of the current code base, then workarounds should be installed within the relevant implementation modules rather than in the framework (and documented with bugs filed in either or both of Flutter's github repos, and/or the Skia issue database).

Workaround originally created in this PR: flutter#9654
CaseyHillers pushed a commit to CaseyHillers/flutter that referenced this pull request May 24, 2023
@justinmc
Revert "Remove obsolete drawShadow bounds workaround" (flutter#127110)
aa786ac 
Reverts flutter#127052, which seems to be breaking the build
because of a goldens failure.

```
The following TestFailure was thrown running a test:
Expected: one widget whose rasterized image matches golden image "shadow.PhysicalModel.enabled.png"
  Actual: _WidgetTypeFinder:<exactly one widget with type "Container" (ignoring offstage widgets):
Container(bg: Color(0xfffff59d), margin: EdgeInsets.all(150.0))>
```

See
https://ci.chromium.org/ui/p/flutter/builders/prod/Linux%20web_tests_6/11051/overview
jason-simmons added a commit to jason-simmons/flutter_engine that referenced this pull request May 25, 2023
@jason-simmons
Apply the drawShadow bounds workaround to the Web HTML backend
b71122f 
Previously the Flutter framework had been inflating the rectangle
computed for the bounds of a drawShadow operation in order to work
around potential inaccuracies in the SkPicture's bounds calculation.

That workaround is now obsolete for most platforms and was removed from
the framework (see flutter/flutter#127052).
But the Web HTML backend is using different code for computing shadow
bounds.  This PR restores the workaround for Web HTML for consistency
with the old behavior.
@jason-simmons jason-simmons mentioned this pull request May 25, 2023
Merged
jason-simmons added a commit to jason-simmons/flutter_engine that referenced this pull request May 25, 2023
@jason-simmons
Apply the drawShadow bounds workaround to the Web HTML backend
c15ddfb 
Previously the Flutter framework had been inflating the rectangle
computed for the bounds of a drawShadow operation in order to work
around potential inaccuracies in the SkPicture's bounds calculation.

That workaround is now obsolete for most platforms and was removed from
the framework (see flutter/flutter#127052).
But the Web HTML backend is using different code for computing shadow
bounds.  This PR restores the workaround for Web HTML for consistency
with the old behavior.
@reidbaker reidbaker mentioned this pull request May 25, 2023
Merged
8 tasks
jason-simmons added a commit to jason-simmons/flutter_engine that referenced this pull request May 25, 2023
@jason-simmons
Apply the drawShadow bounds workaround to the Web HTML backend
2b768b0 
Previously the Flutter framework had been inflating the rectangle
computed for the bounds of a drawShadow operation in order to work
around potential inaccuracies in the SkPicture's bounds calculation.

That workaround is now obsolete for most platforms and was removed from
the framework (see flutter/flutter#127052).
But the Web HTML backend is using different code for computing shadow
bounds.  This PR restores the workaround for Web HTML for consistency
with the old behavior.
auto-submit bot pushed a commit to flutter/engine that referenced this pull request May 26, 2023
@jason-simmons
Apply the drawShadow bounds workaround to the Web HTML backend (#42304)
19b366c 
Previously the Flutter framework had been inflating the rectangle computed for the bounds of a drawShadow operation in order to work around potential inaccuracies in the SkPicture's bounds calculation.

That workaround is now obsolete for most platforms and was removed from the framework (see flutter/flutter#127052). But the Web HTML backend is using different code for computing shadow bounds.  This PR restores the workaround for Web HTML for consistency with the old behavior.
@reidbaker reidbaker mentioned this pull request May 26, 2023
Merged
8 tasks
@flar flar mentioned this pull request May 26, 2023
Closed
flar added a commit to flar/flutter that referenced this pull request Jun 1, 2023
@flar
Remove obsolete drawShadow bounds workaround (flutter#127052)
da48266 
This workaround was created 6 years ago with no links to bug databases to track. As best we can determine, the issue is no longer present in SkPicture or DisplayList and is most likely obsolete. More importantly, though, non-rendering primitives are ignored by the DisplayList construction and so this workaround will just be ignored anyway. If a problem surfaces about this issue that we haven't discovered by a thorough code search of the current code base, then workarounds should be installed within the relevant implementation modules rather than in the framework (and documented with bugs filed in either or both of Flutter's github repos, and/or the Skia issue database).

Workaround originally created in this PR: flutter#9654
auto-submit bot pushed a commit that referenced this pull request Jun 2, 2023
@flar
Reland "Remove obsolete drawShadow bounds workaround (#127052)" (#127231
6f66113 
)

This workaround was created 6 years ago with no links to bug databases to track. As best we can determine, the issue is no longer present in SkPicture or DisplayList and is most likely obsolete. More importantly, though, non-rendering primitives are ignored by the DisplayList construction and so this workaround will just be ignored anyway. If a problem surfaces about this issue that we haven't discovered by a thorough code search of the current code base, then workarounds should be installed within the relevant implementation modules rather than in the framework (and documented with bugs filed in either or both of Flutter's github repos, and/or the Skia issue database).

Workaround originally created in this PR: #9654
nploi pushed a commit to nploi/packages that referenced this pull request Jul 16, 2023
@engine-flutter-autoroll @nploi
Roll Flutter from d0d1feb to 5ae6438 (42 revisions) (flutter#4038)
bfd8555 
Roll Flutter from d0d1feb to 5ae6438 (42 revisions)

flutter/flutter@d0d1feb...5ae6438

2023-05-18 [email protected] Revert "Handle null return from WillPopCallback" (flutter/flutter#127112)
2023-05-18 [email protected] Roll Flutter Engine from a22dd6438335 to 843ce0bba356 (6 revisions) (flutter/flutter#127116)
2023-05-18 [email protected] mark windows build tests non-bringup (flutter/flutter#127059)
2023-05-18 [email protected] Roll Flutter Engine from d97037077963 to a22dd6438335 (1 revision) (flutter/flutter#127071)
2023-05-18 [email protected] Roll Packages from 5c69914 to b31a128 (9 revisions) (flutter/flutter#127109)
2023-05-18 [email protected] Revert "Remove obsolete drawShadow bounds workaround" (flutter/flutter#127110)
2023-05-18 [email protected] Clip search view content during animation (flutter/flutter#126975)
2023-05-18 [email protected] Remove obsolete drawShadow bounds workaround (flutter/flutter#127052)
2023-05-17 [email protected] [Windows] Improve version migration message (flutter/flutter#127048)
2023-05-17 49699333+dependabot[bot]@users.noreply.github.com Bump actions/labeler from 9471598e3b7ff22b2fa181bd79addf94cb3e0847 to 6b107e7a7ee5e054e0bcce60de5181d21e2f00fb (flutter/flutter#127056)
2023-05-17 [email protected] Remove deprecated fixTextFieldOutlineLabel (flutter/flutter#125893)
2023-05-17 [email protected] Roll Flutter Engine from 331c5769e291 to d97037077963 (3 revisions) (flutter/flutter#127053)
2023-05-17 [email protected] shard windows build tests 3 -> 4 (flutter/flutter#127057)
2023-05-17 [email protected] Roll Flutter Engine from 0ae3719d7043 to 331c5769e291 (1 revision) (flutter/flutter#127049)
2023-05-17 [email protected] Ignore unused_element_parameter (flutter/flutter#126926)
2023-05-17 [email protected] Roll Flutter Engine from 47fd496c6f8d to 0ae3719d7043 (2 revisions) (flutter/flutter#127043)
2023-05-17 [email protected] Roll Flutter Engine from 400a26ad8343 to 47fd496c6f8d (3 revisions) (flutter/flutter#127041)
2023-05-17 [email protected] [flutter_tools] unpin and roll camera_android (flutter/flutter#126945)
2023-05-17 [email protected] Handle null return from WillPopCallback (flutter/flutter#127039)
2023-05-17 [email protected] Roll Flutter Engine from 6048360c1837 to 400a26ad8343 (1 revision) (flutter/flutter#127036)
2023-05-17 [email protected] Remove unused (and untested) parameters from private _MasterDetailFlow (flutter/flutter#126935)
2023-05-17 [email protected] Autocomplete async examples (flutter/flutter#126283)
2023-05-17 [email protected] Expose callback that allows focus traversal customization (flutter/flutter#120235)
2023-05-17 [email protected] Roll Packages from b971830 to 5c69914 (5 revisions) (flutter/flutter#127034)
2023-05-17 [email protected] Roll Flutter Engine from ea3f27383acc to 6048360c1837 (2 revisions) (flutter/flutter#127032)
2023-05-17 [email protected] Add missing `Switch.onFocusChange` test (flutter/flutter#126685)
2023-05-17 [email protected] Remove 'url_launcher' from pubspec.yaml. (flutter/flutter#126939)
2023-05-17 [email protected] Roll Flutter Engine from 4b7c37532ea4 to ea3f27383acc (1 revision) (flutter/flutter#126994)
2023-05-17 [email protected] Roll Flutter Engine from 170b45bae571 to 4b7c37532ea4 (1 revision) (flutter/flutter#126985)
2023-05-17 [email protected] Roll Flutter Engine from 28b9dc993c30 to 170b45bae571 (1 revision) (flutter/flutter#126981)
2023-05-17 [email protected] Roll Flutter Engine from 87a03e107df7 to 28b9dc993c30 (1 revision) (flutter/flutter#126974)
2023-05-17 [email protected] Roll Flutter Engine from 1c775e34e2d5 to 87a03e107df7 (3 revisions) (flutter/flutter#126968)
2023-05-16 [email protected] Fix style issues (flutter/flutter#122586)
2023-05-16 [email protected] Manual roll Flutter Engine from fe2476743b59 to 1c775e34e2d5 (10 revisions) (flutter/flutter#126961)
2023-05-16 [email protected] Make SlottedMultiChildRenderObjectWidgetMixin a concrete class (flutter/flutter#126108)
2023-05-16 [email protected] Add ScrollMetrics.extentTotal for completeness (flutter/flutter#126607)
2023-05-16 [email protected] Fix drone_dimensions. (flutter/flutter#126953)
2023-05-16 [email protected] Add checkmark style to CupertinoRadio (flutter/flutter#126480)
2023-05-16 [email protected] Move `Mac_build_test flutter_gallery__transition_perf_e2e_ios` to prod (flutter/flutter#126941)
2023-05-16 [email protected] Fix DataTableThemeData.copyWith handling of dataRowHeight (flutter/flutter#126943)
2023-05-16 [email protected] Fix copyWith method of ActionIconThemeData (flutter/flutter#126763)
2023-05-16 [email protected] Revert "Roll Flutter Engine from fe2476743b59 to 5cf141f7c03c (2 revisions)" (flutter/flutter#126954)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
...
@flar flar mentioned this pull request Jul 21, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2023
@engine-flutter-autoroll
8089a30 Remove obsolete drawShadow bounds workaround (flutter/flutter…
d2ea0f1 
…#127052)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
@engine-flutter-autoroll
8089a30 Remove obsolete drawShadow bounds workaround (flutter/flutter…
6d617bd 
…#127052)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
@engine-flutter-autoroll
8089a30 Remove obsolete drawShadow bounds workaround (flutter/flutter…
ba8aebb 
…#127052)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
@engine-flutter-autoroll
8089a30 Remove obsolete drawShadow bounds workaround (flutter/flutter…
71c4dc4 
…#127052)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jason-simmons jason-simmons jason-simmons approved these changes

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App framework flutter/packages/flutter repository. See also f: labels. will affect goldens Changes to golden files

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@flar @jason-simmons