Ash: Move TPM firmware update notification to MessageCenter [chromium/src : main]
Georg Neis (Gerrit)
Georg Neis added 2 comments![]( "Open in Gerrit")
const user_manager::User& user = CHECK_DEREF(Please fix this WARNING reported by autoreview issue finding: There is an inconsistency here regarding whether the `User` can be null.
Here, `CHECK_DEREF` will crash if `GetUserByBrowserContext` returns null. However, in `Click()` above, there is a null check (`if (user)`) and a TODO asking to revisit if we always have the user.
If it is possible for this profile to not have an associated user (e.g., on the sign-in screen), this `CHECK_DEREF` will crash and we need to handle it gracefully. If it's guaranteed to always be non-null for the profiles that reach this code, we should remove the null check and TODO in `Click()` to avoid confusion.
}Same question as in the other CL but here even more relevant due to the retained Profile pointer: what happens when the user logs out?
Related details
- Di Wu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Di Wu (Gerrit)
Di Wu added 2 comments![]( "Open in Gerrit")
Please fix this WARNING reported by autoreview issue finding: There is an inconsistency here regarding whether the `User` can be null.
Here, `CHECK_DEREF` will crash if `GetUserByBrowserContext` returns null. However, in `Click()` above, there is a null check (`if (user)`) and a TODO asking to revisit if we always have the user.
If it is possible for this profile to not have an associated user (e.g., on the sign-in screen), this `CHECK_DEREF` will crash and we need to handle it gracefully. If it's guaranteed to always be non-null for the profiles that reach this code, we should remove the null check and TODO in `Click()` to avoid confusion.
Thanks. The TODO from `crbug.com/447287122` (which itself was a broad sweep of 20 or more CLs across the code base) was not taking into account the real UX flow here and is overly conservative. The non-null is guaranteed as can be seen [here].
The null check and the todo is now deleted.
Same question as in the other CL but here even more relevant due to the retained Profile pointer: what happens when the user logs out?
Great question. This touches the soul of this series of NDS removal. Message center dies with Ash shell, which dies noticeably earlier than the destruction of the profile. And this notification dies with message center. By the time the user logs out and profile starts getting torn down. Both message center and the tpm notification are long dead.
Related details
- Georg Neis
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Georg Neis (Gerrit)
Georg Neis added 3 comments![]( "Open in Gerrit")
const std::string& notification_id)Can we pass by value and use move semantics?
"%s#%s", kTPMFirmwareUpdateNotificationId, user.username_hash().c_str());I think we should define a function for this encoding and use it here and elsewhere. Maybe add to notification_utils.cc?
Di WuSame question as in the other CL but here even more relevant due to the retained Profile pointer: what happens when the user logs out?
Great question. This touches the soul of this series of NDS removal. Message center dies with Ash shell, which dies noticeably earlier than the destruction of the profile. And this notification dies with message center. By the time the user logs out and profile starts getting torn down. Both message center and the tpm notification are long dead.
I was thinking of a secondary user. But looks like it's impossible to sign out only a secondary user.
Related details
- Di Wu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |