Update global error bubble view's boundary (position and size) after loading elevation icon.

Update global error bubble view's boundary (position and size) after loading elevation icon.

BUG=454908
Committed: https://crrev.com/ef78449d76dc0a41f25dc7fc859967057f1b5541
Cr-Commit-Position: refs/heads/master@{#317696}

[xiaoling, 2015-02-21 00:17:22]

xiaolingbao@chromium.org changed reviewers:
+ pkasting@chromium.org

[xiaoling, 2015-02-21 00:19:33]

Peter,

Can you please take a look whether this is the correct way to fix the button
size issue after loading the elevation icon?

[Peter Kasting, 2015-02-21 01:06:27]

Seems like all three places that call this setter should probably trigger a
re-layout once it finishes.

https://codereview.chromium.org/941133002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/global_error_bubble_view.cc (right):

https://codereview.chromium.org/941133002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/global_error_bubble_view.cc:108:
base::Bind(&GlobalErrorBubbleView::UpdateBound, AsWeakPtr())));
Why not just pass &views::BubbleDelegateView::SizeToContents() directly and
avoid adding the UpdateBound() function?

[xiaoling, 2015-02-21 01:14:52]

On 2015/02/21 01:06:27, Peter Kasting wrote:
> Seems like all three places that call this setter should probably trigger a
> re-layout once it finishes.
Yes. I didn't add it now since I want to consult your opinions first. Also, the
issue is not that severe for the other two places at the moment (if other ui
elements make parent size large enough).
> 
>
https://codereview.chromium.org/941133002/diff/20001/chrome/browser/ui/views/...
> File chrome/browser/ui/views/global_error_bubble_view.cc (right):
> 
>
https://codereview.chromium.org/941133002/diff/20001/chrome/browser/ui/views/...
> chrome/browser/ui/views/global_error_bubble_view.cc:108:
> base::Bind(&GlobalErrorBubbleView::UpdateBound, AsWeakPtr())));
> Why not just pass &views::BubbleDelegateView::SizeToContents() directly and
> avoid adding the UpdateBound() function?

[xiaoling, 2015-02-21 01:15:00]

https://codereview.chromium.org/941133002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/global_error_bubble_view.cc (right):

https://codereview.chromium.org/941133002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/global_error_bubble_view.cc:108:
base::Bind(&GlobalErrorBubbleView::UpdateBound, AsWeakPtr())));
On 2015/02/21 01:06:27, Peter Kasting wrote:
> Why not just pass &views::BubbleDelegateView::SizeToContents() directly and
> avoid adding the UpdateBound() function?

Because the function is the protected. Is there any trick to get around that?

[Peter Kasting, 2015-02-21 01:21:49]

https://codereview.chromium.org/941133002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/global_error_bubble_view.cc (right):

https://codereview.chromium.org/941133002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/global_error_bubble_view.cc:108:
base::Bind(&GlobalErrorBubbleView::UpdateBound, AsWeakPtr())));
On 2015/02/21 01:15:00, xiaoling wrote:
> On 2015/02/21 01:06:27, Peter Kasting wrote:
> > Why not just pass &views::BubbleDelegateView::SizeToContents() directly and
> > avoid adding the UpdateBound() function?
> 
> Because the function is the protected. Is there any trick to get around that?

Sorry, try passing &GlobalErrorBubbleView::SizeToContents instead.  That ought
to work.

[xiaoling, 2015-02-23 18:19:41]

thank you.

https://codereview.chromium.org/941133002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/global_error_bubble_view.cc (right):

https://codereview.chromium.org/941133002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/global_error_bubble_view.cc:108:
base::Bind(&GlobalErrorBubbleView::UpdateBound, AsWeakPtr())));
On 2015/02/21 01:21:49, Peter Kasting wrote:
> On 2015/02/21 01:15:00, xiaoling wrote:
> > On 2015/02/21 01:06:27, Peter Kasting wrote:
> > > Why not just pass &views::BubbleDelegateView::SizeToContents() directly
and
> > > avoid adding the UpdateBound() function?
> > 
> > Because the function is the protected. Is there any trick to get around
that?
> 
> Sorry, try passing &GlobalErrorBubbleView::SizeToContents instead.  That ought
> to work.

Thanks for the trick. It worked, although I'm not clear why.

[Peter Kasting, 2015-02-23 21:26:48]

https://codereview.chromium.org/941133002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/global_error_bubble_view.h (right):

https://codereview.chromium.org/941133002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/global_error_bubble_view.h:20: public
base::SupportsWeakPtr<GlobalErrorBubbleView>,
There's no need for any of the classes in this CL to subclass SupportsWeakPtr
and bind the closures to weak pointers.  These classes own the
ElevationIconSetters, so if they're destroyed, the setters will also be
destroyed and can't call back.  (That's why ElevationIconSetter uses a weak
pointer internally for thread-hopping.)  So you can bind all these as
Unretained.

