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

Conversation

ColdPaleLight
Copy link
Member

@ColdPaleLight ColdPaleLight commented Mar 31, 2022

fix flutter/flutter#101120

related PR: #32106

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.

@@ -34,7 +34,8 @@ class TestIOManager final : public IOManager {
: nullptr),
unref_queue_(fml::MakeRefCounted<SkiaUnrefQueue>(
task_runner,
fml::TimeDelta::FromNanoseconds(0))),
fml::TimeDelta::FromNanoseconds(0),
gl_context_)),
Copy link
Member

Choose a reason for hiding this comment

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

It looks like there are several places other than this in the code where a context is not passed to the SkiaUnrefQueue constructor. If failing to pass the context where needed can result in crashes, then that usage of optional arguments is probably not in agreement with the Google style guide. I'd like to hear thoughts from @jason-simmons on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to making this a require parameter.

Copy link
Member

Choose a reason for hiding this comment

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

Making it a required parameter would be ideal.

AFAICT making it optional is a workaround for scenarios like unit tests where it may be impractical to provide something resembling a GrContext. It would be preferable to avoid that if possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

The class was written as a template class so that tests could explicitly create their own idea of what a context is.

We should make it non-optional and consider adding a DCHECK that it's not nullptr in the constructor.

@zanderso
Copy link
Member

This issue is causing roll blockage now. So I'm going to land this. I filed flutter/flutter#101142 for the API cleanup.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flake in ImageDecoderFixtureTest.ExifDataIsRespectedOnDecode in Skia roll presubmit
4 participants