Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(16)

Issue 2734113006: "Bootstrap" a toolkit-views Typography spec. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months, 2 weeks ago by tapted
Modified:
3 months, 1 week ago
Reviewers:
Peter Kasting, sky
CC:
chromium-reviews, tfarina, chrome-apps-syd-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

"Bootstrap" a toolkit-views Typography spec. Under Harmony, we want to abstract away decisions about font selection. Harmony has a set of styles as part of the spec that encapsulate: - Font (typeface) - Font size, weight and color - Line spacing ("leading") gfx::Font[List] can represent Font, size and weight, but not color or line spacing. Also callers typically _never_ care about typeface, but APIs using gfx::Font are unable to encapsulate this. One side-effect is that a lot of callers use gfx::Font::Derive(..) rather than ResourceBundle::GetFontWithDelta(..) and so miss out on caching benefits. The typography concepts are split into "TextContext" and "TextStyle". The former describes the location of the text, and the latter describes its current state (e.g. Disabled) or other appearance (e.g. a Hyperlink) in that context. This CL exposes a set of abstract typography concepts at views::style, and explores the impact on client code by refactoring the views::Label constructor that takes a gfx::FontList to take a Label::CustomFont instead. Where it makes sense, callers are migrated to typography concepts rather than use Label::CustomFont. The transformations typically look like: - ui::ResourceBundle::MediumFont -> views::style::CONTEXT_DIALOG_TITLE "15pt" - GetFontListWithDelta(ui::kTitleFontSizeDelta) -> views::style::CONTEXT_DIALOG_TITLE - GetFontListWithDelta(1) -> (chrome) CONTEXT_BODY_TEXT_LARGE "13pt" - [default constructor] or ResourceBundle::BaseFont -> CONTEXT_BODY_TEXT_LARGE or views::style::CONTEXT_LABEL "12pt" - ui::ResourceBundle::SmallFont -> (chrome) CONTEXT_DEPRECATED_SMALL (Harmony doesn't have an 11pt font in the spec) - ui::ResourceBundle::BoldFont -> (chrome) STYLE_EMPHASIZED (Harmony doesn't have a "bold" font in the spec) These transformations, and the default TypographyProvider, are chosen to effectively be a "no-op" so that no existing dialogs change font sizes or other properties. Follow-ups will: - Implement a Harmony TypographyProvider. - Require all Label constructors to take a typography concept or Label::CustomFont. - Remove Label::SetFontList, Label::Set*Color, etc. (use typography or merge into CustomFont). - Remove gfx::FontList from the API of other toolkit-views classes. - Audit remaining users of Label::CustomFont to see if they can be replaced by Typography concepts. - Remove ResourceBundle::FontStyle (SmallFont, etc.) - Remove ui/default_style.h (ui::kFooFontSizeDelta, etc.) BUG=691891, 701241 Review-Url: https://codereview.chromium.org/2734113006 Cr-Commit-Position: refs/heads/master@{#458261} Committed: https://chromium.googlesource.com/chromium/src/+/9577332120682b5400df78b16edcc0c759bb5881

Patch Set 1 : Get more insight #

Patch Set 2 : API shell #

Patch Set 3 : All builds on mac_views_browser #

Patch Set 4 : ChromeOS pass #

Patch Set 5 : fix windows #

Total comments: 40

Patch Set 6 : ExtendableEnum #

Total comments: 7

Patch Set 7 : Do TextStyle. Make Windows happy. #

Patch Set 8 : respond to comments #

Patch Set 9 : Rebase (conflict in test/BUILD.gn) #

Patch Set 10 : Shred Typography class, follow-up on some other things. fix compile. #

Total comments: 10

Patch Set 11 : Red builds give tapted ocd #

Patch Set 12 : delegate to delegate delegate. enummify. lint. make gcc happy #

Patch Set 13 : Fix tests failing because they have no ViewsDelegate, but it looks like I also need to rebase #

Patch Set 14 : Rebase #

Total comments: 78

Patch Set 15 : respond to comments #

Total comments: 12

Patch Set 16 : pkasting comments #

