Refactor ChromeTypographyProvider::GetFont |
|||||
Issue description1. Make ChromeTypographyProvider::GetFont default to DefaultTypographyProvider::GetFont. 2. Move handling of views::style enums used in ChromeTypographyProvider to the base class, DefaultTypographyProvider.
,
Nov 13
,
Nov 13
TypographyProvider owners (msw, taped) are you ok with this change? I see that DefaultTypographyProvider's purpose is to "provide values to match pre-Harmony constants for the given contexts so that old UI does not change". To me, this means we probably should not move the views::style enums referenced in ChromeTypographyProvider to DefaultTypographyProvider. This could modify the behavior of older UI. WDYT?
,
Nov 13
I'd think Views *should* use Harmony values, I'm not sure why we'd want anything to look "pre-Harmony" at this point. It would help to audit the UI surfaces affected and the specific appearance changes that this would cause. pkasting@, bsep@, any thoughts or ideas who to ping about the intent here? This refactoring is not P1.
,
Nov 13
See this CL comment thread for a bit more context: https://crrev.com/c/1329672/1/chrome/browser/ui/views/chrome_typography_provider.cc#136
,
Nov 13
Nothing should look pre-Harmony. However, in the past, sky was insistent that concepts only used in chrome/ should be handled by providers defined in chrome/ and not handled by code in ui/views/. So I would not proceed with the larger refactoring here without sky's approval.
,
Nov 13
Adding sky@. The contexts handled explicitly in ChromeTypographyProvider::GetFont look agnostic to Chrome, IMO: https://cs.chromium.org/chromium/src/chrome/browser/ui/views/chrome_typography_provider.cc?rcl=e0f977777fecfecda53b05e5dcc50f9281204d66&l=114 Even CONTEXT_HEADLINE_OVERSIZED itself seems generic enough for Views (now in ApplyAshFontStyles): https://cs.chromium.org/chromium/src/ash/public/cpp/ash_typography.cc?rcl=e349a45e258782e8e00713b0690f38d6001fc650&l=23 I'd also imagine that CONTEXT_WINDOWS10_NATIVE is not Chrome-specific (now in ApplyCommonFontStyles): https://cs.chromium.org/chromium/src/chrome/browser/ui/views/chrome_typography.cc?rcl=e349a45e258782e8e00713b0690f38d6001fc650&l=82 Other things like launcher, toolbar, omnibox, definitely seem more chrome-specific. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by newcomer@chromium.org
, Nov 13