Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
58183002:
Auto refresh avatar bubble after adding a secondary account (Closed)
|
Created:
7 years, 1 month ago by guohui Modified:
7 years, 1 month ago CC:
chromium-reviews, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAuto refresh avatar bubble after adding a secondary account
BUG=
R=rogerta@chromium.org, sky@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233306
Patch Set 1 #
Total comments: 4
Patch Set 2 : roger comments addressed #
Total comments: 4
Patch Set 3 : move initialization of view_mode_ to constructor #
Messages
Total messages: 15 (0 generated)
Hey, could you please take a look at the CL? Thanks, Hui
lgtm https://codereview.chromium.org/58183002/diff/1/chrome/browser/ui/views/profi... File chrome/browser/ui/views/profile_chooser_view.cc (right): https://codereview.chromium.org/58183002/diff/1/chrome/browser/ui/views/profi... chrome/browser/ui/views/profile_chooser_view.cc:224: browser_(browser) { Initialize view_mode_ here? https://codereview.chromium.org/58183002/diff/1/chrome/browser/ui/views/profi... chrome/browser/ui/views/profile_chooser_view.cc:280: ShowView(ACCOUNT_MANAGEMENT_VIEW, avatar_menu_.get()); Should add { and } around if block.
https://codereview.chromium.org/58183002/diff/1/chrome/browser/ui/views/profi... File chrome/browser/ui/views/profile_chooser_view.cc (right): https://codereview.chromium.org/58183002/diff/1/chrome/browser/ui/views/profi... chrome/browser/ui/views/profile_chooser_view.cc:224: browser_(browser) { On 2013/11/04 19:13:14, Roger Tawa wrote: > Initialize view_mode_ here? Done. https://codereview.chromium.org/58183002/diff/1/chrome/browser/ui/views/profi... chrome/browser/ui/views/profile_chooser_view.cc:280: ShowView(ACCOUNT_MANAGEMENT_VIEW, avatar_menu_.get()); On 2013/11/04 19:13:14, Roger Tawa wrote: > Should add { and } around if block. Done.
https://codereview.chromium.org/58183002/diff/80001/chrome/browser/ui/views/p... File chrome/browser/ui/views/profile_chooser_view.cc (right): https://codereview.chromium.org/58183002/diff/80001/chrome/browser/ui/views/p... chrome/browser/ui/views/profile_chooser_view.cc:259: view_mode_ = PROFILE_CHOOSER_VIEW; Is there a reason this is here not the member initializer list?
https://codereview.chromium.org/58183002/diff/80001/chrome/browser/ui/views/p... File chrome/browser/ui/views/profile_chooser_view.cc (right): https://codereview.chromium.org/58183002/diff/80001/chrome/browser/ui/views/p... chrome/browser/ui/views/profile_chooser_view.cc:259: view_mode_ = PROFILE_CHOOSER_VIEW; On 2013/11/04 21:19:33, sky wrote: > Is there a reason this is here not the member initializer list? Because this is the current central place to initialize and reset all instance members with default values. The method is called from constructor.
https://codereview.chromium.org/58183002/diff/80001/chrome/browser/ui/views/p... File chrome/browser/ui/views/profile_chooser_view.cc (right): https://codereview.chromium.org/58183002/diff/80001/chrome/browser/ui/views/p... chrome/browser/ui/views/profile_chooser_view.cc:259: view_mode_ = PROFILE_CHOOSER_VIEW; On 2013/11/05 14:40:01, guohui wrote: > On 2013/11/04 21:19:33, sky wrote: > > Is there a reason this is here not the member initializer list? > > Because this is the current central place to initialize and reset all instance > members with default values. The method is called from constructor. For view_mode_ don't you only want it reset to the default from the constructor? The one other place that invokes ResetLinksAndButtons() immediately resets view_mode_.
https://codereview.chromium.org/58183002/diff/80001/chrome/browser/ui/views/p... File chrome/browser/ui/views/profile_chooser_view.cc (right): https://codereview.chromium.org/58183002/diff/80001/chrome/browser/ui/views/p... chrome/browser/ui/views/profile_chooser_view.cc:259: view_mode_ = PROFILE_CHOOSER_VIEW; On 2013/11/05 15:53:50, sky wrote: > On 2013/11/05 14:40:01, guohui wrote: > > On 2013/11/04 21:19:33, sky wrote: > > > Is there a reason this is here not the member initializer list? > > > > Because this is the current central place to initialize and reset all instance > > members with default values. The method is called from constructor. > > For view_mode_ don't you only want it reset to the default from the constructor? > The one other place that invokes ResetLinksAndButtons() immediately resets > view_mode_. it is the same as all other members, first reset to default, and then assigned to a proper value based on the selected view. But i really don't have strong preference on this, so i moved it to initializer in a new patch.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/guohui@chromium.org/58183002/140002
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/guohui@chromium.org/58183002/140002
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/guohui@chromium.org/58183002/140002
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Message was sent while issue was closed.
Committed patchset #3 manually as r233306 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
