Add explicit completion condition for Direct Share
Prior to this change, we've implicitly relied on the async
Direct Share results coming in before the service watchdog
timer goes off. This has several issues -- the results may
not *actually* come in before the timeout; the old flow
still depended on us waiting for the full timeout even if
we were done sooner; and it's only somewhat inci dental
that we were even still scheduling the timer in the first
place, since that occurs on a code branch that's slated
for deletion. This CL makes those assumptions explicit in
preparation to remove the timeout logic altogether (and
then from there to remove the rest of the ChooserTargetService
support in ChooserActivity).
There are a couple concerns we should consider in review:
1. I don't know why the sendShareShortcutInfoList()
helper had been implemented to send RESULT_COMPLETED
only if it notifies at least one SHARE_TARGET_RESULT
event. It seems to me that this is an essential step
to gate the progress of our async flow, but (on a new
phone with no apps installed as share targets) I
ended up never seeing the COMPLETED event at all.
With the change from this CL, I see the COMPLETED event
exactly when I would expect -- but I could easily be
missing something about the original intention of
this code. (However I will note that there was no
logic to retry the request if resultMessageSent is
false, so it *seems* like we're just left hanging
in this case?)
2. In practice, this change allows the flow to complete
immediately once the Direct Share targets are received,
since we'll never have any service connections to
wait for. That's great -- it means we can get to the
completeServiceTargetLoading() event that signals the
end of our flow, effectively one full second earlier
(confirmed in informal testing). However, as currently
written, it's actually possible that we'll signal
completeServiceTargetLoading() more than once;
ChooserHandler's maybeStopServiceRequestTimer() method
is poorly-named and doesn't actually cancel the
timer, nor otherwise ensure that we haven't already
completed.
IMO this should be OK since it's already happening
without this change (we were already firing twice, for
the min & max watchdog timer events), and I expect to
clean it up in my next CL anyways. Just wanted to call
attention since this will result in one *additional*
"extra" event until the timer logic is removed.
Test: manual (may need more coverage)
Change-Id: I62c714b235fe5521b1ad01c3556a5a6be1db539c
1 file changed