|
|
Created:
3 years, 6 months ago by Tim Song Modified:
3 years, 5 months ago Reviewers:
sacomoto CC:
chromium-reviews, jlklein+watch-tether_chromium.org, tengs+watch-tether_chromium.org, hansberry+watch-tether_chromium.org, jhawkins+watch-tether_chromium.org, lesliewatkins+watch-tether_chromium.org, khorimoto+watch-tether_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[EasyUnlock] Display a timeout state when the device isn't connected after a long time.
BUG=726792
Review-Url: https://codereview.chromium.org/2951223002
Cr-Commit-Position: refs/heads/master@{#484801}
Committed: https://chromium.googlesource.com/chromium/src/+/3fc9e0b56e8a5426b6d5c33fb9df67e618bbb48e
Patch Set 1 #
Total comments: 6
Patch Set 2 : fix test #
Messages
Total messages: 19 (9 generated)
tengs@chromium.org changed reviewers: + sacomoto@chromium.org
https://codereview.chromium.org/2951223002/diff/1/components/proximity_auth/u... File components/proximity_auth/unlock_manager_impl.cc (right): https://codereview.chromium.org/2951223002/diff/1/components/proximity_auth/u... components/proximity_auth/unlock_manager_impl.cc:368: if (is_waking_up_) Why don't want you the AUTHENTICATING and FINDING_CONNECTION to show the connecting icon? What icon would it show in this case? https://codereview.chromium.org/2951223002/diff/1/components/proximity_auth/u... components/proximity_auth/unlock_manager_impl.cc:373: // Show a timeout state if we can not connect to the remote device in a It's unclear to me where this timeout is defined. The connection finder never times out.
https://codereview.chromium.org/2951223002/diff/1/components/proximity_auth/u... File components/proximity_auth/unlock_manager_impl.cc (right): https://codereview.chromium.org/2951223002/diff/1/components/proximity_auth/u... components/proximity_auth/unlock_manager_impl.cc:368: if (is_waking_up_) On 2017/06/22 15:49:11, sacomoto wrote: > Why don't want you the AUTHENTICATING and FINDING_CONNECTION to show the > connecting icon? What icon would it show in this case? Basically |is_waking_up_| is true for 15 seconds after the lock screen is first shown. We should always show the spinner in this 15 seconds, unless it is superseded by another state. https://codereview.chromium.org/2951223002/diff/1/components/proximity_auth/u... components/proximity_auth/unlock_manager_impl.cc:373: // Show a timeout state if we can not connect to the remote device in a On 2017/06/22 15:49:11, sacomoto wrote: > It's unclear to me where this timeout is defined. The connection finder never > times out. The connection finder always runs, but the timeout is purely cosmetic. It is set at line 448 in this file. Basically, it's still possible to connect after this timeout, in which case the correct state will be displayed. However, we make the assumption that if you don't connect after a sufficiently long time, your phone is most likely not with you.
lgtm https://codereview.chromium.org/2951223002/diff/1/components/proximity_auth/u... File components/proximity_auth/unlock_manager_impl.cc (right): https://codereview.chromium.org/2951223002/diff/1/components/proximity_auth/u... components/proximity_auth/unlock_manager_impl.cc:368: if (is_waking_up_) On 2017/06/22 20:52:11, Tim Song wrote: > On 2017/06/22 15:49:11, sacomoto wrote: > > Why don't want you the AUTHENTICATING and FINDING_CONNECTION to show the > > connecting icon? What icon would it show in this case? > > Basically |is_waking_up_| is true for 15 seconds after the lock screen is first > shown. > > We should always show the spinner in this 15 seconds, unless it is superseded by > another state. > I see. That makes sense. https://codereview.chromium.org/2951223002/diff/1/components/proximity_auth/u... components/proximity_auth/unlock_manager_impl.cc:373: // Show a timeout state if we can not connect to the remote device in a On 2017/06/22 20:52:11, Tim Song wrote: > On 2017/06/22 15:49:11, sacomoto wrote: > > It's unclear to me where this timeout is defined. The connection finder never > > times out. > > The connection finder always runs, but the timeout is purely cosmetic. It is set > at line 448 in this file. > > Basically, it's still possible to connect after this timeout, in which case the > correct state will be displayed. However, we make the assumption that if you > don't connect after a sufficiently long time, your phone is most likely not with > you. Got it. Thanks for the explanation.
The CQ bit was checked by tengs@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tengs@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sacomoto@chromium.org Link to the patchset: https://codereview.chromium.org/2951223002/#ps20001 (title: "fix test")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by tengs@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 20001, "attempt_start_ts": 1499386422526560, "parent_rev": "8a589e1f43f36535e10667213f4ad19e33ad17e3", "commit_rev": "3fc9e0b56e8a5426b6d5c33fb9df67e618bbb48e"}
Message was sent while issue was closed.
Description was changed from ========== [EasyUnlock] Display a timeout state when the device isn't connected after a long time. BUG=726792 ========== to ========== [EasyUnlock] Display a timeout state when the device isn't connected after a long time. BUG=726792 Review-Url: https://codereview.chromium.org/2951223002 Cr-Commit-Position: refs/heads/master@{#484801} Committed: https://chromium.googlesource.com/chromium/src/+/3fc9e0b56e8a5426b6d5c33fb9df... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/3fc9e0b56e8a5426b6d5c33fb9df... |