commit fb8126b8fc336d681ba2ded6214d90a6f113b92e
author Joshua Trask <joshtrask@google.com> Mon Sep 25 17:29:32 2023 +0000
committer Joshua Trask <joshtrask@google.com> Mon Sep 25 18:24:27 2023 +0000
tree a73de9383da9e693437e8c56c92fb00061d0e13d
parent cadd75621c2b819035e010bc320206f7549847ef

For consistency, stop throttling "list ready" CBs.

This change is best understood through the newly-introduced
`testPostListReadyAtEndOfRebuild_` tests:
 1. The `synchronous` and `stages` variants pass both before and
    after this change, to show the expected "normal" behavior.
 2. The `queued` variant exercises the hypothetical bug that motivated
    this change; it only passes once the legacy "throttling" condition
    is removed in this CL.
 3. The `skippedIfStillQueuedOnDestroy` variant covers a side-effect
    of the fix. The original implementation had logic for this
    "skipping" condition (and presumably would've passed this new
    test as-written), but because the fix relaxes the invariant where
    we can have only a single "active" callback instance at a time, I
    also had to introduce the new `mDestroyed` condition to effect the
    same high-level "cancellation" behavior. (This also addresses a
    minor theoretical bug where the "destroy" could race against our
    internal two-stage asynchronous flow, such that we'd end up
    posting the follow-up callback to the handler after we'd
    supposedly already been destroyed. I didn't think it was important
    to test for this bug separately from the other coverage of the
    `mDestroyed` condition.)

  --

This CL removes a "throttling" condition that was unneeded, but which
could cause a hypothetical bug (reproducible in tests). The original
condition prevented list-ready messages if we'd already had one posted
in the queue (but not yet processed); however, there was no check
that the messages had the same parameters to indicate "partial" vs.
"complete" progress. Since the legacy mechanism favored the earlier
messages, we could end up dropping the one-off "completion" message
where our listener actually does work -- i.e., if we had to drop
messages, this is exactly the one we would want *not* to drop.

I believe this "throttling" mechanism was likely an optimization to
support the legacy `ChooserTargetService` design; the simplified
newer design requests fewer callbacks and so the throttling should
rarely come into play (but presents a bigger risk whenever it might).
Even in the older design, I suspect there would've been a risk of the
same "dropped completion" bug.

And, of course, it's nice to "simplify" by removing this condition,
even if it *weren't* strictly harmful.

Update: the second snapshot removes the old "callback removal" on
destroy, since the new `mDestroyed` condition gives effectively the
same behavior. Technically that's the only reason we depended on
`Handler` and we could now switch to using an `Executor` or etc -- but
I definitely want to keep that update to a separate CL.

Bug: as described above
Test: IntentResolverUnitTests, CtsSharesheetDeviceTest

Change-Id: Ifda9dc9a8ac8512d241e15fe52f24c3dea5bd9e7