Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
528423002:
Unload Easy Unlock app when ChromeOS suspends and reload on wake up. (Closed)
DescriptionUnload Easy Unlock app when ChromeOS suspends and reload on wake up.
When the system wakes up, it takes a few seconds for the correct
Bluetooth state to propogate to the app. A bad sync state allows
the lock screen to be unlocked even though the phone is not connected
in reality.
BUG=410082
TEST=manual
Committed: https://crrev.com/668c5c10fd476546fc7d6c22b64c1460dd61b247
Cr-Commit-Position: refs/heads/master@{#294104}
Patch Set 1 #Patch Set 2 : in file #
Total comments: 4
Patch Set 3 : fixes #Patch Set 4 : Unload instead of uninstalling app #
Total comments: 5
Messages
Total messages: 26 (8 generated)
tengs@chromium.org changed reviewers: + xiyuan@chromium.org
lgtm
tbarzic@chromium.org changed reviewers: + tbarzic@chromium.org
https://codereview.chromium.org/528423002/diff/20001/chrome/browser/signin/ea... File chrome/browser/signin/easy_unlock_service.cc (right): https://codereview.chromium.org/528423002/diff/20001/chrome/browser/signin/ea... chrome/browser/signin/easy_unlock_service.cc:128: service_->UnloadApp(); you could add screenlock_state_handler_.reset() to UnloadApp instead of resetting the screenlock_state_handler here. https://codereview.chromium.org/528423002/diff/20001/ui/login/account_picker/... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/528423002/diff/20001/ui/login/account_picker/... ui/login/account_picker/user_pod_row.js:2355: pod.customIconElement.hidden = true; why is this needed?
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/528423002/diff/20001/chrome/browser/signin/ea... File chrome/browser/signin/easy_unlock_service.cc (right): https://codereview.chromium.org/528423002/diff/20001/chrome/browser/signin/ea... chrome/browser/signin/easy_unlock_service.cc:128: service_->UnloadApp(); On 2014/09/03 21:40:37, tbarzic wrote: > you could add screenlock_state_handler_.reset() to UnloadApp instead of > resetting the screenlock_state_handler here. Done. https://codereview.chromium.org/528423002/diff/20001/ui/login/account_picker/... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/528423002/diff/20001/ui/login/account_picker/... ui/login/account_picker/user_pod_row.js:2355: pod.customIconElement.hidden = true; On 2014/09/03 21:40:37, tbarzic wrote: > why is this needed? Changed to use hide() instead.
lgtm
The CQ bit was checked by tengs@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tengs@chromium.org/528423002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
tengs@chromium.org changed reviewers: + noms@chromium.org
+noms for ui/login/account_picker/user_pod_row.js
tengs@chromium.org changed reviewers: + nkostylev@chromium.org
This needs to be merged into 38. Please approve.
Sorry, I didn't get an email the first time you added me to this. lgtm.
Thank you Monica. Xiyuan, Toni, can you take another look at easy_unlock_service.cc? I changed to app unloading to be a disable rather than uninstall. Otherwise, app uninstall logic (eg. gcm cleanup) gets triggered each time.
SLGTM
The CQ bit was checked by tengs@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tengs@chromium.org/528423002/80001
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as c1a2832692d54d52c13b019ae2a198288cd9fc32
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/668c5c10fd476546fc7d6c22b64c1460dd61b247 Cr-Commit-Position: refs/heads/master@{#294104}
Message was sent while issue was closed.
https://codereview.chromium.org/528423002/diff/80001/chrome/browser/signin/ea... File chrome/browser/signin/easy_unlock_service.cc (right): https://codereview.chromium.org/528423002/diff/80001/chrome/browser/signin/ea... chrome/browser/signin/easy_unlock_service.cc:358: } else { We probably want to ensure that the extension is enabled even when it's loaded. (consider the case where the device shuts down during sleep; the app will remain disabled even when it's loaded on the next boot) https://codereview.chromium.org/528423002/diff/80001/chrome/browser/signin/ea... chrome/browser/signin/easy_unlock_service.cc:363: if (registry->GetExtensionById(extension_misc::kEasyUnlockAppId, you can probably skip this if; EnableExtension handles the case when the extension is already enabled. same for DisableExtension bellow
Message was sent while issue was closed.
https://codereview.chromium.org/528423002/diff/80001/chrome/browser/signin/ea... File chrome/browser/signin/easy_unlock_service.cc (right): https://codereview.chromium.org/528423002/diff/80001/chrome/browser/signin/ea... chrome/browser/signin/easy_unlock_service.cc:358: } else { On 2014/09/10 19:56:46, tbarzic wrote: > We probably want to ensure that the extension is enabled even when it's loaded. > (consider the case where the device shuts down during sleep; the app will remain > disabled even when it's loaded on the next boot) Can this scenario even happen? It doesn't seem like component_loader stores extensions that were previously added, so we will re-add this extension when the system reboots. https://codereview.chromium.org/528423002/diff/80001/chrome/browser/signin/ea... chrome/browser/signin/easy_unlock_service.cc:363: if (registry->GetExtensionById(extension_misc::kEasyUnlockAppId, On 2014/09/10 19:56:46, tbarzic wrote: > you can probably skip this if; EnableExtension handles the case when the > extension is already enabled. > > same for DisableExtension bellow Good point. I'll fix it in https://codereview.chromium.org/535343003/.
Message was sent while issue was closed.
https://codereview.chromium.org/528423002/diff/80001/chrome/browser/signin/ea... File chrome/browser/signin/easy_unlock_service.cc (right): https://codereview.chromium.org/528423002/diff/80001/chrome/browser/signin/ea... chrome/browser/signin/easy_unlock_service.cc:358: } else { On 2014/09/11 03:00:05, Tim Song wrote: > On 2014/09/10 19:56:46, tbarzic wrote: > > We probably want to ensure that the extension is enabled even when it's > loaded. > > (consider the case where the device shuts down during sleep; the app will > remain > > disabled even when it's loaded on the next boot) > > Can this scenario even happen? It doesn't seem like component_loader stores > extensions that were previously added, so we will re-add this extension when the > system reboots. Afaik, this may happen e.g. if chrome crashes when waking up, or battery runs out while sleeping. We will re-add the extension, but it will be disabled (the state is saved in profile prefs, so it persists across sessions). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
