Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10852015:
Initial constants for new ConstrainedWindow style. (Closed)
|
Created:
8 years, 4 months ago by groby-ooo-7-16 Modified:
8 years, 4 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionInitial constants for new ConstrainedWindow style.
BUG=134584
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152039
Patch Set 1 #
Total comments: 12
Patch Set 2 : Deleting bad code. #
Total comments: 2
Patch Set 3 : Account for review comments. #
Total comments: 5
Patch Set 4 : Removed empty namespace #
Total comments: 4
Patch Set 5 : Fix review issue. #Messages
Total messages: 15 (0 generated)
This is halfway between an actual changelist and just being an RFC. Not entirely sure if a) This is the right place. (constrained_window_style.h/cc instead?) b) We shouldn't treat fonts as gfx::Font instead of just passing around strings. Also, more constants for animations will be added.
http://codereview.chromium.org/10852015/diff/1/chrome/browser/ui/constrained_... File chrome/browser/ui/constrained_window.cc (right): http://codereview.chromium.org/10852015/diff/1/chrome/browser/ui/constrained_... chrome/browser/ui/constrained_window.cc:23: const char kFont[] = "Segoe UI"; you should be using the default font. this will be segoe UI on vista+ http://codereview.chromium.org/10852015/diff/1/chrome/browser/ui/constrained_... chrome/browser/ui/constrained_window.cc:25: const char kFont[] = "Lucida Grande"; same comment applies to other platforms. if you construct a gfx::Font(), you'll get the default. you can make it bigger, smaller, etc. http://codereview.chromium.org/10852015/diff/1/chrome/browser/ui/constrained_... File chrome/browser/ui/constrained_window.h (right): http://codereview.chromium.org/10852015/diff/1/chrome/browser/ui/constrained_... chrome/browser/ui/constrained_window.h:38: namespace ConstrainedWindowStyle { we don't use namespaces like this. according to google style, they must be based on directories/files. http://codereview.chromium.org/10852015/diff/1/chrome/browser/ui/constrained_... chrome/browser/ui/constrained_window.h:40: const int kVerticalPadding = 14; // top/bottom padding. I am also surprised to find all these constants in this cross platform file. I expect that they will vary slightly for each platform/port, even though the result will look very similar.
Planning to keep shared constants in the cross-platform file, just to avoid duplication. I'll follow your suggestion and move platform-specific stuff into platform-specific files. http://codereview.chromium.org/10852015/diff/1/chrome/browser/ui/constrained_... File chrome/browser/ui/constrained_window.cc (right): http://codereview.chromium.org/10852015/diff/1/chrome/browser/ui/constrained_... chrome/browser/ui/constrained_window.cc:23: const char kFont[] = "Segoe UI"; On 2012/08/03 23:09:07, Ben Goodger (Google) wrote: > you should be using the default font. this will be segoe UI on vista+ Have a request open with ui-leads to clarify. The mock calls for named fonts, not for the default fonts - but I assume you are right. http://codereview.chromium.org/10852015/diff/1/chrome/browser/ui/constrained_... chrome/browser/ui/constrained_window.cc:25: const char kFont[] = "Lucida Grande"; Yep. Will change as soon as I have clarity on named fonts vs. default fonts. On 2012/08/03 23:09:07, Ben Goodger (Google) wrote: > same comment applies to other platforms. > > if you construct a gfx::Font(), you'll get the default. you can make it bigger, > smaller, etc. http://codereview.chromium.org/10852015/diff/1/chrome/browser/ui/constrained_... File chrome/browser/ui/constrained_window.h (right): http://codereview.chromium.org/10852015/diff/1/chrome/browser/ui/constrained_... chrome/browser/ui/constrained_window.h:38: namespace ConstrainedWindowStyle { So make the constants part of the class, then? (They're specific to ConstrainedWindow, not browser::ui) On 2012/08/03 23:09:07, Ben Goodger (Google) wrote: > we don't use namespaces like this. according to google style, they must be based > on directories/files. http://codereview.chromium.org/10852015/diff/1/chrome/browser/ui/constrained_... chrome/browser/ui/constrained_window.h:40: const int kVerticalPadding = 14; // top/bottom padding. On 2012/08/03 23:09:07, Ben Goodger (Google) wrote: > I am also surprised to find all these constants in this cross platform file. I > expect that they will vary slightly for each platform/port, even though the > result will look very similar. According to the mocks, these are the same across platforms. See http://www.corp.google.com/~kenmoore/mocks/chromeos/Misc_2012/Dialogs/Markup2... I assume I'll break them up once they start differing.
http://codereview.chromium.org/10852015/diff/1005/chrome/browser/ui/constrain... File chrome/browser/ui/constrained_window.cc (right): http://codereview.chromium.org/10852015/diff/1005/chrome/browser/ui/constrain... chrome/browser/ui/constrained_window.cc:20: const SkColor kTextColor = SkColorSetRGB(3, 3, 3); #333 == #333333 in CSS, so I think this should be SkColorSetRGB(0x33, 0x33, 0x33).
http://codereview.chromium.org/10852015/diff/1/chrome/browser/ui/constrained_... File chrome/browser/ui/constrained_window.cc (right): http://codereview.chromium.org/10852015/diff/1/chrome/browser/ui/constrained_... chrome/browser/ui/constrained_window.cc:23: const char kFont[] = "Segoe UI"; Unified across platforms Done. On 2012/08/03 23:18:20, groby wrote: > On 2012/08/03 23:09:07, Ben Goodger (Google) wrote: > > you should be using the default font. this will be segoe UI on vista+ > > Have a request open with ui-leads to clarify. The mock calls for named fonts, > not for the default fonts - but I assume you are right. http://codereview.chromium.org/10852015/diff/1/chrome/browser/ui/constrained_... chrome/browser/ui/constrained_window.cc:25: const char kFont[] = "Lucida Grande"; On 2012/08/03 23:09:07, Ben Goodger (Google) wrote: > same comment applies to other platforms. > > if you construct a gfx::Font(), you'll get the default. you can make it bigger, > smaller, etc. Done. http://codereview.chromium.org/10852015/diff/1/chrome/browser/ui/constrained_... File chrome/browser/ui/constrained_window.h (right): http://codereview.chromium.org/10852015/diff/1/chrome/browser/ui/constrained_... chrome/browser/ui/constrained_window.h:38: namespace ConstrainedWindowStyle { On 2012/08/03 23:09:07, Ben Goodger (Google) wrote: > we don't use namespaces like this. according to google style, they must be based > on directories/files. Done. http://codereview.chromium.org/10852015/diff/1/chrome/browser/ui/constrained_... chrome/browser/ui/constrained_window.h:40: const int kVerticalPadding = 14; // top/bottom padding. On 2012/08/03 23:18:20, groby wrote: > On 2012/08/03 23:09:07, Ben Goodger (Google) wrote: > > I am also surprised to find all these constants in this cross platform file. I > > expect that they will vary slightly for each platform/port, even though the > > result will look very similar. > > According to the mocks, these are the same across platforms. See > http://www.corp.google.com/%7Ekenmoore/mocks/chromeos/Misc_2012/Dialogs/Marku... > > I assume I'll break them up once they start differing. > Done. http://codereview.chromium.org/10852015/diff/1005/chrome/browser/ui/constrain... File chrome/browser/ui/constrained_window.cc (right): http://codereview.chromium.org/10852015/diff/1005/chrome/browser/ui/constrain... chrome/browser/ui/constrained_window.cc:20: const SkColor kTextColor = SkColorSetRGB(3, 3, 3); On 2012/08/06 19:22:42, Mike Wittman wrote: > #333 == #333333 in CSS, so I think this should be SkColorSetRGB(0x33, 0x33, > 0x33). Done.
A couple comments on the font specification. http://codereview.chromium.org/10852015/diff/9003/chrome/browser/ui/constrain... File chrome/browser/ui/constrained_window.h (right): http://codereview.chromium.org/10852015/diff/9003/chrome/browser/ui/constrain... chrome/browser/ui/constrained_window.h:28: ui::ResourceBundle::BaseFont; On Windows, the specified size of the text font appears to be 1 point larger than BaseFont. I don't know if it's the same relationship on Mac or ChromeOS. While testing this in Views out I created a function: gfx::Font ConstrainedWindowViews::GetBodyFont() { return ui::ResourceBundle::GetSharedInstance().GetFont( ui::ResourceBundle::BaseFont).DeriveFont(1); } I *think* this is similar to what Ben was describing in his comment. http://codereview.chromium.org/10852015/diff/9003/chrome/browser/ui/constrain... chrome/browser/ui/constrained_window.h:31: ui::ResourceBundle::MediumFont; Similarly, on Windows the title font is 1 point larger than MediumFont. http://codereview.chromium.org/10852015/diff/9003/chrome/browser/ui/constrain... chrome/browser/ui/constrained_window.h:53: namespace ConstrainedWindowStyle { Delete this namespace?
http://codereview.chromium.org/10852015/diff/9003/chrome/browser/ui/constrain... File chrome/browser/ui/constrained_window.h (right): http://codereview.chromium.org/10852015/diff/9003/chrome/browser/ui/constrain... chrome/browser/ui/constrained_window.h:28: ui::ResourceBundle::BaseFont; Clarifying w/ UI leads. Last word was "use BaseFont/MediumFont" On 2012/08/14 00:14:23, Mike Wittman wrote: > On Windows, the specified size of the text font appears to be 1 point larger > than BaseFont. I don't know if it's the same relationship on Mac or ChromeOS. > > While testing this in Views out I created a function: > gfx::Font ConstrainedWindowViews::GetBodyFont() > { > return ui::ResourceBundle::GetSharedInstance().GetFont( > ui::ResourceBundle::BaseFont).DeriveFont(1); > } > > I *think* this is similar to what Ben was describing in his comment. http://codereview.chromium.org/10852015/diff/9003/chrome/browser/ui/constrain... chrome/browser/ui/constrained_window.h:53: namespace ConstrainedWindowStyle { On 2012/08/14 00:14:23, Mike Wittman wrote: > Delete this namespace? Done.
OK, Glen just confirmed to just use BaseFont/MediumFont, not differing font sizes. Ben, Mike: icanhaz LGTM?
lgtm
https://chromiumcodereview.appspot.com/10852015/diff/5004/chrome/browser/ui/c... File chrome/browser/ui/constrained_window.cc (right): https://chromiumcodereview.appspot.com/10852015/diff/5004/chrome/browser/ui/c... chrome/browser/ui/constrained_window.cc:10: return SkColorSetRGB(0xfb, 0xfb, 0xfb); I believe these are macros so you don't need to do them as functions. https://chromiumcodereview.appspot.com/10852015/diff/5004/chrome/browser/ui/c... File chrome/browser/ui/constrained_window.h (right): https://chromiumcodereview.appspot.com/10852015/diff/5004/chrome/browser/ui/c... chrome/browser/ui/constrained_window.h:33: SkColor GetBackgroundColor(); // Dialog background color. indentation is off for these two lines
https://chromiumcodereview.appspot.com/10852015/diff/5004/chrome/browser/ui/c... File chrome/browser/ui/constrained_window.cc (right): https://chromiumcodereview.appspot.com/10852015/diff/5004/chrome/browser/ui/c... chrome/browser/ui/constrained_window.cc:10: return SkColorSetRGB(0xfb, 0xfb, 0xfb); Yes and no :) They're macros in NDEBUG, but functions in DEBUG. And gcc will create static initializers in that case. On 2012/08/14 23:53:57, Ben Goodger (Google) wrote: > I believe these are macros so you don't need to do them as functions. https://chromiumcodereview.appspot.com/10852015/diff/5004/chrome/browser/ui/c... File chrome/browser/ui/constrained_window.h (right): https://chromiumcodereview.appspot.com/10852015/diff/5004/chrome/browser/ui/c... chrome/browser/ui/constrained_window.h:33: SkColor GetBackgroundColor(); // Dialog background color. On 2012/08/14 23:53:57, Ben Goodger (Google) wrote: > indentation is off for these two lines Done.
Weird. LGTM.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/groby@chromium.org/10852015/7003
Change committed as 152039 |
