Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2179283002:
Refactored signin/sync error controllers for the avatar button (Closed)
DescriptionRefactored signin/sync error controllers for the avatar button
Background:
The titlebar avatar button needs to display a warning icon when there is:
1. an auth error; or
2. a sync error, including unrecoverable errors, outdated client error, and sync
passphrase error (MD user menu only).
Its auth error controller and sync error controller previously resided in its UI
code, new_avatar_button.
Meanwhile, the MD user menu displays an error message and button for the same set
of errors, and the logic for classifying each error previously resided in its UI
code, profile_chooser_view.
In this CL, the following refactoring was done:
1. sync error controller for avatar button: from new_avatar_button to
avatar_sync_error_controller;
2. signin error controller for avatar button: from new_avatar_button to
avatar_signin_error_controller;
3. the logic for classifying different errors: from profile_chooser_view to
sync_ui_util.
They will all be used on both Linux UI (this CL) and Mac UI (followup CL).
BUG=615893
Committed: https://crrev.com/e4525c4a250b0d405abaecd6ba9bae7e46bfdda3
Cr-Commit-Position: refs/heads/master@{#409439}
Patch Set 1 : Prototype #
Total comments: 6
Patch Set 2 : Working version #Patch Set 3 : Refactored logic for classifying errors #
Total comments: 2
Patch Set 4 : Enum name change #
Total comments: 6
Patch Set 5 : Made nested classes private #
Total comments: 2
Patch Set 6 : New refactoring #
Total comments: 6
Patch Set 7 : Pure virtual interface #
Total comments: 9
Patch Set 8 : Comments #
Total comments: 2
Patch Set 9 : Switch statement minor change #Patch Set 10 : Renaming NO_ERROR to prevent WIN compilation error #Patch Set 11 : Uninitialized ptr #Messages
Total messages: 48 (27 generated)
janeliulwq@google.com changed reviewers: + rogerta@chromium.org
Hi Roger, I put together a working prototype for the refactored sync error observer that will be used in both Linux and Mac. In this CL, I made a new class that observers sync error, and inherited from the new class in the Linux view controller to update the avatar button. Could you take a quick look and let me know if this overall design (and naming) looks ok? If so, I will then proceed to refactoring signin error observer the same way. Thanks!
Looks good Jane, a few nits below. https://codereview.chromium.org/2179283002/diff/1/chrome/browser/sync/avatar_... File chrome/browser/sync/avatar_sync_error.h (right): https://codereview.chromium.org/2179283002/diff/1/chrome/browser/sync/avatar_... chrome/browser/sync/avatar_sync_error.h:14: class AvatarSyncError : public SyncErrorController::Observer { Rename to AvatarSyncErrorController might be better. https://codereview.chromium.org/2179283002/diff/1/chrome/browser/sync/avatar_... chrome/browser/sync/avatar_sync_error.h:22: bool HasSyncError() { return has_sync_error_; } According to style guide this method should be named: bool has_sync_error() or maybe more simply: bool has_error() https://codereview.chromium.org/2179283002/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/new_avatar_button.h (right): https://codereview.chromium.org/2179283002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/new_avatar_button.h:86: bool has_sync_error_; I don't think you need this member anymore. Anywhere where you accessed |has_sync_error_|, you can now access |avatar_sync_error_.has_error()|
Description was changed from ========== Working prototype of refactored sync error observer BUG= ========== to ========== Refactored signin/sync error controllers for the avatar button The titlebar avatar button needs to display a warning icon when there is: 1. an auth error; or 2. a sync error, including unrecoverable errors, outdated client error, and sync passphrase error (MD user menu only). Its auth error controller and sync error controller previously reside in the UI code (new_avatar_button). In this CL, they are being factored out to be in avatar_signin_error_controller and avatar_sync_error_controller. They will be used on both Linux UI (this CL) and Mac UI (followup CL). BUG=615893 ==========
janeliulwq@google.com changed required reviewers: + rogerta@chromium.org
Hi Roger, I addressed your comments and completed the CL. PTAL, thanks! https://codereview.chromium.org/2179283002/diff/1/chrome/browser/sync/avatar_... File chrome/browser/sync/avatar_sync_error.h (right): https://codereview.chromium.org/2179283002/diff/1/chrome/browser/sync/avatar_... chrome/browser/sync/avatar_sync_error.h:14: class AvatarSyncError : public SyncErrorController::Observer { On 2016/07/26 14:34:24, Roger Tawa wrote: > Rename to AvatarSyncErrorController might be better. Done. https://codereview.chromium.org/2179283002/diff/1/chrome/browser/sync/avatar_... chrome/browser/sync/avatar_sync_error.h:22: bool HasSyncError() { return has_sync_error_; } On 2016/07/26 14:34:24, Roger Tawa wrote: > According to style guide this method should be named: > > bool has_sync_error() > > or maybe more simply: bool has_error() Done. https://codereview.chromium.org/2179283002/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/new_avatar_button.h (right): https://codereview.chromium.org/2179283002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/new_avatar_button.h:86: bool has_sync_error_; On 2016/07/26 14:34:24, Roger Tawa wrote: > I don't think you need this member anymore. Anywhere where you accessed > |has_sync_error_|, you can now access |avatar_sync_error_.has_error()| Done.
Description was changed from ========== Refactored signin/sync error controllers for the avatar button The titlebar avatar button needs to display a warning icon when there is: 1. an auth error; or 2. a sync error, including unrecoverable errors, outdated client error, and sync passphrase error (MD user menu only). Its auth error controller and sync error controller previously reside in the UI code (new_avatar_button). In this CL, they are being factored out to be in avatar_signin_error_controller and avatar_sync_error_controller. They will be used on both Linux UI (this CL) and Mac UI (followup CL). BUG=615893 ========== to ========== Refactored signin/sync error controllers for the avatar button The titlebar avatar button needs to display a warning icon when there is: 1. an auth error; or 2. a sync error, including unrecoverable errors, outdated client error, and sync passphrase error (MD user menu only). Its auth error controller and sync error controller previously reside in the UI code (new_avatar_button). In this CL, they are being factored out to be in avatar_signin_error_controller and avatar_sync_error_controller. Also, the logic in profile_chooser_view that was used to classify sync errors are factored out to be in sync_ui_util. They will be used on both Linux UI (this CL) and Mac UI (followup CL). BUG=615893 ==========
Description was changed from ========== Refactored signin/sync error controllers for the avatar button The titlebar avatar button needs to display a warning icon when there is: 1. an auth error; or 2. a sync error, including unrecoverable errors, outdated client error, and sync passphrase error (MD user menu only). Its auth error controller and sync error controller previously reside in the UI code (new_avatar_button). In this CL, they are being factored out to be in avatar_signin_error_controller and avatar_sync_error_controller. Also, the logic in profile_chooser_view that was used to classify sync errors are factored out to be in sync_ui_util. They will be used on both Linux UI (this CL) and Mac UI (followup CL). BUG=615893 ========== to ========== Refactored signin/sync error controllers for the avatar button The titlebar avatar button needs to display a warning icon when there is: 1. an auth error; or 2. a sync error, including unrecoverable errors, outdated client error, and sync passphrase error (MD user menu only). Its auth error controller and sync error controller previously reside in the UI code (new_avatar_button). In this CL, they are being factored out to be in avatar_signin_error_controller and avatar_sync_error_controller. Also, the logic in profile_chooser_view that was used to classify sync errors are factored out to be in sync_ui_util. They will be used on both Linux UI (this CL) and Mac UI (followup CL). BUG=615893 ==========
Hi Roger, I previously forgot to factor out another chunk of code from profile_chooser_view. Patch 3 should contain everything now. Thanks!
Description was changed from ========== Refactored signin/sync error controllers for the avatar button The titlebar avatar button needs to display a warning icon when there is: 1. an auth error; or 2. a sync error, including unrecoverable errors, outdated client error, and sync passphrase error (MD user menu only). Its auth error controller and sync error controller previously reside in the UI code (new_avatar_button). In this CL, they are being factored out to be in avatar_signin_error_controller and avatar_sync_error_controller. Also, the logic in profile_chooser_view that was used to classify sync errors are factored out to be in sync_ui_util. They will be used on both Linux UI (this CL) and Mac UI (followup CL). BUG=615893 ========== to ========== Refactored signin/sync error controllers for the avatar button Background: The titlebar avatar button needs to display a warning icon when there is: 1. an auth error; or 2. a sync error, including unrecoverable errors, outdated client error, and sync passphrase error (MD user menu only). Its auth error controller and sync error controller previously resided in its UI code, new_avatar_button. Meanwhile, the MD user menu displays an error message and button for the same set of errors, and the logic for classifying each error previously resided in its UI code, profile_chooser_view. In this CL, the following refactoring was done: 1. sync error controller for avatar button: from new_avatar_button to avatar_sync_error_controller; 2. signin error controller for avatar button: from new_avatar_button to avatar_signin_error_controller; 3. the logic for classifying different errors: from profile_chooser_view to sync_ui_util. They will all be used on both Linux UI (this CL) and Mac UI (followup CL). BUG=615893 ==========
lgtm Nice refactor Jane. https://codereview.chromium.org/2179283002/diff/40001/chrome/browser/sync/syn... File chrome/browser/sync/sync_ui_util.h (right): https://codereview.chromium.org/2179283002/diff/40001/chrome/browser/sync/syn... chrome/browser/sync/sync_ui_util.h:40: }; Maybe make the name of this enum type mention "avatar" somehow?
janeliulwq@google.com changed reviewers: + sky@chromium.org, zea@chromium.org
janeliulwq@google.com changed required reviewers: + sky@chromium.org, zea@chromium.org
Thanks Roger! Hi zea@, PTAL at c/b/sync/; and sky@, c/b/ui/views/profiles please. Thanks! https://codereview.chromium.org/2179283002/diff/40001/chrome/browser/sync/syn... File chrome/browser/sync/sync_ui_util.h (right): https://codereview.chromium.org/2179283002/diff/40001/chrome/browser/sync/syn... chrome/browser/sync/sync_ui_util.h:40: }; On 2016/07/27 15:38:02, Roger Tawa wrote: > Maybe make the name of this enum type mention "avatar" somehow? Done.
https://codereview.chromium.org/2179283002/diff/60001/chrome/browser/signin/a... File chrome/browser/signin/avatar_signin_error_controller.h (right): https://codereview.chromium.org/2179283002/diff/60001/chrome/browser/signin/a... chrome/browser/signin/avatar_signin_error_controller.h:16: explicit AvatarSigninErrorController(Profile* profile); This class and usage of it would be more natural if it took a SigninErrorController::Observer. That way NewAvatarButton could pass itself as the SigninErrorController::Observer and directly extend SigninErrorController::Observer rather than creating helper classes. https://codereview.chromium.org/2179283002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/new_avatar_button.h (right): https://codereview.chromium.org/2179283002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.h:22: class AvatarSigninErrorObserver : public AvatarSigninErrorController { Does this and the next really need to be in the public section? Also, why do you need a separate class for these rather than making NewAvatarButton directly extend interfaces?
maxbogue@chromium.org changed reviewers: + maxbogue@chromium.org - zea@chromium.org
maxbogue@chromium.org changed required reviewers: + maxbogue@chromium.org - zea@chromium.org
zea@ just left for vacation; I'll review the sync code once sky@'s comments are addressed.
Thanks! https://codereview.chromium.org/2179283002/diff/60001/chrome/browser/signin/a... File chrome/browser/signin/avatar_signin_error_controller.h (right): https://codereview.chromium.org/2179283002/diff/60001/chrome/browser/signin/a... chrome/browser/signin/avatar_signin_error_controller.h:16: explicit AvatarSigninErrorController(Profile* profile); On 2016/07/27 18:17:30, sky wrote: > This class and usage of it would be more natural if it took a > SigninErrorController::Observer. That way NewAvatarButton could pass itself as > the SigninErrorController::Observer and directly extend > SigninErrorController::Observer rather than creating helper classes. The rationale of not having NewAvatarButton directly extending SigninErrorController::Observer and SyncErrorController::Observer is because both observers have OnErrorChange() and that would result in a bad-styled naming collision. That's why helper classes were created. Although, if AvatarSigninErrorController and AvatarSyncErrorController can keep their current implementation, I can add helper functions so that NewAvatarButton can directly extend AvatarSigninErrorController and AvatarSyncErrorController. The rationale of having this class inherit from SigninErrorController::Observer instead of taking one in is so that all the code for adding/removing observer and detecting error can be kept here and used across linux and mac (more so for avatar_sync_error_controller.cc). Apologies if I misunderstood your suggestion, but hopefully this could explain the current implementation. Let me know how you think this could be improved! https://codereview.chromium.org/2179283002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/new_avatar_button.h (right): https://codereview.chromium.org/2179283002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.h:22: class AvatarSigninErrorObserver : public AvatarSigninErrorController { On 2016/07/27 18:17:30, sky wrote: > Does this and the next really need to be in the public section? Also, why do you > need a separate class for these rather than making NewAvatarButton directly > extend interfaces? You're right, no need to be in the public section. Moved them to be private. For the second question, see reply to your other comment.
https://codereview.chromium.org/2179283002/diff/60001/chrome/browser/signin/a... File chrome/browser/signin/avatar_signin_error_controller.h (right): https://codereview.chromium.org/2179283002/diff/60001/chrome/browser/signin/a... chrome/browser/signin/avatar_signin_error_controller.h:16: explicit AvatarSigninErrorController(Profile* profile); On 2016/07/27 20:17:49, Jane wrote: > On 2016/07/27 18:17:30, sky wrote: > > This class and usage of it would be more natural if it took a > > SigninErrorController::Observer. That way NewAvatarButton could pass itself as > > the SigninErrorController::Observer and directly extend > > SigninErrorController::Observer rather than creating helper classes. > > The rationale of not having NewAvatarButton directly extending > SigninErrorController::Observer and SyncErrorController::Observer is because > both observers have OnErrorChange() and that would result in a bad-styled naming > collision. That's why helper classes were created. Although, if > AvatarSigninErrorController and AvatarSyncErrorController can keep their current > implementation, I can add helper functions so that NewAvatarButton can directly > extend AvatarSigninErrorController and AvatarSyncErrorController. > The rationale of having this class inherit from SigninErrorController::Observer > instead of taking one in is so that all the code for adding/removing observer > and detecting error can be kept here and used across linux and mac (more so for > avatar_sync_error_controller.cc). > Apologies if I misunderstood your suggestion, but hopefully this could explain > the current implementation. Let me know how you think this could be improved! I missed that the two classes are implementing different interfaces. It seems unfortunate that both SyncErrorController::Observer and SigninErrorController::Observer have the exact same function. I an not familiar with them, is there a reason the names aren't more specific, e.g. OnSyncErrorChanged and OnSigninErrorChanged? Can they change? It seems like the two classes you are creating, AvatarSigninErrorController and AvatarSigninErrorController are purely for the new avatar button. Is that right? If so, I recommend the following: . move the code to chrome/browser/ui . name it better to reflect what it is and used for. . combine the functionality into a single class that takes a pure virtual interface with the single method when the status changes. (if this class needs to use two classes because of the interfaces having the same function name, do that). . NewAvatarButton creates the single class and implements the pure virtual interface. Does that make sense? https://codereview.chromium.org/2179283002/diff/80001/chrome/browser/signin/a... File chrome/browser/signin/avatar_signin_error_controller.h (right): https://codereview.chromium.org/2179283002/diff/80001/chrome/browser/signin/a... chrome/browser/signin/avatar_signin_error_controller.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. nit: no '(c)', see style guide.
Patchset #6 (id:100001) has been deleted
https://codereview.chromium.org/2179283002/diff/60001/chrome/browser/signin/a... File chrome/browser/signin/avatar_signin_error_controller.h (right): https://codereview.chromium.org/2179283002/diff/60001/chrome/browser/signin/a... chrome/browser/signin/avatar_signin_error_controller.h:16: explicit AvatarSigninErrorController(Profile* profile); On 2016/07/27 21:42:54, sky wrote: > On 2016/07/27 20:17:49, Jane wrote: > > On 2016/07/27 18:17:30, sky wrote: > > > This class and usage of it would be more natural if it took a > > > SigninErrorController::Observer. That way NewAvatarButton could pass itself > as > > > the SigninErrorController::Observer and directly extend > > > SigninErrorController::Observer rather than creating helper classes. > > > > The rationale of not having NewAvatarButton directly extending > > SigninErrorController::Observer and SyncErrorController::Observer is because > > both observers have OnErrorChange() and that would result in a bad-styled > naming > > collision. That's why helper classes were created. Although, if > > AvatarSigninErrorController and AvatarSyncErrorController can keep their > current > > implementation, I can add helper functions so that NewAvatarButton can > directly > > extend AvatarSigninErrorController and AvatarSyncErrorController. > > The rationale of having this class inherit from > SigninErrorController::Observer > > instead of taking one in is so that all the code for adding/removing observer > > and detecting error can be kept here and used across linux and mac (more so > for > > avatar_sync_error_controller.cc). > > Apologies if I misunderstood your suggestion, but hopefully this could explain > > the current implementation. Let me know how you think this could be improved! > > I missed that the two classes are implementing different interfaces. It seems > unfortunate that both SyncErrorController::Observer and > SigninErrorController::Observer have the exact same function. I an not familiar > with them, is there a reason the names aren't more specific, e.g. > OnSyncErrorChanged and OnSigninErrorChanged? Can they change? > > It seems like the two classes you are creating, AvatarSigninErrorController and > AvatarSigninErrorController are purely for the new avatar button. Is that right? > If so, I recommend the following: > . move the code to chrome/browser/ui > . name it better to reflect what it is and used for. > . combine the functionality into a single class that takes a pure virtual > interface with the single method when the status changes. (if this class needs > to use two classes because of the interfaces having the same function name, do > that). > . NewAvatarButton creates the single class and implements the pure virtual > interface. > > Does that make sense? Yes, these two classes are for avatar button (linux and mac) only. There's no good reason why they need to have the more generic name, but when I first added sync errors to avatar button, we figured that changing the interface name would involve changing many other places, and it'd be easier to just use nested classes. Your suggestion makes a lot of sense. I made these changes: . moved the code to chrome/browser/ui/avatar_button_error_controller . the functionality is contained inside avatar_button_error_controller, and this class has two nested class that observes signin error and sync error respectively . NewAvatarButton directly inherits from the class Let me know if I missed anything and if this looks ok! https://codereview.chromium.org/2179283002/diff/80001/chrome/browser/signin/a... File chrome/browser/signin/avatar_signin_error_controller.h (right): https://codereview.chromium.org/2179283002/diff/80001/chrome/browser/signin/a... chrome/browser/signin/avatar_signin_error_controller.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/07/27 21:42:54, sky wrote: > nit: no '(c)', see style guide. Done.
https://codereview.chromium.org/2179283002/diff/120001/chrome/browser/ui/avat... File chrome/browser/ui/avatar_button_error_controller.h (right): https://codereview.chromium.org/2179283002/diff/120001/chrome/browser/ui/avat... chrome/browser/ui/avatar_button_error_controller.h:20: void UpdateSigninError(bool has_signin_error); The two Update functions are meant for internal use, right? They should be private. If you need to friend Avatar*ErrorController. https://codereview.chromium.org/2179283002/diff/120001/chrome/browser/ui/avat... chrome/browser/ui/avatar_button_error_controller.h:28: class AvatarSigninErrorController : public SigninErrorController::Observer { Please add description. Also, this class is an inner class so that the Avatar part of the name is redundant. In other words I suggest naming this SigninErrorControllerObserver and the the other SyncErrorControllerObserver, or just SigninErrorObserver and SyncErrorObserver, but it's up to you. https://codereview.chromium.org/2179283002/diff/120001/chrome/browser/ui/avat... chrome/browser/ui/avatar_button_error_controller.h:64: virtual void OnAvatarErrorChanged() = 0; Does this really work? OnAvatarErrorChanged is in the private section? None-the-less the pattern you have here is problematic for two reasons: 1. Style guide discourages multiple inheritance of anything but pure virtual classes, and this class is no longer pure virtual. 2. You don't really want NewAvatarButton to be a AvatarButtonErrorController, but rather have a AvatarButtonErrorController. In general try to favor composition over inheritance. To fix this make AvatarButtonErrorController have an AvatarButtonErrorControllerDelegate that has the single function OnAvatarErrorChanged(). NewAvatarButton then has a AvatarButtonErrorController and is a AvatarButtonErrorControllerDelegate. Sorry if I wasn't clear earlier, but this is what I meant when I said, "a single class that takes a pure virtual interface with the single method when the status changes."
Thanks! https://codereview.chromium.org/2179283002/diff/120001/chrome/browser/ui/avat... File chrome/browser/ui/avatar_button_error_controller.h (right): https://codereview.chromium.org/2179283002/diff/120001/chrome/browser/ui/avat... chrome/browser/ui/avatar_button_error_controller.h:20: void UpdateSigninError(bool has_signin_error); On 2016/08/01 15:55:26, sky wrote: > The two Update functions are meant for internal use, right? They should be > private. If you need to friend Avatar*ErrorController. Done. https://codereview.chromium.org/2179283002/diff/120001/chrome/browser/ui/avat... chrome/browser/ui/avatar_button_error_controller.h:28: class AvatarSigninErrorController : public SigninErrorController::Observer { On 2016/08/01 15:55:26, sky wrote: > Please add description. Also, this class is an inner class so that the Avatar > part of the name is redundant. In other words I suggest naming this > SigninErrorControllerObserver and the the other SyncErrorControllerObserver, or > just SigninErrorObserver and SyncErrorObserver, but it's up to you. Done. https://codereview.chromium.org/2179283002/diff/120001/chrome/browser/ui/avat... chrome/browser/ui/avatar_button_error_controller.h:64: virtual void OnAvatarErrorChanged() = 0; On 2016/08/01 15:55:26, sky wrote: > Does this really work? OnAvatarErrorChanged is in the private section? > None-the-less the pattern you have here is problematic for two reasons: > 1. Style guide discourages multiple inheritance of anything but pure virtual > classes, and this class is no longer pure virtual. > 2. You don't really want NewAvatarButton to be a AvatarButtonErrorController, > but rather have a AvatarButtonErrorController. In general try to favor > composition over inheritance. > > To fix this make AvatarButtonErrorController have an > AvatarButtonErrorControllerDelegate that has the single function > OnAvatarErrorChanged(). NewAvatarButton then has a AvatarButtonErrorController > and is a AvatarButtonErrorControllerDelegate. Sorry if I wasn't clear earlier, > but this is what I meant when I said, "a single class that takes a pure virtual > interface with the single method when the status changes." Ah, I see what you mean! Thanks for the detailed explanation! Refactored it this way, PTAL.
https://codereview.chromium.org/2179283002/diff/140001/chrome/browser/ui/avat... File chrome/browser/ui/avatar_button_error_controller.cc (right): https://codereview.chromium.org/2179283002/diff/140001/chrome/browser/ui/avat... chrome/browser/ui/avatar_button_error_controller.cc:22: void AvatarButtonErrorController::GetAvatarErrorUpdate() { Can you elaborate on why you need this function? AFAICT you only call this once, and shortly after the constructor. Can't the constructor do this? I suspect you have this function to delay calling to the delegate from the constructor. I would definitely avoid calling the delegate from the constructor, but having an explicit GetAvatarErrorUpdate() is easy to miss, resulting in an awkward api. My recommendation is to have the constructor update has_*_error_, but not call the delegate. The delegate is only called on changes after the constructor. NewAvatarButton checks HasAvatarError() from the constructor and does what ever it needs to do. https://codereview.chromium.org/2179283002/diff/140001/chrome/browser/ui/avat... chrome/browser/ui/avatar_button_error_controller.cc:29: delegate_->OnAvatarErrorChanged(); The public api for this class is effectively HasAvatarError(), you should call OnAvatarErrorChanged() only if HasAvatarError() has changed. https://codereview.chromium.org/2179283002/diff/140001/chrome/browser/ui/avat... File chrome/browser/ui/avatar_button_error_controller.h (right): https://codereview.chromium.org/2179283002/diff/140001/chrome/browser/ui/avat... chrome/browser/ui/avatar_button_error_controller.h:20: virtual ~AvatarButtonErrorController() {} You shouldn't need this to be virtual anymore, and don't inline the destructor. https://codereview.chromium.org/2179283002/diff/140001/chrome/browser/ui/avat... File chrome/browser/ui/avatar_button_error_controller_delegate.h (right): https://codereview.chromium.org/2179283002/diff/140001/chrome/browser/ui/avat... chrome/browser/ui/avatar_button_error_controller_delegate.h:15: }; Add a virtual destructor in the protected section to make it clear the controller doesn't own the delegate.
Thanks! https://codereview.chromium.org/2179283002/diff/140001/chrome/browser/ui/avat... File chrome/browser/ui/avatar_button_error_controller.cc (right): https://codereview.chromium.org/2179283002/diff/140001/chrome/browser/ui/avat... chrome/browser/ui/avatar_button_error_controller.cc:22: void AvatarButtonErrorController::GetAvatarErrorUpdate() { On 2016/08/01 22:59:40, sky wrote: > Can you elaborate on why you need this function? AFAICT you only call this once, > and shortly after the constructor. Can't the constructor do this? > > I suspect you have this function to delay calling to the delegate from the > constructor. I would definitely avoid calling the delegate from the constructor, > but having an explicit GetAvatarErrorUpdate() is easy to miss, resulting in an > awkward api. > > My recommendation is to have the constructor update has_*_error_, but not call > the delegate. The delegate is only called on changes after the constructor. > NewAvatarButton checks HasAvatarError() from the constructor and does what ever > it needs to do. Done. Makes sense! I refactored the actual error detection code into helper functions HasSigninError() and HasSyncError(), call them from AvatarButtonErrorController ctor, and have NewAvatarButton() check HasAvatarError() in its ctor. https://codereview.chromium.org/2179283002/diff/140001/chrome/browser/ui/avat... chrome/browser/ui/avatar_button_error_controller.cc:29: delegate_->OnAvatarErrorChanged(); On 2016/08/01 22:59:40, sky wrote: > The public api for this class is effectively HasAvatarError(), you should call > OnAvatarErrorChanged() only if HasAvatarError() has changed. Done. I added a check for HasAvatarError(). https://codereview.chromium.org/2179283002/diff/140001/chrome/browser/ui/avat... File chrome/browser/ui/avatar_button_error_controller.h (right): https://codereview.chromium.org/2179283002/diff/140001/chrome/browser/ui/avat... chrome/browser/ui/avatar_button_error_controller.h:20: virtual ~AvatarButtonErrorController() {} On 2016/08/01 22:59:40, sky wrote: > You shouldn't need this to be virtual anymore, and don't inline the destructor. Done. Removed virtual, and moved the empty dtor to .cc. https://codereview.chromium.org/2179283002/diff/140001/chrome/browser/ui/avat... File chrome/browser/ui/avatar_button_error_controller_delegate.h (right): https://codereview.chromium.org/2179283002/diff/140001/chrome/browser/ui/avat... chrome/browser/ui/avatar_button_error_controller_delegate.h:15: }; On 2016/08/01 22:59:40, sky wrote: > Add a virtual destructor in the protected section to make it clear the > controller doesn't own the delegate. Done. Is it ok to inline it here? Couldn't think of another way.
LGTM https://codereview.chromium.org/2179283002/diff/140001/chrome/browser/ui/avat... File chrome/browser/ui/avatar_button_error_controller_delegate.h (right): https://codereview.chromium.org/2179283002/diff/140001/chrome/browser/ui/avat... chrome/browser/ui/avatar_button_error_controller_delegate.h:15: }; On 2016/08/02 14:23:18, Jane wrote: > On 2016/08/01 22:59:40, sky wrote: > > Add a virtual destructor in the protected section to make it clear the > > controller doesn't own the delegate. > > Done. Is it ok to inline it here? Couldn't think of another way. Yes. We generally inline destructor for pure virtual classes. https://codereview.chromium.org/2179283002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2179283002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1501: default: default -> NO_ERROR. That way if a new type is added you get a compile error. Then you can remove the DCHECK to and change to NOTREACHED.
Thanks sky@! Hi maxbogue@, would you take a look at c/b/sync please (most of the code is a simple refactor out of profile_chooser_view). Thanks! https://codereview.chromium.org/2179283002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2179283002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1501: default: On 2016/08/02 19:34:40, sky wrote: > default -> NO_ERROR. That way if a new type is added you get a compile error. > Then you can remove the DCHECK to and change to NOTREACHED. Done.
//chrome/browser/sync lgtm
The CQ bit was checked by janeliulwq@google.com to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by janeliulwq@google.com to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by janeliulwq@google.com to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by janeliulwq@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from rogerta@chromium.org, sky@chromium.org, maxbogue@chromium.org Link to the patchset: https://codereview.chromium.org/2179283002/#ps220001 (title: "Uninitialized ptr")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Refactored signin/sync error controllers for the avatar button Background: The titlebar avatar button needs to display a warning icon when there is: 1. an auth error; or 2. a sync error, including unrecoverable errors, outdated client error, and sync passphrase error (MD user menu only). Its auth error controller and sync error controller previously resided in its UI code, new_avatar_button. Meanwhile, the MD user menu displays an error message and button for the same set of errors, and the logic for classifying each error previously resided in its UI code, profile_chooser_view. In this CL, the following refactoring was done: 1. sync error controller for avatar button: from new_avatar_button to avatar_sync_error_controller; 2. signin error controller for avatar button: from new_avatar_button to avatar_signin_error_controller; 3. the logic for classifying different errors: from profile_chooser_view to sync_ui_util. They will all be used on both Linux UI (this CL) and Mac UI (followup CL). BUG=615893 ========== to ========== Refactored signin/sync error controllers for the avatar button Background: The titlebar avatar button needs to display a warning icon when there is: 1. an auth error; or 2. a sync error, including unrecoverable errors, outdated client error, and sync passphrase error (MD user menu only). Its auth error controller and sync error controller previously resided in its UI code, new_avatar_button. Meanwhile, the MD user menu displays an error message and button for the same set of errors, and the logic for classifying each error previously resided in its UI code, profile_chooser_view. In this CL, the following refactoring was done: 1. sync error controller for avatar button: from new_avatar_button to avatar_sync_error_controller; 2. signin error controller for avatar button: from new_avatar_button to avatar_signin_error_controller; 3. the logic for classifying different errors: from profile_chooser_view to sync_ui_util. They will all be used on both Linux UI (this CL) and Mac UI (followup CL). BUG=615893 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Refactored signin/sync error controllers for the avatar button Background: The titlebar avatar button needs to display a warning icon when there is: 1. an auth error; or 2. a sync error, including unrecoverable errors, outdated client error, and sync passphrase error (MD user menu only). Its auth error controller and sync error controller previously resided in its UI code, new_avatar_button. Meanwhile, the MD user menu displays an error message and button for the same set of errors, and the logic for classifying each error previously resided in its UI code, profile_chooser_view. In this CL, the following refactoring was done: 1. sync error controller for avatar button: from new_avatar_button to avatar_sync_error_controller; 2. signin error controller for avatar button: from new_avatar_button to avatar_signin_error_controller; 3. the logic for classifying different errors: from profile_chooser_view to sync_ui_util. They will all be used on both Linux UI (this CL) and Mac UI (followup CL). BUG=615893 ========== to ========== Refactored signin/sync error controllers for the avatar button Background: The titlebar avatar button needs to display a warning icon when there is: 1. an auth error; or 2. a sync error, including unrecoverable errors, outdated client error, and sync passphrase error (MD user menu only). Its auth error controller and sync error controller previously resided in its UI code, new_avatar_button. Meanwhile, the MD user menu displays an error message and button for the same set of errors, and the logic for classifying each error previously resided in its UI code, profile_chooser_view. In this CL, the following refactoring was done: 1. sync error controller for avatar button: from new_avatar_button to avatar_sync_error_controller; 2. signin error controller for avatar button: from new_avatar_button to avatar_signin_error_controller; 3. the logic for classifying different errors: from profile_chooser_view to sync_ui_util. They will all be used on both Linux UI (this CL) and Mac UI (followup CL). BUG=615893 Committed: https://crrev.com/e4525c4a250b0d405abaecd6ba9bae7e46bfdda3 Cr-Commit-Position: refs/heads/master@{#409439} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/e4525c4a250b0d405abaecd6ba9bae7e46bfdda3 Cr-Commit-Position: refs/heads/master@{#409439} |