https://codereview.chromium.org/941133002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/infobars/confirm_infobar.cc (right):

https://codereview.chromium.org/941133002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/infobars/confirm_infobar.cc:76:
base::Bind(&ConfirmInfoBar::InvalidateLayout, AsWeakPtr())));
I would bind to Layout() rather than InvalidateLayout().

[xiaoling, 2015-02-23 21:50:04]

Thanks for the feedback. PTAL.

https://codereview.chromium.org/941133002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/global_error_bubble_view.h (right):

https://codereview.chromium.org/941133002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/global_error_bubble_view.h:20: public
base::SupportsWeakPtr<GlobalErrorBubbleView>,
On 2015/02/23 21:26:48, Peter Kasting wrote:
> There's no need for any of the classes in this CL to subclass SupportsWeakPtr
> and bind the closures to weak pointers.  These classes own the
> ElevationIconSetters, so if they're destroyed, the setters will also be
> destroyed and can't call back.  (That's why ElevationIconSetter uses a weak
> pointer internally for thread-hopping.)  So you can bind all these as
> Unretained.

Done.

https://codereview.chromium.org/941133002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/infobars/confirm_infobar.cc (right):

https://codereview.chromium.org/941133002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/infobars/confirm_infobar.cc:76:
base::Bind(&ConfirmInfoBar::InvalidateLayout, AsWeakPtr())));
On 2015/02/23 21:26:48, Peter Kasting wrote:
> I would bind to Layout() rather than InvalidateLayout().

Done.

[Peter Kasting, 2015-02-23 21:58:35]

LGTM

https://codereview.chromium.org/941133002/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/elevation_icon_setter.h (right):

https://codereview.chromium.org/941133002/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/elevation_icon_setter.h:24: // |done_cb| will be called
once the icon is set to the button.
Nit: How about this (see next comment regarding arg name):

|callback| will be called if the button icon is actually changed; callers should
pass a function which does a relayout on the view containing the button, to
ensure the button is correctly resized as necessary.

https://codereview.chromium.org/941133002/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/elevation_icon_setter.h:25:
ElevationIconSetter(views::LabelButton* button, const base::Closure& done_cb);
Nit: The header and .cc disagree over the name of the second arg, and "cb"
arguably violates the Google style guide proscription against abbreviation.

I would just change all places in both files to use |callback| as the name.

[xiaoling, 2015-02-23 22:05:07]

thank you!

https://codereview.chromium.org/941133002/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/elevation_icon_setter.h (right):

https://codereview.chromium.org/941133002/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/elevation_icon_setter.h:24: // |done_cb| will be called
once the icon is set to the button.
On 2015/02/23 21:58:35, Peter Kasting wrote:
> Nit: How about this (see next comment regarding arg name):
> 
> |callback| will be called if the button icon is actually changed; callers
should
> pass a function which does a relayout on the view containing the button, to
> ensure the button is correctly resized as necessary.

Done.

https://codereview.chromium.org/941133002/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/elevation_icon_setter.h:25:
ElevationIconSetter(views::LabelButton* button, const base::Closure& done_cb);
On 2015/02/23 21:58:35, Peter Kasting wrote:
> Nit: The header and .cc disagree over the name of the second arg, and "cb"
> arguably violates the Google style guide proscription against abbreviation.
> 
> I would just change all places in both files to use |callback| as the name.

Done.

[xiaoling, 2015-02-23 22:05:11]

The CQ bit was checked by xiaolingbao@chromium.org

[xiaoling, 2015-02-23 22:05:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/941133002/#ps120001
(title: " ")

[commit-bot: I haz the power, 2015-02-23 22:05:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/941133002/120001

[commit-bot: I haz the power, 2015-02-23 23:14:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-23 23:14:29]

Failed to apply the patch.

[commit-bot: I haz the power, 2015-02-23 23:15:29]

The CQ bit was unchecked by commit-bot@chromium.org

[xiaoling, 2015-02-23 23:30:21]

The CQ bit was checked by xiaolingbao@chromium.org

[xiaoling, 2015-02-23 23:30:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/941133002/#ps140001
(title: " ")

[commit-bot: I haz the power, 2015-02-23 23:32:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/941133002/140001

[commit-bot: I haz the power, 2015-02-24 00:47:25]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-02-24 00:47:58]

Patchset 8 (id:??) landed as
https://crrev.com/ef78449d76dc0a41f25dc7fc859967057f1b5541
Cr-Commit-Position: refs/heads/master@{#317696}