Oh I see, no that's a great idea actually. Looks good as-is.

When the sensitivity changes, I think we should be sure that:

1. Inside SensitiveContentHost, the platform never gets called twice back-to-back (register/unregister).
2. The build method hides the child until reregistration is finished.

Can you double check that both of those are working properly and consider testing them?

For 1 You might need a SensitiveContentHost.reregister or .update or something.

For 2 you might need to call setState here.

Added a test that I think covers these! I'll think on other cases to test as well.

I can confirm that 1 is pretty thoroughly tested in sensitive_content_host_test.dart and I added new tests for 2.

As a follow-up from the discussion about this call failing (https://github.com/flutter/flutter/pull/158473/files#r1976045506), is there anything we should do for error handling here? Currently, I think if this fails then the app will crash. Would it be better to just log an error and display child without doing anything for SensitiveContent?

Updated the calls to setContentSensitivity to log an error!

Currently I think if this call fails then the whole app will crash right? I left a comment about possibly recovering from this in the widget.

Assuming we don't let the app crash, then after the call fails, _contentSensitivitySetting will have been updated but calculatedContentSensitivityLevel will not. You're saying that's correct, because _contentSensitivitySetting represents the widget tree, and calculatedContentSensitivityLevel represents what we think Android has it set to?

Do you know where is the async operation this is waiting on? It will be better if we wait for a completer future instead of a delay

Ok I thought this was needed to make sure that re-registering the SensitiveContent widget happens, but it seems to pass without it. Let's see what happens in CI because I did see a failure before.

We use a completer to complete the fake async method call, but I thought we needed to wait for the Future that the FutureBuilder relies on to finish to check that the actual SensitiveContent widget child is built. I thought we couldn't use a Completer for this without faking/mocking the Future that the FutureBuilder relies on.

a pump should be enough since unittest uses a fake async. Unless you are introducing scheduleTask or other type a random delay future in the widget, we shouldn't need to wait a real future in unittest. and even then, we should expose a future for test to await in the await tester.runAsync

shouldn't this return error result instead?

No. The idea here was that it a user calls getContentSensitivity and an unknown content sensitivity is returned by Android, we should not crash because the user may have no idea that an unknown one was returned, e.g. if we bump to Android 16 and the default content sensitivity changes. Instead, we return unknown to signal to the framework side to handle this accordingly.

I'm sure we could throw an error to accomplish the same thing but I think using the unknown value is a bit clearer in terms of our intent.

sgtm.

Why are we using string intead of index of enum to communicate the sensitivity level?

right now the communication is the following.

andoird (int) -> converting to String -> channel -> converting string to enum -> dart(enum)

dart(enum) -> converting to String -> channel -> converting string to int -> andoird (int)

we could just do the following

android (int) -> converting to enum int in dart -> channel -> recast int to enum -> dart(enum)

dart(enum) -> recast to int -> channel -> converting to android int -> andoird (int)

Isn't this the same level of work?

Also, I think the Strings are more readable for the serialization/deserialization done on the platform side.

If using string, you will do 2 string to int conversion and sending string through channel is slightly more costly.

If using int, you will do 1 Android int to dart enum conversion.

You can get the same level of readability when using enum. For example the semanticsFlag, we defined one enum in dart, and the matching enum in Android. To make sense of the int passing through channel all you need to do is recast to the respective enum.

for cases where you send _unknown through channel, can we just return error instead?

@reidbaker and I discussed this at length. I'm sure we could accomplish the same thing that way, as I mentioned before, but this gives a standardized way for any platform to signal that an unknown value was detected and we should gracefully fail and not crash the app.

sgtm

If we use int, we don't need to do a linear look up and can just recast with ContentSensitivity.values[result]

I'm not sold on converting back to ints. See #158473 (comment).

we should have one method to replace the new old registration with new one instead of two. having two separate setSenstiveContent call worries me a bit.

this way, you only need to keep track of one future

I actually did this before but I thought it was breaking something. It was not; fixed.

some where in the doc should mention this is not ready for production

Nit: What's the benefit of catching and rethrowing here?

Undid this.; now the SensitiveContentHost just catches the PlatformException

Why the ! here?

The method channel call results in Future<bool> which according to the analyzer, can be null.

Oh right. This is probably another thing that we should do error handling for. Currently it looks like there is no try/catch where this is called in _register. Currently if this call fails, or if it unexpectedly returns null, then the app will crash. Instead I guess we should just consider it to be unsupported?

I think after doing this you also should remove it:

      addTearDown(
        () => tester.binding.defaultBinaryMessenger.setMockMethodCallHandler(
          SystemChannels.sensitiveContent,
          null,
        ),
      );

Won't the tearDown do this?

Ah sorry I totally missed that above, this is good as-is 👍

Thanks for splitting the tests into different files like this.

Thanks for explaining this. I'm glad you ended up not needing pumpAndSettle or expectLater.