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

Conversation

@gaaclarke
Copy link
Member

issue: flutter/flutter#124277

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@gaaclarke gaaclarke requested a review from bdero April 5, 2023 22:33
Copy link
Member

@bdero bdero left a comment

Choose a reason for hiding this comment

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

LGTM!

paint.color = Color(0.1, 0.1, 0.1, 1.0);
canvas.DrawRect(
Rect::MakeLTRB(0, 0, GetWindowSize().width, GetWindowSize().height),
paint);
Copy link
Member

Choose a reason for hiding this comment

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

nit: DrawPaint would also work here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call. I guess we should probably add docstrings. It didn't occur to me that's what it would do, but makes sense.

@flutter-dashboard
Copy link

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.

Changes reported for pull request #40954 at sha 8f14f89

@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 5, 2023
@auto-submit auto-submit bot merged commit df53c24 into flutter:main Apr 5, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 6, 2023
zanderso pushed a commit to flutter/flutter that referenced this pull request Apr 6, 2023
…ions) (#124300)

Manual roll requested by [email protected]

flutter/engine@610cdf0...1ec74c6

2023-04-06 [email protected] Revert "[web] Move
text editing nodes outside of shadowDOM - reopened"
(flutter/engine#40960)
2023-04-05 [email protected] [web] Move text
editing nodes outside of shadowDOM - reopened (flutter/engine#40904)
2023-04-05 [email protected] [Impeller]
adjusted the rotated text test to have an opaque background and account
for screen scale (flutter/engine#40954)
2023-04-05 [email protected] Copy canvaskit files directly into
`flutter_web_sdk` (flutter/engine#40951)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected],[email protected] on
the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@dnfield
Copy link
Contributor

dnfield commented Apr 7, 2023

I'm seeing this flake still. I think we need to have something in gold that accounts for the GPU model.

@gaaclarke
Copy link
Member Author

I'm seeing this flake still. I think we need to have something in gold that accounts for the GPU model.

Is this flaking or getting confused since it is so different than the old goldens? It seems like there should be a way to reset the test but I couldn't find a way.

@gaaclarke
Copy link
Member Author

I've added an ignore rule for that test: https://flutter-engine-gold.skia.org/ignores

I think this will allow us to observe all the variations and decide what to do.

exaby73 pushed a commit to NevercodeHQ/flutter that referenced this pull request Apr 17, 2023
…ions) (flutter#124300)

Manual roll requested by [email protected]

flutter/engine@610cdf0...1ec74c6

2023-04-06 [email protected] Revert "[web] Move
text editing nodes outside of shadowDOM - reopened"
(flutter/engine#40960)
2023-04-05 [email protected] [web] Move text
editing nodes outside of shadowDOM - reopened (flutter/engine#40904)
2023-04-05 [email protected] [Impeller]
adjusted the rotated text test to have an opaque background and account
for screen scale (flutter/engine#40954)
2023-04-05 [email protected] Copy canvaskit files directly into
`flutter_web_sdk` (flutter/engine#40951)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected],[email protected] on
the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App e: impeller will affect goldens

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants