Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
326963002:
[Views] Add a capability to the bubble delegate to be able to show the title in BaseFont. (Closed)
|
Created:
6 years, 6 months ago by Greg Billock Modified:
6 years, 6 months ago CC:
chromium-reviews, msw+watch_chromium.org, alicet1, tfarina, yao Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Views] Add a capability to the bubble delegate to be able to set the font of the title.
BUG=383947
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278045
Patch Set 1 #
Total comments: 2
Patch Set 2 : Change to set the fontlist for the title #Patch Set 3 : . #
Total comments: 2
Patch Set 4 : Change title font method to protected #Patch Set 5 : Rebase #Patch Set 6 : . #
Messages
Total messages: 21 (0 generated)
msw, can you have a look? This is for the permissions bubble -- we want the UI to have a BaseFont title and content, to be similar to the other bubbles.
Can you set the BUG= and point me to mocks? It seems odd that we'd want these bubbles to use a different title font than other bubbles. Are you really sure that's the intent? (perhaps UX didn't mean to suggest a unique font, or perhaps they want all bubble titles to be in the base font, instead of just the permissions bubbles). https://codereview.chromium.org/326963002/diff/1/ui/views/bubble/bubble_deleg... File ui/views/bubble/bubble_delegate.h (right): https://codereview.chromium.org/326963002/diff/1/ui/views/bubble/bubble_deleg... ui/views/bubble/bubble_delegate.h:54: virtual bool ShouldShowWindowTitleInBaseFont() const; How about GetTitleFontList? https://codereview.chromium.org/326963002/diff/1/ui/views/bubble/bubble_frame... File ui/views/bubble/bubble_frame_view.h (right): https://codereview.chromium.org/326963002/diff/1/ui/views/bubble/bubble_frame... ui/views/bubble/bubble_frame_view.h:47: void ShowTitleInBaseFont(); How about SetTitleFontList?
On 2014/06/10 18:16:05, msw wrote: > Can you set the BUG= and point me to mocks? It seems odd that we'd want these > bubbles to use a different title font than other bubbles. Are you really sure > that's the intent? (perhaps UX didn't mean to suggest a unique font, or perhaps > they want all bubble titles to be in the base font, instead of just the > permissions bubbles). > > https://codereview.chromium.org/326963002/diff/1/ui/views/bubble/bubble_deleg... > File ui/views/bubble/bubble_delegate.h (right): > > https://codereview.chromium.org/326963002/diff/1/ui/views/bubble/bubble_deleg... > ui/views/bubble/bubble_delegate.h:54: virtual bool > ShouldShowWindowTitleInBaseFont() const; > How about GetTitleFontList? > > https://codereview.chromium.org/326963002/diff/1/ui/views/bubble/bubble_frame... > File ui/views/bubble/bubble_frame_view.h (right): > > https://codereview.chromium.org/326963002/diff/1/ui/views/bubble/bubble_frame... > ui/views/bubble/bubble_frame_view.h:47: void ShowTitleInBaseFont(); > How about SetTitleFontList? Ruby, can you comment on the font sizing in bubbles thing? wrt using FontList, I went through various stages of grief in realizing that I can't really see the title from the bubble delegate to set the font list on it directly, to trying to figure out how to provide the font list directly (it'd take two methods, one for "should use" and the other for "use this", but then it'd likely require awkward defaults as well. In the end I just gave up and put in exactly the narrow hole I wanted, but it's not really ideal. I think the big problem is that "title" for a bubble doesn't really mean quite the same thing as "title" for other widgets, where it'd be ridiculous to have font settings, so it's awkward. I thought about trying to attack that directly, but ultimately shrank away in cowardly fear.
On 2014/06/10 18:34:40, Greg Billock wrote: > On 2014/06/10 18:16:05, msw wrote: > > Can you set the BUG= and point me to mocks? It seems odd that we'd want these > > bubbles to use a different title font than other bubbles. Are you really sure > > that's the intent? (perhaps UX didn't mean to suggest a unique font, or > perhaps > > they want all bubble titles to be in the base font, instead of just the > > permissions bubbles). > > > > > https://codereview.chromium.org/326963002/diff/1/ui/views/bubble/bubble_deleg... > > File ui/views/bubble/bubble_delegate.h (right): > > > > > https://codereview.chromium.org/326963002/diff/1/ui/views/bubble/bubble_deleg... > > ui/views/bubble/bubble_delegate.h:54: virtual bool > > ShouldShowWindowTitleInBaseFont() const; > > How about GetTitleFontList? > > > > > https://codereview.chromium.org/326963002/diff/1/ui/views/bubble/bubble_frame... > > File ui/views/bubble/bubble_frame_view.h (right): > > > > > https://codereview.chromium.org/326963002/diff/1/ui/views/bubble/bubble_frame... > > ui/views/bubble/bubble_frame_view.h:47: void ShowTitleInBaseFont(); > > How about SetTitleFontList? > > Ruby, can you comment on the font sizing in bubbles thing? > > wrt using FontList, I went through various stages of grief in realizing that I > can't really see the title from the bubble delegate to set the font list on it > directly, to trying to figure out how to provide the font list directly (it'd > take two methods, one for "should use" and the other for "use this", but then > it'd likely require awkward defaults as well. In the end I just gave up and put > in exactly the narrow hole I wanted, but it's not really ideal. I think the big > problem is that "title" for a bubble doesn't really mean quite the same thing as > "title" for other widgets, where it'd be ridiculous to have font settings, so > it's awkward. I thought about trying to attack that directly, but ultimately > shrank away in cowardly fear. The goal here is to use the same font as the translate bubble on Windows (you can see the translate bubble by going to www.worldjournal.com). However, the translate bubble has no close control (X) or title, so when we try to reuse its formatting, we end up with a top margin that is too wide. In order to avoid the top margin, we have to use a title. The title isn't really a title though - we want the title to be the same font as the rest of the bubble. Here's a bug with mocks attached: crbug.com/383947
On 2014/06/12 18:06:08, rubylee wrote: > On 2014/06/10 18:34:40, Greg Billock wrote: > > On 2014/06/10 18:16:05, msw wrote: > > > Can you set the BUG= and point me to mocks? It seems odd that we'd want > these > > > bubbles to use a different title font than other bubbles. Are you really > sure > > > that's the intent? (perhaps UX didn't mean to suggest a unique font, or > > perhaps > > > they want all bubble titles to be in the base font, instead of just the > > > permissions bubbles). > > > > > > > > > https://codereview.chromium.org/326963002/diff/1/ui/views/bubble/bubble_deleg... > > > File ui/views/bubble/bubble_delegate.h (right): > > > > > > > > > https://codereview.chromium.org/326963002/diff/1/ui/views/bubble/bubble_deleg... > > > ui/views/bubble/bubble_delegate.h:54: virtual bool > > > ShouldShowWindowTitleInBaseFont() const; > > > How about GetTitleFontList? > > > > > > > > > https://codereview.chromium.org/326963002/diff/1/ui/views/bubble/bubble_frame... > > > File ui/views/bubble/bubble_frame_view.h (right): > > > > > > > > > https://codereview.chromium.org/326963002/diff/1/ui/views/bubble/bubble_frame... > > > ui/views/bubble/bubble_frame_view.h:47: void ShowTitleInBaseFont(); > > > How about SetTitleFontList? > > > > Ruby, can you comment on the font sizing in bubbles thing? > > > > wrt using FontList, I went through various stages of grief in realizing that I > > can't really see the title from the bubble delegate to set the font list on it > > directly, to trying to figure out how to provide the font list directly (it'd > > take two methods, one for "should use" and the other for "use this", but then > > it'd likely require awkward defaults as well. In the end I just gave up and > put > > in exactly the narrow hole I wanted, but it's not really ideal. I think the > big > > problem is that "title" for a bubble doesn't really mean quite the same thing > as > > "title" for other widgets, where it'd be ridiculous to have font settings, so > > it's awkward. I thought about trying to attack that directly, but ultimately > > shrank away in cowardly fear. > > The goal here is to use the same font as the translate bubble on Windows (you > can see the translate bubble by going to http://www.worldjournal.com). However, the > translate bubble has no close control (X) or title, so when we try to reuse its > formatting, we end up with a top margin that is too wide. In order to avoid the > top margin, we have to use a title. The title isn't really a title though - we > want the title to be the same font as the rest of the bubble. Here's a bug with > mocks attached: crbug.com/383947 msw, any further thoughts on this? If there's a better way to do this in the views bubble, I'm up for it, but I hunted around and couldn't find it. I know the tab-modal had this same limitation at one point, but I don't think bubbles got any workaround.
On 2014/06/16 15:09:23, Greg Billock wrote: > On 2014/06/12 18:06:08, rubylee wrote: > > On 2014/06/10 18:34:40, Greg Billock wrote: > > > On 2014/06/10 18:16:05, msw wrote: > > > > Can you set the BUG= and point me to mocks? It seems odd that we'd want > > these > > > > bubbles to use a different title font than other bubbles. Are you really > > sure > > > > that's the intent? (perhaps UX didn't mean to suggest a unique font, or > > > perhaps > > > > they want all bubble titles to be in the base font, instead of just the > > > > permissions bubbles). > > > > > > > > > > > > > > https://codereview.chromium.org/326963002/diff/1/ui/views/bubble/bubble_deleg... > > > > File ui/views/bubble/bubble_delegate.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/326963002/diff/1/ui/views/bubble/bubble_deleg... > > > > ui/views/bubble/bubble_delegate.h:54: virtual bool > > > > ShouldShowWindowTitleInBaseFont() const; > > > > How about GetTitleFontList? > > > > > > > > > > > > > > https://codereview.chromium.org/326963002/diff/1/ui/views/bubble/bubble_frame... > > > > File ui/views/bubble/bubble_frame_view.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/326963002/diff/1/ui/views/bubble/bubble_frame... > > > > ui/views/bubble/bubble_frame_view.h:47: void ShowTitleInBaseFont(); > > > > How about SetTitleFontList? > > > > > > Ruby, can you comment on the font sizing in bubbles thing? > > > > > > wrt using FontList, I went through various stages of grief in realizing that > I > > > can't really see the title from the bubble delegate to set the font list on > it > > > directly, to trying to figure out how to provide the font list directly > (it'd > > > take two methods, one for "should use" and the other for "use this", but > then > > > it'd likely require awkward defaults as well. In the end I just gave up and > > put > > > in exactly the narrow hole I wanted, but it's not really ideal. I think the > > big > > > problem is that "title" for a bubble doesn't really mean quite the same > thing > > as > > > "title" for other widgets, where it'd be ridiculous to have font settings, > so > > > it's awkward. I thought about trying to attack that directly, but ultimately > > > shrank away in cowardly fear. > > > > The goal here is to use the same font as the translate bubble on Windows (you > > can see the translate bubble by going to http://www.worldjournal.com). > However, the > > translate bubble has no close control (X) or title, so when we try to reuse > its > > formatting, we end up with a top margin that is too wide. In order to avoid > the > > top margin, we have to use a title. The title isn't really a title though - we > > want the title to be the same font as the rest of the bubble. Here's a bug > with > > mocks attached: crbug.com/383947 > > msw, any further thoughts on this? If there's a better way to do this in the > views bubble, I'm up for it, but I hunted around and couldn't find it. I know > the tab-modal had this same limitation at one point, but I don't think bubbles > got any workaround. Okay, so you'd like to use the built-in bubble code to show the (x) close button and the title ("<site> would like to use your"), but the font for the title is wrong. If you instead use a Label with the correct (default) font, it's placed too low (because it's in the content view, not in the frame view). If that's the case, then this is probably more or less the right way to go (at least for now). Please address the comments I've already made; instead of the base font flag functions, add BubbleDelegateView::GetTitleFontList (return the same value that's used for titles currently, and override this with return FontList(); in your bubble), add BubbleFrameView::SetTitleFontList, which is called by BubbleDelegateView similar to your ShowTitleInBaseFont... it's just a generalization of your current pattern to work for any FontList.
On 2014/06/16 19:17:33, msw wrote: > On 2014/06/16 15:09:23, Greg Billock wrote: > > On 2014/06/12 18:06:08, rubylee wrote: > > > On 2014/06/10 18:34:40, Greg Billock wrote: > > > > On 2014/06/10 18:16:05, msw wrote: > > > > > Can you set the BUG= and point me to mocks? It seems odd that we'd want > > > these > > > > > bubbles to use a different title font than other bubbles. Are you really > > > sure > > > > > that's the intent? (perhaps UX didn't mean to suggest a unique font, or > > > > perhaps > > > > > they want all bubble titles to be in the base font, instead of just the > > > > > permissions bubbles). > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/326963002/diff/1/ui/views/bubble/bubble_deleg... > > > > > File ui/views/bubble/bubble_delegate.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/326963002/diff/1/ui/views/bubble/bubble_deleg... > > > > > ui/views/bubble/bubble_delegate.h:54: virtual bool > > > > > ShouldShowWindowTitleInBaseFont() const; > > > > > How about GetTitleFontList? > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/326963002/diff/1/ui/views/bubble/bubble_frame... > > > > > File ui/views/bubble/bubble_frame_view.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/326963002/diff/1/ui/views/bubble/bubble_frame... > > > > > ui/views/bubble/bubble_frame_view.h:47: void ShowTitleInBaseFont(); > > > > > How about SetTitleFontList? > > > > > > > > Ruby, can you comment on the font sizing in bubbles thing? > > > > > > > > wrt using FontList, I went through various stages of grief in realizing > that > > I > > > > can't really see the title from the bubble delegate to set the font list > on > > it > > > > directly, to trying to figure out how to provide the font list directly > > (it'd > > > > take two methods, one for "should use" and the other for "use this", but > > then > > > > it'd likely require awkward defaults as well. In the end I just gave up > and > > > put > > > > in exactly the narrow hole I wanted, but it's not really ideal. I think > the > > > big > > > > problem is that "title" for a bubble doesn't really mean quite the same > > thing > > > as > > > > "title" for other widgets, where it'd be ridiculous to have font settings, > > so > > > > it's awkward. I thought about trying to attack that directly, but > ultimately > > > > shrank away in cowardly fear. > > > > > > The goal here is to use the same font as the translate bubble on Windows > (you > > > can see the translate bubble by going to http://www.worldjournal.com). > > However, the > > > translate bubble has no close control (X) or title, so when we try to reuse > > its > > > formatting, we end up with a top margin that is too wide. In order to avoid > > the > > > top margin, we have to use a title. The title isn't really a title though - > we > > > want the title to be the same font as the rest of the bubble. Here's a bug > > with > > > mocks attached: crbug.com/383947 > > > > msw, any further thoughts on this? If there's a better way to do this in the > > views bubble, I'm up for it, but I hunted around and couldn't find it. I know > > the tab-modal had this same limitation at one point, but I don't think bubbles > > got any workaround. > > Okay, so you'd like to use the built-in bubble code to show the (x) close button > and the title ("<site> would like to use your"), but the font for the title is > wrong. If you instead use a Label with the correct (default) font, it's placed > too low (because it's in the content view, not in the frame view). > > If that's the case, then this is probably more or less the right way to go (at > least for now). Please address the comments I've already made; instead of the > base font flag functions, add BubbleDelegateView::GetTitleFontList (return the > same value that's used for titles currently, and override this with return > FontList(); in your bubble), add BubbleFrameView::SetTitleFontList, which is > called by BubbleDelegateView similar to your ShowTitleInBaseFont... it's just a > generalization of your current pattern to work for any FontList. ok, done.
lgtm with a nit. https://codereview.chromium.org/326963002/diff/40001/ui/views/bubble/bubble_d... File ui/views/bubble/bubble_delegate.h (right): https://codereview.chromium.org/326963002/diff/40001/ui/views/bubble/bubble_d... ui/views/bubble/bubble_delegate.h:55: virtual const gfx::FontList& GetTitleFontList() const; nit: make this protected. (or order it below GetAnchorRect)
https://codereview.chromium.org/326963002/diff/40001/ui/views/bubble/bubble_d... File ui/views/bubble/bubble_delegate.h (right): https://codereview.chromium.org/326963002/diff/40001/ui/views/bubble/bubble_d... ui/views/bubble/bubble_delegate.h:55: virtual const gfx::FontList& GetTitleFontList() const; On 2014/06/16 22:58:32, msw wrote: > nit: make this protected. (or order it below GetAnchorRect) Good point. Moved below GetBubbleBounds, which is a very similar method.
The CQ bit was checked by gbillock@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/326963002/60001
The CQ bit was checked by gbillock@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/326963002/80001
FYI, I think the win_gpu_triggered_tests failures are real, and caused by this patch. Here are a few failing builds: http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered... http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered... http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered... It looks to me like the browser process is crashing intermittently. Could you please try to reproduce these problems locally? See http://www.chromium.org/developers/testing/gpu-testing for information on running these tests and contact me directly if you have any problems doing so. Thanks.
Please roll back. On Tue, Jun 17, 2014 at 11:41 AM, <kbr@chromium.org> wrote: > FYI, I think the win_gpu_triggered_tests failures are real, and caused by > this > patch. Here are a few failing builds: > > http://build.chromium.org/p/tryserver.chromium.gpu/ > builders/win_gpu_triggered_tests/builds/18136 > http://build.chromium.org/p/tryserver.chromium.gpu/ > builders/win_gpu_triggered_tests/builds/18152 > http://build.chromium.org/p/tryserver.chromium.gpu/ > builders/win_gpu_triggered_tests/builds/18221 > > It looks to me like the browser process is crashing intermittently. Could > you > please try to reproduce these problems locally? See > http://www.chromium.org/developers/testing/gpu-testing for information on > running these tests and contact me directly if you have any problems doing > so. > Thanks. > > > https://codereview.chromium.org/326963002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
I'm sorry, please roll back what? Your CL seems to be causing browser crashes and that's why it isn't passing the win_gpu_triggered_tests bot. Please do not ignore this failure. If you sidestep the CQ and break the GPU bots on the main waterfall your CL will be reverted. On 2014/06/17 19:05:54, Greg Billock wrote: > Please roll back. > > > On Tue, Jun 17, 2014 at 11:41 AM, <mailto:kbr@chromium.org> wrote: > > > FYI, I think the win_gpu_triggered_tests failures are real, and caused by > > this > > patch. Here are a few failing builds: > > > > http://build.chromium.org/p/tryserver.chromium.gpu/ > > builders/win_gpu_triggered_tests/builds/18136 > > http://build.chromium.org/p/tryserver.chromium.gpu/ > > builders/win_gpu_triggered_tests/builds/18152 > > http://build.chromium.org/p/tryserver.chromium.gpu/ > > builders/win_gpu_triggered_tests/builds/18221 > > > > It looks to me like the browser process is crashing intermittently. Could > > you > > please try to reproduce these problems locally? See > > http://www.chromium.org/developers/testing/gpu-testing for information on > > running these tests and contact me directly if you have any problems doing > > so. > > Thanks. > > > > > > https://codereview.chromium.org/326963002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by gbillock@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/326963002/100001
Message was sent while issue was closed.
Change committed as 278045 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
