Inline & remove extraneous helper method.
This is trivially a "pure refactoring" (after a fair amount of
reasoning, to follow in this CL description). We have ongoing work
both to unify the TargetInfo APIs (go/chooser-targetinfo-cleanup) and
to reduce our internal reliance on the deprecated `ChooserTarget` API.
This CL simplifies one usage site in advance of further cleanup work,
while removing one of our `ChooserTarget`-based internal APIs. The old
design was also unnecessarily flexible, requiring us to reason through
runtime behavior to prove(*) the equivalence of this CL; that
reasoning is much more apparent the way it's written now.
* [Proof of equivalence]
By inspection in `ChooserListAdapter`, the `ChooserTarget` was
originally accessed by
```
public ChooserTarget getChooserTargetForValue(int value) {
return mServiceTargets.get(value).getChooserTarget();
}
```
We assert that the call to `mServiceTargets.get(value)` will retrieve
the same `ChooserTargetInfo` instance as already returned by
`ChooserListAdapter.targetInfoForPosition(which, filtered)` (noting
that `value` and `which` are aliases in all the relevant legacy code).
The logic in `targetInfoForPosition()` returns the same
`mServiceTargets.get(position)` (with `position` another alias of
`value`/`which`) for any non-negative `position`, up to some
`serviceTargetCount` which depends on the value of `filtered` (a
parameter that seems a little confusing & not well-documented, but
we'll leave that detail out-of-scope for now). This is somewhat
concerning because our `filtered` value comes from the argument to
`ChooserActivity.startSelected()`, and it's passed on to
`ChooserListAdapter.targetInfoForPosition()` but *not* to the helper
`getChooserTargetForValue()` -- suggesting a possible bug if
`targetInfoForPosition()` ever in fact returns a different value
depending on the choice of `filtered` parameter, since then the two
getters would return info about different targets, and our log event
would get misattributed. (The ability to return a `ChooserTarget` that
isn't composed-into the `TargetInfo` at that position is the
aforementioned "extra flexibility" that can do us no good.)
The proposed change only applies in the case when
`ChooserListAdapter.getPositionTargetType(which)` returns
`TARGET_SERVICE`, implying `which < getServiceTargetCount()`; this is
the same limit selected for `targetInfoForPosition()` when `filtered`
is true, so in that event the behavior will be the same.
If `filtered` is false, `targetInfoForPosition()` instead compares to
a limit given by `getSelectableServiceTargetCount()`, the number of
elements in `mServiceTargets` that are instances of
`SelectableTargetInfo`. (We can already see this *should* be safe,
since the pre-existing code in `ChooserActivity.startSelected()`
already included an unchecked downcast to `SelectableTargetInfo`, and
we would've observed a "noisier" crash if there was a problem with
that assumption; note no other branch of `targetInfoForPosition()`
returns objects of any subtype of `SelectableTargetInfo`, so these
instances must have been retrieved directly from `mServiceTargets`.
It may also be reasonable to assume that a target we examine in the
`startSelected()` method is in fact "Selectable", which in fact it
already checks -- *as long as the target is a subclass of
`ChooserTargetInfo`*, and thus partitioned into being either a
`Selectable-` or `NotSelectableTargetInfo`).
For the hypothetical bug to occur, we would need to be requesting the
`TargetInfo` for a `position` greater than the value returned from
`getSelectableServiceTargetCount()` (so that `targetInfoForPosition()`
goes on to take the target from somewhere other than the corresponding
position in the `mServiceTargets` list), but less than the value
returned from `getServiceTargetCount()` (so that the target is still
classified as a `TARGET_SERVICE`). Thus we're concerned with cases
when the "selectable" target count might be lower than the total
target count (even after accounting for any other conditions in
`getServiceTargetCount()`). Inspecting all the insertions to
`ChooserListAdapter.mServiceTargets` we note that each site takes care
to honor the limit set by
`mChooserListCommunicator.getMaxRankedTargets()`; that is,
`mServiceTargets.size()` should never exceed this limit, and the
`Math.min` expression in `getServiceTargetCount()` should always be a
no-op. Thus the result of `getSelectableServiceTargetCount()` can only
be lower *because* it filtered out exactly that many service targets
that didn't inherit from `SelectableTargetInfo`. OTOH we can see from
`ChooserListAdapter.getSurfacedTargetInfo()` that the "selectable"
targets are represented as a *prefix* of the `mServiceTargets` list
(i.e., any filtered-out "non-selectable" items can only occur after
all others), and thus any problematic `position` we might query beyond
the limit of `getSelectableServiceTargetCount()` would have to be a
non-selectable item (by inspection, one of `ChooserActivity`'s two
inner-class implementations of `NotSelectableTargetInfo`). But any
item of these non-selectable types would be discarded by the guard
clause at the top of `ChooserActivity.startSelected()`, so any item
that we went on to process for logging must be from one of the
earlier indices directly into `mServiceTargets`, QED.
Test: atest IntentResolverUnitTests
Bug: 202167050
Change-Id: Ia6435345786d43e26be6384c4de941fd86c6079d
2 files changed