Skip to content
Commit 04b56be7 authored by Joshua Trask's avatar Joshua Trask
Browse files

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
parent f8c928be
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment