|
|
Descriptionviews: 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 #
Messages
Total messages: 23 (8 generated)
danakj@chromium.org changed reviewers: + sky@chromium.org
danakj@chromium.org changed reviewers: + dzhioev@chromium.org
+dzhioev
This is a question for dzhioev..
pkotwicz pointed me the right way here, you can get a "placeholder" LogoutButton by clicking on the user in the tray menu when running with --enable-account-consistency I confirmed that with and without this patch, the user's name text gets elipsized the same way, the invisible button still takes up space.
On 2015/03/24 20:58:06, danakj wrote: > pkotwicz pointed me the right way here, you can get a "placeholder" LogoutButton > by clicking on the user in the tray menu when running with > --enable-account-consistency > > I confirmed that with and without this patch, the user's name text gets > elipsized the same way, the invisible button still takes up space. LGTM
Thanks, Stefan can you give owners review?
skuhne@chromium.org changed reviewers: + skuhne@chromium.org
lgtm
The CQ bit was checked by danakj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1037443003/1
On 2015/03/24 22:52:18, Mr4D wrote: > lgtm Couldn't we get rid of the button altogether?
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.
The CQ bit was unchecked by danakj@chromium.org
The CQ bit was checked by danakj@chromium.org
The CQ bit was unchecked by danakj@chromium.org
The CQ bit was checked by danakj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1037443003/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/1d5a4c807e011a38ec9a2aa84ce66c1cbfe52be1 Cr-Commit-Position: refs/heads/master@{#322093}
Message was sent while issue was closed.
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.
Message was sent while issue was closed.
On Tue, Mar 24, 2015 at 4:50 PM, <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 chromium-reviews+unsubscribe@chromium.org.
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. |