Patch Set 17 : Rebase (conflict in layout_delegate.h due to r457774) #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+823 lines, -114 lines) Patch
M chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc View 1 2 3 4 5 6 7 2 chunks +4 lines, -4 lines 0 comments Download
M chrome/browser/first_run/try_chrome_dialog_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +3 lines, -3 lines 0 comments Download
M chrome/browser/ui/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/accessibility/invert_bubble_view.cc View 1 2 3 4 5 6 7 1 chunk +3 lines, -2 lines 0 comments Download
M chrome/browser/ui/views/apps/app_info_dialog/app_info_header_panel.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +2 lines, -5 lines 0 comments Download
M chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +3 lines, -1 line 0 comments Download
M chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc View 1 2 3 4 5 6 7 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/views/chrome_views_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/ui/views/chrome_views_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/content_setting_bubble_contents.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 4 chunks +10 lines, -10 lines 0 comments Download
M chrome/browser/ui/views/create_application_shortcut_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +3 lines, -4 lines 0 comments Download
M chrome/browser/ui/views/crypto_module_password_dialog_view_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/ui/views/extensions/extension_install_dialog_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +14 lines, -9 lines 0 comments Download
M chrome/browser/ui/views/first_run_bubble.cc View 1 2 3 4 5 6 7 1 chunk +6 lines, -4 lines 0 comments Download
M chrome/browser/ui/views/frame/glass_browser_frame_view.cc View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/frame/opaque_browser_frame_view.cc View 1 2 3 4 5 6 7 1 chunk +3 lines, -3 lines 0 comments Download
A chrome/browser/ui/views/harmony/chrome_typography.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +58 lines, -0 lines 0 comments Download
A chrome/browser/ui/views/harmony/chrome_typography.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +43 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/harmony/layout_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 3 chunks +10 lines, -2 lines 0 comments Download
M chrome/browser/ui/views/harmony/layout_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +8 lines, -0 lines 0 comments Download
A chrome/browser/ui/views/harmony/layout_delegate_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +231 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/ime/ime_warning_bubble_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +5 lines, -7 lines 0 comments Download
M chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/location_bar/keyword_hint_view.cc View 1 2 3 4 5 6 7 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/views/location_bar/location_bar_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/ui/views/location_bar/location_icon_view.cc View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/page_info/website_settings_popup_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +2 lines, -3 lines 0 comments Download
M chrome/browser/ui/views/passwords/credentials_item_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +12 lines, -5 lines 0 comments Download
M chrome/browser/ui/views/subtle_notification_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 8 chunks +13 lines, -15 lines 0 comments Download
M chrome/browser/ui/views/sync/bubble_sync_promo_view_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +3 lines, -2 lines 0 comments Download
M chrome/browser/ui/views/validation_message_bubble_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +3 lines, -2 lines 0 comments Download
M chrome/test/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +1 line, -0 lines 0 comments Download
M ui/app_list/views/speech_view.cc View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M ui/chromeos/ime/infolist_window.cc View 1 2 3 4 5 6 7 1 chunk +2 lines, -2 lines 0 comments Download
M ui/gfx/render_text_unittest.cc View 1 2 3 6 1 chunk +39 lines, -0 lines 2 comments Download
M ui/message_center/views/notifier_settings_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -2 lines 0 comments Download
M ui/views/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +5 lines, -1 line 0 comments Download
M ui/views/bubble/bubble_frame_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -3 lines 0 comments Download
M ui/views/controls/button/label_button.cc View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M ui/views/controls/label.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +28 lines, -1 line 0 comments Download
M ui/views/controls/label.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +9 lines, -6 lines 0 comments Download
M ui/views/controls/styled_label_unittest.cc View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M ui/views/controls/tabbed_pane/tabbed_pane.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -5 lines 0 comments Download
M ui/views/controls/textfield/textfield.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +1 line, -1 line 0 comments Download
M ui/views/mus/aura_init.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +7 lines, -0 lines 0 comments Download
A ui/views/style/typography.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +88 lines, -0 lines 3 comments Download
A ui/views/style/typography.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +42 lines, -0 lines 0 comments Download
A ui/views/style/typography_provider.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +66 lines, -0 lines 0 comments Download
A ui/views/style/typography_provider.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +55 lines, -0 lines 0 comments Download
M ui/views/test/test_views_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +3 lines, -0 lines 0 comments Download
M ui/views/test/test_views_delegate_aura.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +4 lines, -0 lines 0 comments Download
M ui/views/test/test_views_delegate_mac.mm View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +4 lines, -0 lines 0 comments Download
M ui/views/views_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +5 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 143 (117 generated)
tapted
Hi Peter and sky, please take a look. I'm calling this a "Bootstrap" since it's ...
3 months, 2 weeks ago (2017-03-14 09:56:18 UTC) #43
sky
Maybe the embedder expanding the set of styles/contexts should happen like contents NotificationType? https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typography.h File ...
3 months, 1 week ago (2017-03-14 17:31:47 UTC) #46
Peter Kasting
https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typography.h File ui/views/style/typography.h (right): https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typography.h#newcode22 ui/views/style/typography.h:22: enum class TextContext { On 2017/03/14 17:31:47, sky wrote: ...
3 months, 1 week ago (2017-03-14 19:35:49 UTC) #47
Peter Kasting
I didn't look at everything, just a scattering of consumers + the typography and label ...
3 months, 1 week ago (2017-03-14 23:53:34 UTC) #48
tapted
https://codereview.chromium.org/2734113006/diff/260001/ui/views/style/typography.h File ui/views/style/typography.h (right): https://codereview.chromium.org/2734113006/diff/260001/ui/views/style/typography.h#newcode43 ui/views/style/typography.h:43: class VIEWS_EXPORT TextContext : public ExtendableEnum<TextContext> { Still working ...
3 months, 1 week ago (2017-03-15 00:30:05 UTC) #49
Peter Kasting
https://codereview.chromium.org/2734113006/diff/260001/ui/views/style/typography.h File ui/views/style/typography.h (right): https://codereview.chromium.org/2734113006/diff/260001/ui/views/style/typography.h#newcode43 ui/views/style/typography.h:43: class VIEWS_EXPORT TextContext : public ExtendableEnum<TextContext> { On 2017/03/15 ...
3 months, 1 week ago (2017-03-15 06:38:48 UTC) #59
tapted
PTAL https://codereview.chromium.org/2734113006/diff/240001/chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc (right): https://codereview.chromium.org/2734113006/diff/240001/chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc#newcode506 chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:506: views::Label::CustomFont{rb.GetFontListWithDelta( On 2017/03/14 23:53:33, Peter Kasting wrote: > ...
3 months, 1 week ago (2017-03-15 08:26:25 UTC) #70
Peter Kasting
https://codereview.chromium.org/2734113006/diff/240001/chrome/browser/ui/views/content_setting_bubble_contents.cc File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/2734113006/diff/240001/chrome/browser/ui/views/content_setting_bubble_contents.cc#newcode236 chrome/browser/ui/views/content_setting_bubble_contents.cc:236: : views::TextContext::DIALOG_TEXT_SMALL; On 2017/03/15 08:26:24, tapted wrote: > On ...
3 months, 1 week ago (2017-03-15 10:01:49 UTC) #77
tapted
(didn't get everything, and I have some compilers to poke still) Tomorrow, I'll try out ...
3 months, 1 week ago (2017-03-15 11:04:07 UTC) #78
Peter Kasting
https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typography.cc File ui/views/style/typography.cc (right): https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typography.cc#newcode80 ui/views/style/typography.cc:80: (new DefaultProvider)); On 2017/03/15 11:04:06, tapted wrote: > On ...
3 months, 1 week ago (2017-03-15 17:52:47 UTC) #79
tapted
(just following up on 3 threads here with some "next steps") https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typography.cc File ui/views/style/typography.cc (right): ...
3 months, 1 week ago (2017-03-15 22:43:35 UTC) #80
Peter Kasting
https://codereview.chromium.org/2734113006/diff/260001/ui/views/style/typography.h File ui/views/style/typography.h (right): https://codereview.chromium.org/2734113006/diff/260001/ui/views/style/typography.h#newcode43 ui/views/style/typography.h:43: class VIEWS_EXPORT TextContext : public ExtendableEnum<TextContext> { On 2017/03/15 ...
3 months, 1 week ago (2017-03-16 00:17:47 UTC) #85
tapted
PTAL - I think I've addressed everything - TypographyProvider now provided by the ViewsDelegate - ...
3 months, 1 week ago (2017-03-16 11:13:40 UTC) #107
sky
https://codereview.chromium.org/2734113006/diff/500001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/2734113006/diff/500001/ui/views/controls/label.cc#newcode54 ui/views/controls/label.cc:54: Init(text, typography::GetFont(text_context, text_style)); Is the plan to change other ...
3 months, 1 week ago (2017-03-16 16:23:51 UTC) #108
Peter Kasting
> I'm was too wary of making a bitmask - I think it would add ...
3 months, 1 week ago (2017-03-16 19:33:13 UTC) #109
sky
https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typography.h File ui/views/style/typography.h (right): https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typography.h#newcode45 ui/views/style/typography.h:45: VIEWS_TEXT_STYLE_START = TEXT_CONTEXT_MAX, On 2017/03/16 19:33:13, Peter Kasting wrote: ...
3 months, 1 week ago (2017-03-16 20:48:47 UTC) #110
Peter Kasting
I like how this is working in general. https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/views/extensions/extension_install_dialog_view.cc File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc (right): https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/views/extensions/extension_install_dialog_view.cc#newcode235 chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:235: int ...
3 months, 1 week ago (2017-03-17 02:26:01 UTC) #111
tapted
https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/views/content_setting_bubble_contents.cc File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/views/content_setting_bubble_contents.cc#newcode236 chrome/browser/ui/views/content_setting_bubble_contents.cc:236: title_context = CONTEXT_DIALOG_TEXT_SMALL; On 2017/03/16 19:33:13, Peter Kasting wrote: ...
3 months, 1 week ago (2017-03-17 10:33:11 UTC) #120
Peter Kasting
LGTM https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/views/harmony/chrome_typography.h File chrome/browser/ui/views/harmony/chrome_typography.h (right): https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/views/harmony/chrome_typography.h#newcode17 chrome/browser/ui/views/harmony/chrome_typography.h:17: CONTEXT_DIALOG_MESSAGE, On 2017/03/17 10:33:10, tapted wrote: > On ...
3 months, 1 week ago (2017-03-17 20:59:39 UTC) #123
tapted
https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/views/harmony/chrome_typography.h File chrome/browser/ui/views/harmony/chrome_typography.h (right): https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/views/harmony/chrome_typography.h#newcode17 chrome/browser/ui/views/harmony/chrome_typography.h:17: CONTEXT_DIALOG_MESSAGE, On 2017/03/17 20:59:39, Peter Kasting wrote: > On ...
3 months, 1 week ago (2017-03-20 07:33:36 UTC) #132
sky
Two minor questions. https://codereview.chromium.org/2734113006/diff/580001/ui/gfx/render_text_unittest.cc File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/2734113006/diff/580001/ui/gfx/render_text_unittest.cc#newcode2346 ui/gfx/render_text_unittest.cc:2346: // Disabled since this relies on ...
3 months, 1 week ago (2017-03-20 16:33:12 UTC) #133
Peter Kasting
https://codereview.chromium.org/2734113006/diff/580001/ui/views/style/typography.h File ui/views/style/typography.h (right): https://codereview.chromium.org/2734113006/diff/580001/ui/views/style/typography.h#newcode56 ui/views/style/typography.h:56: STYLE_PRIMARY = VIEWS_TEXT_STYLE_START, On 2017/03/20 16:33:11, sky wrote: > ...
3 months, 1 week ago (2017-03-20 20:00:34 UTC) #134
tapted
https://codereview.chromium.org/2734113006/diff/580001/ui/gfx/render_text_unittest.cc File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/2734113006/diff/580001/ui/gfx/render_text_unittest.cc#newcode2346 ui/gfx/render_text_unittest.cc:2346: // Disabled since this relies on machine configuration. http://crbug.com/701241. ...
3 months, 1 week ago (2017-03-20 22:31:43 UTC) #136
sky
LGTM
3 months, 1 week ago (2017-03-20 23:55:04 UTC) #137
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2734113006/580001
3 months, 1 week ago (2017-03-21 00:43:08 UTC) #140
commit-bot: I haz the power
3 months, 1 week ago (2017-03-21 00:52:46 UTC) #143
Message was sent while issue was closed.
Committed patchset #17 (id:580001) as
https://chromium.googlesource.com/chromium/src/+/9577332120682b5400df78b16edc...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cb946e318