Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(39)

Issue 1037443003: views: Make the LogoutButton not visible instead of early out in Paint (Closed)

Created:
5 years, 9 months ago by danakj
Modified:
5 years, 9 months ago
CC:
chromium-reviews, kalyank, sadrul, enne (OOO), piman, sky
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

views: Make the LogoutButton not visible instead of early out in Paint The button is still used for layout, the size of the user name text remains resized according to the button, but this way we don't have to override the Paint() function which helps us make Paint() no longer virtual. R=pkotwicz, skuhne,sky BUG=466426 Committed: https://crrev.com/1d5a4c807e011a38ec9a2aa84ce66c1cbfe52be1 Cr-Commit-Position: refs/heads/master@{#322093}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1 line, -7 lines) Patch
M ash/system/user/user_view.cc View 1 chunk +1 line, -7 lines 0 comments Download

Messages

Total messages: 23 (8 generated)
danakj
+dzhioev
5 years, 9 months ago (2015-03-24 20:37:39 UTC) #3
Mr4D (OOO till 08-26)
This is a question for dzhioev..
5 years, 9 months ago (2015-03-24 20:42:59 UTC) #4
danakj
pkotwicz pointed me the right way here, you can get a "placeholder" LogoutButton by clicking ...
5 years, 9 months ago (2015-03-24 20:58:06 UTC) #5
dzhioev (left Google)
On 2015/03/24 20:58:06, danakj wrote: > pkotwicz pointed me the right way here, you can ...
5 years, 9 months ago (2015-03-24 21:29:51 UTC) #6
danakj
Thanks, Stefan can you give owners review?
5 years, 9 months ago (2015-03-24 21:38:23 UTC) #7
Mr4D (OOO till 08-26)
lgtm
5 years, 9 months ago (2015-03-24 22:52:18 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1037443003/1
5 years, 9 months ago (2015-03-24 22:53:49 UTC) #11
Mr4D (OOO till 08-26)
On 2015/03/24 22:52:18, Mr4D wrote: > lgtm Couldn't we get rid of the button altogether?
5 years, 9 months ago (2015-03-24 22:56:23 UTC) #12
danakj
On 2015/03/24 22:56:23, Mr4D wrote: > On 2015/03/24 22:52:18, Mr4D wrote: > > lgtm > ...
5 years, 9 months ago (2015-03-24 23:18:51 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1037443003/1
5 years, 9 months ago (2015-03-24 23:33:03 UTC) #18
commit-bot: I haz the power
Committed patchset #1 (id:1)
5 years, 9 months ago (2015-03-24 23:48:50 UTC) #19
commit-bot: I haz the power
Patchset 1 (id:??) landed as https://crrev.com/1d5a4c807e011a38ec9a2aa84ce66c1cbfe52be1 Cr-Commit-Position: refs/heads/master@{#322093}
5 years, 9 months ago (2015-03-24 23:49:35 UTC) #20
Mr4D (OOO till 08-26)
On 2015/03/24 23:18:51, danakj wrote: > On 2015/03/24 22:56:23, Mr4D wrote: > > On 2015/03/24 ...
5 years, 9 months ago (2015-03-24 23:50:34 UTC) #21
danakj
On Tue, Mar 24, 2015 at 4:50 PM, <skuhne@chromium.org> wrote: > On 2015/03/24 23:18:51, danakj ...
5 years, 9 months ago (2015-03-24 23:54:09 UTC) #22
Mr4D (OOO till 08-26)
5 years, 9 months ago (2015-03-24 23:58:01 UTC) #23
Message was sent while issue was closed.
On 2015/03/24 23:54:09, danakj wrote:
> On Tue, Mar 24, 2015 at 4:50 PM, <mailto:skuhne@chromium.org> wrote:
> 
> > On 2015/03/24 23:18:51, danakj wrote:
> >
> >> On 2015/03/24 22:56:23, Mr4D wrote:
> >> > On 2015/03/24 22:52:18, Mr4D wrote:
> >> > > lgtm
> >> >
> >> > Couldn't we get rid of the button altogether?
> >>
> >
> >  It's being used for empty space padding. Please do it differently if you
> >> want,
> >> but that's a bit orthogonal to eliminating this virtual method on View.
> >>
> >
> > Not quite sure I follow. Looking at the remainder of the class there isn't
> > much
> > left which justifies having one.
> >
> > The presense of the view makes an empty space the size of the sign out
> button. Without it, you don't get an empty space, text will run into it.
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

I thought you could have simply used the "TrayPopupLabelButton" instead of this
other class since the LogoutButton is at this point essentially a
"TrayPopupLabelButton". But then again, the patch has landed already anyways.

Powered by Google App Engine
This is Rietveld 408576698