Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2861913002:
[EasyUnlock] Fix crash when the same user pod is refocused. (Closed)
|
Created:
3 years, 7 months ago by Tim Song Modified:
3 years, 7 months ago 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] Fix crash when the same user pod is refocused.
Review-Url: https://codereview.chromium.org/2861913002
Cr-Commit-Position: refs/heads/master@{#471877}
Committed: https://chromium.googlesource.com/chromium/src/+/e1186dcd515d1a52561460501faf2a5b5e97ab51
Patch Set 1 : [EasyUnlock] Fix crash when the same user pod is refocused. #
Total comments: 10
Patch Set 2 : fixes #
Total comments: 2
Patch Set 3 : fixes #
Messages
Total messages: 17 (8 generated)
tengs@chromium.org changed reviewers: + sacomoto@chromium.org
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
tengs@chromium.org changed reviewers: + hansberry@chromium.org
Hey Ryan, both Gustavo and Kyle are OOO. Can you take a look at this CL?
https://codereview.chromium.org/2861913002/diff/40001/components/proximity_au... File components/proximity_auth/proximity_auth_system.cc (right): https://codereview.chromium.org/2861913002/diff/40001/components/proximity_au... components/proximity_auth/proximity_auth_system.cc:139: PA_LOG(INFO) << "User " << account_id.Serialize() << " already focused."; nit: describe more what happened, e.g. "Refocused on a user which was already focused." https://codereview.chromium.org/2861913002/diff/40001/components/proximity_au... File components/proximity_auth/proximity_auth_system_unittest.cc (right): https://codereview.chromium.org/2861913002/diff/40001/components/proximity_au... components/proximity_auth/proximity_auth_system_unittest.cc:316: EXPECT_CALL(*unlock_manager_, SetRemoteDeviceLifeCycle(nullptr)) n00b question: I've generally been told that EXPECT_CALL should be at the top of a test method before actual work -- why did you place it at the bottom. Additionally, is Times(AtLeast(1)) correct? Shouldn't it be *exactly* once? https://codereview.chromium.org/2861913002/diff/40001/components/proximity_au... File components/proximity_auth/proximity_monitor_impl.cc (right): https://codereview.chromium.org/2861913002/diff/40001/components/proximity_au... components/proximity_auth/proximity_monitor_impl.cc:196: PA_LOG(INFO) << " Rolling RSSI: " << *rssi_rolling_average_; nit: Why the indententation?
https://codereview.chromium.org/2861913002/diff/40001/components/proximity_au... File components/proximity_auth/proximity_auth_system.cc (right): https://codereview.chromium.org/2861913002/diff/40001/components/proximity_au... components/proximity_auth/proximity_auth_system.cc:139: PA_LOG(INFO) << "User " << account_id.Serialize() << " already focused."; On 2017/05/09 02:51:43, Ryan Hansberry wrote: > nit: describe more what happened, e.g. "Refocused on a user which was already > focused." Done. https://codereview.chromium.org/2861913002/diff/40001/components/proximity_au... File components/proximity_auth/proximity_auth_system_unittest.cc (right): https://codereview.chromium.org/2861913002/diff/40001/components/proximity_au... components/proximity_auth/proximity_auth_system_unittest.cc:316: EXPECT_CALL(*unlock_manager_, SetRemoteDeviceLifeCycle(nullptr)) On 2017/05/09 02:51:43, Ryan Hansberry wrote: > n00b question: I've generally been told that EXPECT_CALL should be at the top of > a test method before actual work -- why did you place it at the bottom. > Additionally, is Times(AtLeast(1)) correct? Shouldn't it be *exactly* once? Ah, this EXPECT_CALL is to catch the call when the |unlock_manager_| is destroyed. You can put EXPECT_CALL anywhere, as long as its before when the call happens. We use AtLeast(1) in order to give the option for the ProximityAuthSystem to make multiple idempotent calls. https://codereview.chromium.org/2861913002/diff/40001/components/proximity_au... File components/proximity_auth/proximity_monitor_impl.cc (right): https://codereview.chromium.org/2861913002/diff/40001/components/proximity_au... components/proximity_auth/proximity_monitor_impl.cc:196: PA_LOG(INFO) << " Rolling RSSI: " << *rssi_rolling_average_; On 2017/05/09 02:51:43, Ryan Hansberry wrote: > nit: Why the indententation? For me, at least, it makes it stand out/easier to ignore in the logs, as this can be quite spammy.
LGTM. https://codereview.chromium.org/2861913002/diff/60001/components/proximity_au... File components/proximity_auth/proximity_auth_system_unittest.cc (right): https://codereview.chromium.org/2861913002/diff/60001/components/proximity_au... components/proximity_auth/proximity_auth_system_unittest.cc:311: // Focusing the user again should be indempotent. The screenlock UI may call nit: s/indempotent/idempotent/.
lgtm with nits https://codereview.chromium.org/2861913002/diff/40001/components/proximity_au... File components/proximity_auth/proximity_auth_system_unittest.cc (right): https://codereview.chromium.org/2861913002/diff/40001/components/proximity_au... components/proximity_auth/proximity_auth_system_unittest.cc:316: EXPECT_CALL(*unlock_manager_, SetRemoteDeviceLifeCycle(nullptr)) On 2017/05/10 18:11:16, Tim Song wrote: > On 2017/05/09 02:51:43, Ryan Hansberry wrote: > > n00b question: I've generally been told that EXPECT_CALL should be at the top > of > > a test method before actual work -- why did you place it at the bottom. > > Additionally, is Times(AtLeast(1)) correct? Shouldn't it be *exactly* once? > > Ah, this EXPECT_CALL is to catch the call when the |unlock_manager_| is > destroyed. You can put EXPECT_CALL anywhere, as long as its before when the call > happens. > > We use AtLeast(1) in order to give the option for the ProximityAuthSystem to > make multiple idempotent calls. Please update the comment "SetRemoteDeviceLifeCycle() is only expected to be called once." to reflect this. https://codereview.chromium.org/2861913002/diff/40001/components/proximity_au... File components/proximity_auth/proximity_monitor_impl.cc (right): https://codereview.chromium.org/2861913002/diff/40001/components/proximity_au... components/proximity_auth/proximity_monitor_impl.cc:196: PA_LOG(INFO) << " Rolling RSSI: " << *rssi_rolling_average_; On 2017/05/10 18:11:16, Tim Song wrote: > On 2017/05/09 02:51:43, Ryan Hansberry wrote: > > nit: Why the indententation? > > For me, at least, it makes it stand out/easier to ignore in the logs, as this > can be quite spammy. If this is a particularly spammy log, then is INFO the correct granularity?
https://codereview.chromium.org/2861913002/diff/40001/components/proximity_au... File components/proximity_auth/proximity_auth_system_unittest.cc (right): https://codereview.chromium.org/2861913002/diff/40001/components/proximity_au... components/proximity_auth/proximity_auth_system_unittest.cc:316: EXPECT_CALL(*unlock_manager_, SetRemoteDeviceLifeCycle(nullptr)) On 2017/05/15 15:48:05, Ryan Hansberry wrote: > On 2017/05/10 18:11:16, Tim Song wrote: > > On 2017/05/09 02:51:43, Ryan Hansberry wrote: > > > n00b question: I've generally been told that EXPECT_CALL should be at the > top > > of > > > a test method before actual work -- why did you place it at the bottom. > > > Additionally, is Times(AtLeast(1)) correct? Shouldn't it be *exactly* once? > > > > Ah, this EXPECT_CALL is to catch the call when the |unlock_manager_| is > > destroyed. You can put EXPECT_CALL anywhere, as long as its before when the > call > > happens. > > > > We use AtLeast(1) in order to give the option for the ProximityAuthSystem to > > make multiple idempotent calls. > > Please update the comment "SetRemoteDeviceLifeCycle() is only expected to be > called once." to reflect this. Done. https://codereview.chromium.org/2861913002/diff/40001/components/proximity_au... File components/proximity_auth/proximity_monitor_impl.cc (right): https://codereview.chromium.org/2861913002/diff/40001/components/proximity_au... components/proximity_auth/proximity_monitor_impl.cc:196: PA_LOG(INFO) << " Rolling RSSI: " << *rssi_rolling_average_; On 2017/05/15 15:48:05, Ryan Hansberry wrote: > On 2017/05/10 18:11:16, Tim Song wrote: > > On 2017/05/09 02:51:43, Ryan Hansberry wrote: > > > nit: Why the indententation? > > > > For me, at least, it makes it stand out/easier to ignore in the logs, as this > > can be quite spammy. > > If this is a particularly spammy log, then is INFO the correct granularity? It's not spammy in the global sense as this is only logged when the remote device is connected and authenticated. However, it is a bit spammy when I try to debug what's going on. I think this is the correct level. https://codereview.chromium.org/2861913002/diff/60001/components/proximity_au... File components/proximity_auth/proximity_auth_system_unittest.cc (right): https://codereview.chromium.org/2861913002/diff/60001/components/proximity_au... components/proximity_auth/proximity_auth_system_unittest.cc:311: // Focusing the user again should be indempotent. The screenlock UI may call On 2017/05/15 14:06:26, sacomoto wrote: > nit: s/indempotent/idempotent/. Done.
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, hansberry@chromium.org Link to the patchset: https://codereview.chromium.org/2861913002/#ps80001 (title: "fixes")
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": 80001, "attempt_start_ts": 1494873053152950,
"parent_rev": "dc5e1afbd1f6b01e44b58ee16f4c556d2ceaa04c", "commit_rev":
"e1186dcd515d1a52561460501faf2a5b5e97ab51"}
Message was sent while issue was closed.
Description was changed from ========== [EasyUnlock] Fix crash when the same user pod is refocused. ========== to ========== [EasyUnlock] Fix crash when the same user pod is refocused. Review-Url: https://codereview.chromium.org/2861913002 Cr-Commit-Position: refs/heads/master@{#471877} Committed: https://chromium.googlesource.com/chromium/src/+/e1186dcd515d1a52561460501faf... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as https://chromium.googlesource.com/chromium/src/+/e1186dcd515d1a52561460501faf... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
